-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix PlatformView multiple pointer crash caused by toMotionEvent #34182
Conversation
|
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. |
|
This PR will fix the following issues But using reconstruct event from Flutter engine may cause PlatformView receive a single-touch event instead of a multi-touch event and missing some touch point infomation. Please make sure the original behavior of multi-touch event with different view in Android. |
|
@iskakaushik Could you have time to look at this PR? |
|
From Triage: It is hard to reason about the correctness of this patch. Can you write a test or describe the issue and fix in more detail? |
|
@chinmaygarde When the gesture event comes, it is stored in the trackedEvent in addPointerForIndex. When the PlatformView wants to respond to the gesture event later, it mixes the MotionEvent synthesized from flutter and the attributes of the event saved at that time. At this time, the correctness of the ActionIndex in the synthesized Event cannot be guaranteed. For example, ActionIndex has always been 2, but pointerCount has changed from 3 to 2, so a crash will occur (in general, it will be caught but the special mechanism of chromium will throw it out) We are also thinking about other solutions, like @xanahopper mentioned above may affect the behavior of multi-point contact, if you have any ideas, you can share them here |
|
Thanks for elaborating. cc @dnfield. This probably still needs a test. |
Sorry for not replying for so long. Not sure if this also needs to be tested. |
|
Below is where the error occurs // frameworks/base/core/jni/android_view_MotionEvent.cpp
static jint android_view_MotionEvent_nativeGetPointerId(JNIEnv* env, jclass clazz,
jlong nativePtr, jint pointerIndex) {
MotionEvent* event = reinterpret_cast<MotionEvent*>(nativePtr);
size_t pointerCount = event->getPointerCount();
if (!validatePointerIndex(env, pointerIndex, pointerCount)) {
return -1;
}
return event->getPointerId(pointerIndex);
}
static bool validatePointerIndex(JNIEnv* env, jint pointerIndex, size_t pointerCount) {
if (pointerIndex < 0 || size_t(pointerIndex) >= pointerCount) {
jniThrowException(env, "java/lang/IllegalArgumentException",
"pointerIndex out of range");
return false;
}
return true;
}I think the action of the event handled by platformView should be consistent with the one sent from the framework side, will this cause other problems?This is a bug fix, but I saw that there is no similar test, what do I need to do? |
|
The test for this would be similar to |
|
/cc @stuartmorgan FYI |
|
@dnfield Thank you for your suggestion. I added the test but the test seems to fail in the compilation engine. I was successful locally, could you help me to find the problem? |
|
Try merging or rebasing on |
|
@dnfield All the tests have passed, could you please review again |
dnfield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM but I must admit I'm not sure I'm fully grasping the failure modes here. At any rate, we need to keep the SDK happy about what action we're sending it.
|
@stuartmorgan or @iskakaushik would you mind providing a second review here? |
|
This fix works fine in our business, so also LGTM. |
…inter count (#47424) related issue flutter/flutter#111268, flutter/flutter#106190 ### Motivation: - At flutter/flutter#111268, we found that Android PlatformView scrolls slowly after #34182 commit ### Modification: - Makes `PlatformViewsController` view to use `tracked event`'s action & pointer count ### Result: - Now PlatformView scrolls not slowly - Close flutter/flutter#111268, flutter/flutter#106190 - [ ] 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], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. Co-authored-by: John McCutchan <[email protected]>
Fixes flutter/flutter#105229
fixes flutter/flutter#70639
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.