engine icon indicating copy to clipboard operation
engine copied to clipboard

Fix array == null error in AccessibilityBridge

Open GuaiYiHu opened this issue 2 years ago • 16 comments

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

https://github.com/flutter/flutter/issues/89834 https://github.com/flutter/flutter/issues/31139

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] 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.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I signed the CLA.
  • [x] All existing and new tests are passing.

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

GuaiYiHu avatar Aug 19 '22 15:08 GuaiYiHu

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.

flutter-dashboard[bot] avatar Aug 19 '22 15:08 flutter-dashboard[bot]

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.

google-cla[bot] avatar Aug 19 '22 15:08 google-cla[bot]

Will any one review this commit?

GuaiYiHu avatar Aug 22 '22 10:08 GuaiYiHu

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

zanderso avatar Sep 08 '22 20:09 zanderso

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

stuartmorgan avatar Sep 13 '22 17:09 stuartmorgan

@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.

GaryQian avatar Sep 20 '22 17:09 GaryQian

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

chinmaygarde avatar Sep 22 '22 20:09 chinmaygarde

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?

chunhtai avatar Sep 22 '22 20:09 chunhtai

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

GuaiYiHu avatar Sep 23 '22 00:09 GuaiYiHu

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.

zanderso avatar Sep 29 '22 20:09 zanderso

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 avatar Sep 29 '22 23:09 GuaiYiHu

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.

GuaiYiHu avatar Sep 30 '22 04:09 GuaiYiHu

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

GuaiYiHu avatar Sep 30 '22 18:09 GuaiYiHu

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?

chunhtai avatar Sep 30 '22 19:09 chunhtai

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.

GuaiYiHu avatar Sep 30 '22 20:09 GuaiYiHu

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

chunhtai avatar Sep 30 '22 20:09 chunhtai

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

GaryQian avatar Oct 18 '22 17:10 GaryQian

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.

chinmaygarde avatar Oct 20 '22 20:10 chinmaygarde