Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Cause crash in FlutterJNI if invoked on non-main thread in debug mode (#31263). #8830

Merged
matthew-carroll merged 2 commits into from
May 3, 2019
Merged

Cause crash in FlutterJNI if invoked on non-main thread in debug mode (#31263). #8830

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f30847a
Cause crash in FlutterJNI if invoked on non-main thread in debug mode…
matthew-carroll May 3, 2019
a5bef04
Changed exception to throw all the time. Updated message.
matthew-carroll May 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import android.content.res.AssetManager;
import android.graphics.Bitmap;
import android.graphics.SurfaceTexture;
import android.os.Looper;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.annotation.UiThread;
Expand All @@ -17,6 +18,7 @@
import java.util.HashSet;
import java.util.Set;

import io.flutter.BuildConfig;
import io.flutter.embedding.engine.dart.PlatformMessageHandler;
import io.flutter.embedding.engine.FlutterEngine.EngineLifecycleListener;
import io.flutter.embedding.engine.renderer.FlutterRenderer;
Expand Down Expand Up @@ -102,6 +104,13 @@ public class FlutterJNI {
private PlatformMessageHandler platformMessageHandler;
private final Set<EngineLifecycleListener> engineLifecycleListeners = new HashSet<>();
private final Set<OnFirstFrameRenderedListener> firstFrameListeners = new HashSet<>();
private final Looper mainLooper; // cached to avoid synchronization on repeat access.

public FlutterJNI() {
// We cache the main looper so that we can ensure calls are made on the main thread
// without consistently paying the synchronization cost of getMainLooper().
mainLooper = Looper.getMainLooper();
}

/**
* Sets the {@link FlutterRenderer.RenderSurface} delegate for the attached Flutter context.
Expand All @@ -119,6 +128,7 @@ public class FlutterJNI {
*/
@UiThread
public void setRenderSurface(@Nullable FlutterRenderer.RenderSurface renderSurface) {
ensureRunningOnMainThread();
this.renderSurface = renderSurface;
}

Expand All @@ -134,6 +144,7 @@ public void setRenderSurface(@Nullable FlutterRenderer.RenderSurface renderSurfa
*/
@UiThread
public void setAccessibilityDelegate(@Nullable AccessibilityDelegate accessibilityDelegate) {
ensureRunningOnMainThread();
this.accessibilityDelegate = accessibilityDelegate;
}

Expand All @@ -146,6 +157,7 @@ public void setAccessibilityDelegate(@Nullable AccessibilityDelegate accessibili
@SuppressWarnings("unused")
@UiThread
private void updateSemantics(ByteBuffer buffer, String[] strings) {
ensureRunningOnMainThread();
if (accessibilityDelegate != null) {
accessibilityDelegate.updateSemantics(buffer, strings);
}
Expand All @@ -163,6 +175,7 @@ private void updateSemantics(ByteBuffer buffer, String[] strings) {
@SuppressWarnings("unused")
@UiThread
private void updateCustomAccessibilityActions(ByteBuffer buffer, String[] strings) {
ensureRunningOnMainThread();
if (accessibilityDelegate != null) {
accessibilityDelegate.updateCustomAccessibilityActions(buffer, strings);
}
Expand All @@ -173,6 +186,7 @@ private void updateCustomAccessibilityActions(ByteBuffer buffer, String[] string
@SuppressWarnings("unused")
@UiThread
private void onFirstFrame() {
ensureRunningOnMainThread();
if (renderSurface != null) {
renderSurface.onFirstFrameRendered();
}
Expand Down Expand Up @@ -209,6 +223,7 @@ private void onFirstFrame() {
*/
@UiThread
public void setPlatformMessageHandler(@Nullable PlatformMessageHandler platformMessageHandler) {
ensureRunningOnMainThread();
this.platformMessageHandler = platformMessageHandler;
}

Expand All @@ -232,28 +247,33 @@ private void handlePlatformMessageResponse(int replyId, byte[] reply) {

@UiThread
public void addEngineLifecycleListener(@NonNull EngineLifecycleListener engineLifecycleListener) {
ensureRunningOnMainThread();
engineLifecycleListeners.add(engineLifecycleListener);
}

@UiThread
public void removeEngineLifecycleListener(@NonNull EngineLifecycleListener engineLifecycleListener) {
ensureRunningOnMainThread();
engineLifecycleListeners.remove(engineLifecycleListener);
}

@UiThread
public void addOnFirstFrameRenderedListener(@NonNull OnFirstFrameRenderedListener listener) {
ensureRunningOnMainThread();
firstFrameListeners.add(listener);
}

@UiThread
public void removeOnFirstFrameRenderedListener(@NonNull OnFirstFrameRenderedListener listener) {
ensureRunningOnMainThread();
firstFrameListeners.remove(listener);
}

// TODO(mattcarroll): rename comments after refactor is done and their origin no longer matters (https://github.com/flutter/flutter/issues/25533)
//----- Start from FlutterView -----
@UiThread
public void onSurfaceCreated(@NonNull Surface surface) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeSurfaceCreated(nativePlatformViewId, surface);
}
Expand All @@ -262,6 +282,7 @@ public void onSurfaceCreated(@NonNull Surface surface) {

@UiThread
public void onSurfaceChanged(int width, int height) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeSurfaceChanged(nativePlatformViewId, width, height);
}
Expand All @@ -270,6 +291,7 @@ public void onSurfaceChanged(int width, int height) {

@UiThread
public void onSurfaceDestroyed() {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeSurfaceDestroyed(nativePlatformViewId);
}
Expand All @@ -290,6 +312,7 @@ public void setViewportMetrics(
int physicalViewInsetBottom,
int physicalViewInsetLeft
) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeSetViewportMetrics(
nativePlatformViewId,
Expand Down Expand Up @@ -324,6 +347,7 @@ private native void nativeSetViewportMetrics(

@UiThread
public Bitmap getBitmap() {
ensureRunningOnMainThread();
ensureAttachedToNative();
return nativeGetBitmap(nativePlatformViewId);
}
Expand All @@ -332,6 +356,7 @@ public Bitmap getBitmap() {

@UiThread
public void dispatchPointerDataPacket(ByteBuffer buffer, int position) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeDispatchPointerDataPacket(nativePlatformViewId, buffer, position);
}
Expand All @@ -358,6 +383,7 @@ public void dispatchSemanticsAction(int id, @NonNull AccessibilityBridge.Action

@UiThread
public void dispatchSemanticsAction(int id, int action, ByteBuffer args, int argsPosition) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeDispatchSemanticsAction(nativePlatformViewId, id, action, args, argsPosition);
}
Expand All @@ -372,6 +398,7 @@ private native void nativeDispatchSemanticsAction(

@UiThread
public void setSemanticsEnabled(boolean enabled) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeSetSemanticsEnabled(nativePlatformViewId, enabled);
}
Expand All @@ -380,6 +407,7 @@ public void setSemanticsEnabled(boolean enabled) {

@UiThread
public void setAccessibilityFeatures(int flags) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeSetAccessibilityFeatures(nativePlatformViewId, flags);
}
Expand All @@ -388,6 +416,7 @@ public void setAccessibilityFeatures(int flags) {

@UiThread
public void registerTexture(long textureId, SurfaceTexture surfaceTexture) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeRegisterTexture(nativePlatformViewId, textureId, surfaceTexture);
}
Expand All @@ -396,6 +425,7 @@ public void registerTexture(long textureId, SurfaceTexture surfaceTexture) {

@UiThread
public void markTextureFrameAvailable(long textureId) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeMarkTextureFrameAvailable(nativePlatformViewId, textureId);
}
Expand All @@ -404,6 +434,7 @@ public void markTextureFrameAvailable(long textureId) {

@UiThread
public void unregisterTexture(long textureId) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeUnregisterTexture(nativePlatformViewId, textureId);
}
Expand All @@ -419,6 +450,7 @@ public boolean isAttached() {

@UiThread
public void attachToNative(boolean isBackgroundView) {
ensureRunningOnMainThread();
ensureNotAttachedToNative();
nativePlatformViewId = nativeAttach(this, isBackgroundView);
}
Expand All @@ -427,6 +459,7 @@ public void attachToNative(boolean isBackgroundView) {

@UiThread
public void detachFromNativeAndReleaseResources() {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeDestroy(nativePlatformViewId);
nativePlatformViewId = null;
Expand All @@ -441,6 +474,7 @@ public void runBundleAndSnapshotFromLibrary(
@Nullable String pathToEntrypointFunction,
@NonNull AssetManager assetManager
) {
ensureRunningOnMainThread();
ensureAttachedToNative();
nativeRunBundleAndSnapshotFromLibrary(
nativePlatformViewId,
Expand All @@ -461,6 +495,7 @@ private native void nativeRunBundleAndSnapshotFromLibrary(

@UiThread
public void dispatchEmptyPlatformMessage(String channel, int responseId) {
ensureRunningOnMainThread();
if (isAttached()) {
nativeDispatchEmptyPlatformMessage(nativePlatformViewId, channel, responseId);
} else {
Expand All @@ -477,6 +512,7 @@ private native void nativeDispatchEmptyPlatformMessage(

@UiThread
public void dispatchPlatformMessage(String channel, ByteBuffer message, int position, int responseId) {
ensureRunningOnMainThread();
if (isAttached()) {
nativeDispatchPlatformMessage(
nativePlatformViewId,
Expand All @@ -501,6 +537,7 @@ private native void nativeDispatchPlatformMessage(

@UiThread
public void invokePlatformMessageEmptyResponseCallback(int responseId) {
ensureRunningOnMainThread();
if (isAttached()) {
nativeInvokePlatformMessageEmptyResponseCallback(nativePlatformViewId, responseId);
} else {
Expand All @@ -516,6 +553,7 @@ private native void nativeInvokePlatformMessageEmptyResponseCallback(

@UiThread
public void invokePlatformMessageResponseCallback(int responseId, ByteBuffer message, int position) {
ensureRunningOnMainThread();
if (isAttached()) {
nativeInvokePlatformMessageResponseCallback(
nativePlatformViewId,
Expand Down Expand Up @@ -560,6 +598,15 @@ private void ensureAttachedToNative() {
}
}

private void ensureRunningOnMainThread() {
if (Looper.myLooper() != mainLooper) {
throw new RuntimeException(
"Methods marked with @UiThread must be executed on the main thread. Current thread: "
+ Thread.currentThread().getName()
);
}
Copy link
Contributor

@mklim mklim May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth an error log in release builds?

Copy link
Contributor

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear on why we'd guard this as debug only - it will crash in both release and debug, right?

Maybe just do a Log.f here for all modes?

Copy link
Contributor

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah, I guess it's actually Log.wtf

Copy link
Contributor Author

@matthew-carroll matthew-carroll May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jason-simmons recommended throwing in debug mode. The bug that lead to this change seemed to suggest that failures were intermittent, meaning some of these calls can survive under certain conditions. If a developer has already shipped a broken app, do we want to force 100% crashes in that environment?

Copy link
Member

@jason-simmons jason-simmons May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods should never be called outside the main thread. I was hoping to avoid the cost of the thread check in release mode, but if that cost is trivial then we can always do it.

Copy link
Contributor

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I misunderstanding that this call would always result in a crash under this condition? It seems more likely that the intermittent nature is because of some other code path leading to this call rather than that this call somehow sometimes doesn't crash.

Copy link
Contributor Author

@matthew-carroll matthew-carroll May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the answer to that.

Copy link
Member

@jason-simmons jason-simmons May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invoking these APIs from a thread other than the main thread is a race condition that will yield unpredictable behavior.

Copy link
Contributor

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be pretty cheap if we cache the result of Looper.getMainLooper() - as long as that's safe to do (I think it is)

Copy link
Contributor

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getMainLooper requires a synchronization: https://github.com/aosp-mirror/platform_frameworks_base/blob/master/core/java/android/os/Looper.java#L133

}

/**
* Delegate responsible for creating and updating Android-side caches of Flutter's semantics
* tree and custom accessibility actions.
Expand Down