Fix array == null error in AccessibilityBridge #35540

GuaiYiHu · 2022-08-19T15:24:58Z

08-16 06:23:59.641 23012 23012 E flutter : [ERROR:flutter/fml/platform/android/jni_util.cc(182)] java.lang.IllegalArgumentException: array == null
at android.opengl.Matrix.multiplyMM(Native Method)
at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:28)
at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:240)
at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:240)
at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:240)
at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:240)
at io.flutter.view.AccessibilityBridge$SemanticsNode.access$4900(Unknown Source:0)
at io.flutter.view.AccessibilityBridge.updateSemantics(Unknown Source:192)
at io.flutter.view.AccessibilityBridge$1.updateSemantics(Unknown Source:21)
at io.flutter.embedding.engine.FlutterJNI.updateSemantics(Unknown Source:9)
at android.os.MessageQueue.nativePollOnce(Native
08-16 06:23:59.641 23012 23012 F flutter : [FATAL:flutter/shell/platform/android/platform_view_android_jni_impl.cc(1214)] Check failed: fml::jni::CheckException(env).

Fix array == null error

flutter/flutter#89834
flutter/flutter#31139

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-08-19T15:25:01Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

google-cla · 2022-08-19T15:25:02Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

GuaiYiHu · 2022-08-22T10:41:13Z

Will any one review this commit?

chinmaygarde · 2022-08-25T20:11:44Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

          globalTransform = new float[16];
        }
+        if (transform == null) {
+          transform = new float[16];


Shouldn't this and the thing above be initialized to an identity transformation?

cc @Hixie who may have context as the author of the original null check that initialized all matrix elements to zero.

better than crash

cc @Hixie who may have context as the author of the original null check that initialized all matrix elements to zero.

Hope merge, plz

This should be initialized during the SemanticsNode.updateWith. The only possible case I can think of is the framework sent a semantics node with child that is not in the update list. This feels like a bug in the framework side. Do you have a reproducible code?

zanderso · 2022-09-08T20:32:23Z

From PR triage: @stuartmorgan Do you know who would be the right reviewer for this change? Should the concern from @chinmaygarde be addressed?

stuartmorgan-g · 2022-09-13T17:26:33Z

@chunhtai Are you familiar with this code? It looks like you are one of the people who has been most active in this file.

GaryQian · 2022-09-20T17:39:52Z

@GuaiYiHu Can you provide a reproduction of the crash so we can investigate what the correct course of action is? Either mitigate the crash with this PR or fix the upstream root bug.

chinmaygarde · 2022-09-22T20:23:12Z

I think this is a safe fix either way and the same pattern followed elsewhere is being followed here.

chunhtai · 2022-09-22T20:47:14Z

I am ok if we add an assert so that we still have a chance to catch the root cause but avoiding crash in production. @GuaiYiHu Can you add an assert that it should never be null?

GuaiYiHu · 2022-09-23T00:06:10Z

I am ok if we add an assert so that we still have a chance to catch the root cause but avoiding crash in production. @GuaiYiHu Can you add an assert that it should never be null?

sure

zanderso · 2022-09-29T20:10:37Z

From PR review triage: @GuaiYiHu are you interested in continuing with this PR? If so, it looks like the next step is to follow-up on the feedback from @chunhtai.

GuaiYiHu · 2022-09-29T23:31:53Z

From PR review triage: @GuaiYiHu are you interested in continuing with this PR? If so, it looks like the next step is to follow-up on the feedback from @chunhtai.

busy these days, will do this on my holiday from tomorrow.😃

GuaiYiHu · 2022-09-30T04:21:49Z

From PR review triage: @GuaiYiHu are you interested in continuing with this PR? If so, it looks like the next step is to follow-up on the feedback from @chunhtai.

@chunhtai @zanderso all done, please review.

chunhtai · 2022-09-30T16:29:46Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

        if (globalTransform == null) {
          globalTransform = new float[16];
        }
+        if (transform == null) {


I imagine you still need another condition to initialize it so it won't crash in release code.

I did try to compile a custom engine in release mode, but it looks like the assertion will still crash the app. @GaryQian Do you know if the assertionError will be ignored in release mode?

You meant the exception will be thrown only on debug mode, right? Shall BuildConfig.DEBUG work?

yeah I think BuildConfig.DEBUG is probably the best way to do this

will reedit it.

chunhtai

Added a comment

GuaiYiHu · 2022-09-30T18:11:00Z

@chunhtai Pushed again, all checks have passed, please review, thanks a lot.

chunhtai

LGTM, I don't think there is a good way to test this change. This is adding an instrument to catch something should happen in the first place.

chunhtai · 2022-09-30T18:36:15Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

+        if (transform == null) {
+          if (BuildConfig.DEBUG) {
+            throw new AssertionError("transform has not been initialized");
+          } else {


this else case is redundant

if (BuildConfig.DEBUG) { throw new AssertionError("transform has not been initialized"); } transform = new float[16];

* Throw AssertionError to track the exception in case transform array is null on debug mode 08-16 06:23:59.641 23012 23012 E flutter : [ERROR:flutter/fml/platform/android/jni_util.cc(182)] java.lang.IllegalArgumentException: array == null at android.opengl.Matrix.multiplyMM(Native Method) at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:28) at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:240) at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:240) at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:240) at io.flutter.view.AccessibilityBridge$SemanticsNode.updateRecursively(Unknown Source:240) at io.flutter.view.AccessibilityBridge$SemanticsNode.access$4900(Unknown Source:0) at io.flutter.view.AccessibilityBridge.updateSemantics(Unknown Source:192) at io.flutter.view.AccessibilityBridge$1.updateSemantics(Unknown Source:21) at io.flutter.embedding.engine.FlutterJNI.updateSemantics(Unknown Source:9) at android.os.MessageQueue.nativePollOnce(Native 08-16 06:23:59.641 23012 23012 F flutter : [FATAL:flutter/shell/platform/android/platform_view_android_jni_impl.cc(1214)] Check failed: fml::jni::CheckException(env).

chunhtai · 2022-09-30T19:02:48Z

Can you add a test to test if it throw assertionError when a node has uninitialized transform, and mock the BuildConfig to test it doesn't crash?

GuaiYiHu · 2022-09-30T20:07:07Z

Can you add a test to test if it throw assertionError when a node has uninitialized transform, and mock the BuildConfig to test it doesn't crash?

I'm afraid I cannot reproduce this crash, it appears just occasionally.

chunhtai · 2022-09-30T20:18:12Z

Can you add a test to test if it throw assertionError when a node has uninitialized transform, and mock the BuildConfig to test it doesn't crash?

I'm afraid I cannot reproduce this crash, it appears just occasionally.

Can you create test similar to ones in AccessibilityBridgeText that sends a TestSemanticsUpdate with a node that contains a child id but not part of the update, for example, you can duplicate TestSemanticsNode but in the addToBuffer method comment out the last part that add child to the update

GaryQian · 2022-10-18T17:57:20Z

@GuaiYiHu Did you still plan on adding a test or finding a way to repro this bug?

chinmaygarde · 2022-10-20T20:08:49Z

From Triage: This looks like a benign change that seems to be hard to test and follows patterns consistent with the rest of the code. It's low risk, fixes crashes found in the wild and has an LGTM. Let's land this and file followup issues as needed.

…ine#35540)

flutter-dashboard bot added the platform-android label Aug 19, 2022

flutter-dashboard bot added the needs tests label Aug 19, 2022

GuaiYiHu changed the title ~~Fix array == null error~~ Fix array == null error in AccessibilityBridge Aug 19, 2022

chinmaygarde reviewed Aug 25, 2022

View reviewed changes

zanderso requested a review from stuartmorgan-g September 8, 2022 20:31

chunhtai self-requested a review September 13, 2022 18:08

GuaiYiHu force-pushed the main branch 3 times, most recently from 9e69811 to 9d3c709 Compare September 30, 2022 02:41

chunhtai reviewed Sep 30, 2022

View reviewed changes

GuaiYiHu force-pushed the main branch 2 times, most recently from 4eb7240 to b383392 Compare September 30, 2022 17:18

chunhtai approved these changes Sep 30, 2022

View reviewed changes

GuaiYiHu force-pushed the main branch from b383392 to ce7e9ec Compare September 30, 2022 18:43

chinmaygarde approved these changes Oct 20, 2022

View reviewed changes

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 20, 2022

auto-submit bot merged commit fd4b90d into flutter:main Oct 20, 2022

engine-flutter-autoroll mentioned this pull request Oct 20, 2022

Roll Flutter Engine from 3b6ecf9f1a66 to 7c8484437d88 (2 revisions) flutter/flutter#113795

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 20, 2022

fd4b90df0 Fix array == null error in AccessibilityBridge (flutter/eng…

f41e7db

…ine#35540)

engine-flutter-autoroll mentioned this pull request Oct 20, 2022

Roll Flutter Engine from 3b6ecf9f1a66 to 2b21a3125ca2 (6 revisions) flutter/flutter#113798

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 20, 2022

fd4b90df0 Fix array == null error in AccessibilityBridge (flutter/eng…

49c9e5c

…ine#35540)

Fix array == null error in AccessibilityBridge #35540

Fix array == null error in AccessibilityBridge #35540

Uh oh!

Conversation

GuaiYiHu commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Aug 19, 2022

Uh oh!

google-cla bot commented Aug 19, 2022

Uh oh!

GuaiYiHu commented Aug 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso commented Sep 8, 2022

Uh oh!

stuartmorgan-g commented Sep 13, 2022

Uh oh!

GaryQian commented Sep 20, 2022

Uh oh!

chinmaygarde commented Sep 22, 2022

Uh oh!

chunhtai commented Sep 22, 2022

Uh oh!

GuaiYiHu commented Sep 23, 2022

Uh oh!

zanderso commented Sep 29, 2022

Uh oh!

GuaiYiHu commented Sep 29, 2022

Uh oh!

GuaiYiHu commented Sep 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuaiYiHu commented Sep 30, 2022

Uh oh!

chunhtai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Sep 30, 2022

Uh oh!

GuaiYiHu commented Sep 30, 2022

Uh oh!

chunhtai commented Sep 30, 2022

Uh oh!

GaryQian commented Oct 18, 2022

Uh oh!

chinmaygarde commented Oct 20, 2022

Uh oh!

Reviewers

Assignees

GuaiYiHu commented Aug 19, 2022 •

edited

Loading

chinmaygarde Aug 25, 2022 •

edited

Loading

chunhtai Sep 13, 2022 •

edited

Loading

chunhtai left a comment •

edited

Loading

chunhtai left a comment •

edited

Loading