1783804 - Touch event isn't received on OOP child frame on GeckoView with fission

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Makoto Kato [:m_kato]]

Step

1. Enable fission on Fenix via fission.autostart=true
2. Browse https://wontfix.net/bugs/iframe3.html
3. Touch yellow area

Result

No touch event is received.

Expected Result

touch event is received even if OOP. (No OOP version is https://wontfix.dev/bugs/iframe3.html).

Comment 1

[Makoto Kato [:m_kato]]

Of course, this sample works on Firefox desktop with Surface Pro.

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to Makoto Kato [:m_kato] from comment #1)

Of course, this sample works on Firefox desktop with Surface Pro.

Meaning the bug can be reproduced on desktop as well? Or the bug does not exist on desktop?

Comment 3

[Makoto Kato [:m_kato]]

Meaning the bug can be reproduced on desktop as well? Or the bug does not exist on desktop?

This issue is GeckoView only. Firefox Windows doesn't have this issue. (I don't test this on Firefox/Linux).

Comment 4

[Hiroyuki Ikezoe (:hiro)]

From the perspective of APZ hit-testing, it's properly working, I dumped the layer id in question, it looks correct, but it somehow delivers to the top level document for some reasons, I guess it's handled somewhere in EventStateManager? CCing Edgar, I believe he knows very well around the code.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Moving into GeckoView:Core for now since I can no longer see Widget:Android component. Anyway a problem is here in capturing aInput value.

The aInput is MultiTouchInput, in the copy constructor of the class we don't copy over mLayersId so that we loose the information there. I am not sure not copying over the value is intentional or not, below change should fix this issue. There are other places we need to properly copy over the layers id in widget/android/nsWindows.cpp for other types of events.

diff --git a/widget/android/nsWindow.cpp b/widget/android/nsWindow.cpp
--- a/widget/android/nsWindow.cpp
+++ b/widget/android/nsWindow.cpp
@@ -845,8 +845,9 @@ class NPZCSupport final
     }
 
     // Dispatch APZ input event on Gecko thread.
-    PostInputEvent([aInput, result](nsWindow* window) {
+    PostInputEvent([aInput, result, layersId = aInput.mLayersId](nsWindow* window) {
       WidgetTouchEvent touchEvent = aInput.ToWidgetEvent(window);
+      touchEvent.mLayersId = layersId;
       window->ProcessUntransformedAPZEvent(&touchEvent, result);
       window->DispatchHitTest(touchEvent);
     });

Comment 6

[Makoto Kato [:m_kato]]

Oh, thanks. We should use std::move instead of copy constructor.

Comment 7

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98f74c2f6efa12e19b133d4ae60e9db3900b3f67

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Bug 1783804 - Use std::move for InputData since copy constructor of InputData doesn't copy all member variable. r=#geckoview-reviewers

When turning on fission, OOP child frame doesn't receive touch events such as
touchstart.

When dispatching touch event from GeckoView side, some informations such as
mLayersId are missing since copy constructor of MultiTouchInput doesn't
copy all information.

Anyways, since the structure of Input data has a lot of members, we should use
std::move instead of copy constructor of InputData. It can avoid
unnecessary copy.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

The aInput is MultiTouchInput, in the copy constructor of the class we don't copy over mLayersId so that we loose the information there. I am not sure not copying over the value is intentional or not, below change should fix this issue. There are other places we need to properly copy over the layers id in widget/android/nsWindows.cpp for other types of events.

If that's not intentional then that seems like a footgun that we should fix. Maybe Botond knows?

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Yeah, totally agree. The mLayersId was introduced in bug 1524239, it looks to me it's unintentional. CCing Masayuki and Henri.

Comment 11

[Makoto Kato [:m_kato]]

I file new issue as https://bugzilla.mozilla.org/show_bug.cgi?id=1784849 for copy constructor issue.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14e1b8cd0f6a
Use std::move for InputData since copy constructor of InputData doesn't copy all member variable. r=geckoview-reviewers,calu

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)

Yeah, totally agree. The mLayersId was introduced in bug 1524239, it looks to me it's unintentional. CCing Masayuki and Henri.

This is unintentional, yes. Sorry about the bug.

Comment 14

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/14e1b8cd0f6a

Comment 15

[Chris Peterson [:cpeterson]]

No need to uplift this fix to Beta because Android Fission is disabled by default.