Cause crash in FlutterJNI if invoked on non-main thread in debug mode (#31263). #8830
Conversation
| private void ensureRunningOnMainThread() { | ||
| if (BuildConfig.DEBUG && Looper.myLooper() != Looper.getMainLooper()) { | ||
| throw new RuntimeException( | ||
| "Attempted to invoke a @UiThread method on a different thread (" |
There was a problem hiding this comment.
Is there any extra guidance you can give here or docs you can point to about why this is an issue or maybe how this happened? I'd have trouble debugging the actual problem from just from seeing this.
There was a problem hiding this comment.
The solution is just to call the method on the main thread, hence the @UiThread annotations on all of them. Is there a clearer message to communicate that?
| "Attempted to invoke a @UiThread method on a different thread (" | ||
| + Thread.currentThread().getName() + ")" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Worth an error log in release builds?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
hah, I guess it's actually Log.wtf
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't know the answer to that.
There was a problem hiding this comment.
Invoking these APIs from a thread other than the main thread is a race condition that will yield unpredictable behavior.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
getMainLooper requires a synchronization: https://github.com/aosp-mirror/platform_frameworks_base/blob/master/core/java/android/os/Looper.java#L133
|
@dnfield @jason-simmons @mklim I changed the exception to throw all the time, and cached the main looper. @mklim I tried a different message. Please let me know if it is now clear. |
flutter/engine@8811392...2c9e37c git log 8811392..2c9e37c --no-merges --oneline 2c9e37c Cause crash in FlutterJNI if invoked on non-main thread (#31263). (flutter/engine#8830) 7ce2666 Guard Android logs (flutter/engine#8824) 8584da2 Roll src/third_party/skia dc19391eef52..70aab823547a (8 commits) (flutter/engine#8836) 1cdc163 Roll src/third_party/dart 1577b95c93..dbe80f3397 (43 commits) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (amirha@google.com), and stop the roller if necessary.
The Android JavaScriptChannel wasn't checking correctly if messages are received on the main thread and took the branch that assumes we are on the main thread. flutter/engine#8830 causes this behavior to fail (which is good). So now we can also add an e2e test that fails with the buggy behavior.
The Android JavaScriptChannel wasn't checking correctly if messages are received on the main thread and took the branch that assumes we are on the main thread. flutter/engine#8830 causes this behavior to fail (which is good). So now we can also add an e2e test that fails with the buggy behavior.
The Android JavaScriptChannel wasn't checking correctly if messages are received on the main thread and took the branch that assumes we are on the main thread. flutter/engine#8830 causes this behavior to fail (which is good). So now we can also add an e2e test that fails with the buggy behavior.
The Android JavaScriptChannel wasn't checking correctly if messages are received on the main thread and took the branch that assumes we are on the main thread. flutter/engine#8830 causes this behavior to fail (which is good). So now we can also add an e2e test that fails with the buggy behavior.
|
I fixed this crash by remove FutureWidget in code. Hope to help your. |
|
Does anyone know in which Flutter version was this shipped to? |
8 participants
Cause crash in FlutterJNI if invoked on non-main thread in debug mode (#31263).