engine
engine copied to clipboard
Fix array == null error in AccessibilityBridge
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.
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.
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.
Will any one review this commit?
From PR triage: @stuartmorgan Do you know who would be the right reviewer for this change? Should the concern from @chinmaygarde be addressed?
@chunhtai Are you familiar with this code? It looks like you are one of the people who has been most active in this file.
@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.
I think this is a safe fix either way and the same pattern followed elsewhere is being followed here.
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?
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
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.
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.😃
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 Pushed again, all checks have passed, please review, thanks a lot.
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?
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 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
@GuaiYiHu Did you still plan on adding a test or finding a way to repro this bug?
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.