1914368 - Enable mochitest helper_fission_tap for Android

Status: NEW

(Core :: Panning and Zooming, task, P2)

Description

[Alex Jakobi [:ajakobi]]

Follow up for Bug 1841896. We currently skip helper_fission_tap.html on Android because we're seeing a test failure on try for Android:

WARNING -  TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_fission.html | x-coord -43 landed near expected value 141.42135623730948
INFO -      SimpleTest.ok@SimpleTest/SimpleTest.js:426:16
INFO -      test@gfx/layers/apz/test/mochitest/helper_fission_tap.html:72:9
INFO -      async*@gfx/layers/apz/test/mochitest/helper_fission_tap.html:79:8

We don't see this when running on a local Android build with the emulator.

Expected result:

• Test should pass locally and on CI for Android
• Test is not skipped for Android (see test_group_fission.html)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

So adding a meta viewport tag didn't help there? I wonder why, I don't see the tag in the file though.

Comment 2

[Alex Jakobi [:ajakobi]]

Attachment: Bug 1914368 - Transform frame point back relative to its root r=botond

Comment 3

[Botond Ballo [:botond]]

Summarizing some of the recent developments here:

• Hiro had a theory that the failure could be related to the fact that the test page (helper_fission_tap.html) does not fit onto a mobile screen, and so we are trying to synthesize the tap at an off-screen location.
• To test this theory, we tried scaling down the size of everything on the page by a factor of 2, and pushed the modified test to Try here.
• The test is still failing on Android (in fission-disabled runs only), with x-coord -22 landed near expected value 70.71067811865474. This is the same failure as before, with the coordinates scaled down by a factor of 2, suggesting that the issue was not the off-screen tap (or at least, it was not the only issue).

Comment 4

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #3)

• To test this theory, we tried scaling down the size of everything on the page by a factor of 2, and pushed the modified test to Try here.

Re-pushed without --artifact to get logging from C++ code: https://treeherder.mozilla.org/jobs?repo=try&revision=7f7b63c22a349b9a0b2cc4e407292c5a0c704333

Comment 5

[Botond Ballo [:botond]]

Attachment: Relevant part of log

Attached is an excerpt of the Android log for the failing run of the test from the debug job in the push linked in comment 4. The excerpt contains the section from "synthesizeNativeTap START" until "received clicked message with coords: {-22, 70}".

Comment 6

[Botond Ballo [:botond]]

What I'm seeing in the log from comment 5 is:

• APZCCallbackHelper::DispatchSynthesizedMouseEvent is being called with the expected coordinates (aRefPoint=(200,200))
• However, we subsequently see Event::GetClientCoords being called on an event with mRefPoint=(71,71), and this is what gives the incorrect result of (-22, 70).

This suggests that something during the event dispatch performed by DispatchSynthesizedMouseEvent itself (i.e. in the implementation of that function -- I believe it dispatches the event synchronously) is changing the mRefPoint from the initial value of (200,200), to the incorrect (already transformed) value of (71,71). The next investigative step would be to find out what.

One approach could be, assuming that GetClientCoords itself is being called synchronously during DispatchSynthesizedMouseEvent (which I believe is the case), is to figure out the chain of calls by which this happens, and log mRefPoint at additional points during that chain. To figure out the chain of calls, we can run the test under a debugger, break in the relevant call to GetClientCoords, and look at the backtrace. This can probably be done on desktop (this being mostly DOM code, the codepaths taken are likely the same between desktop and android).

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)

One approach could be, assuming that GetClientCoords itself is being called synchronously during DispatchSynthesizedMouseEvent (which I believe is the case), is to figure out the chain of calls by which this happens, and log mRefPoint at additional points during that chain. To figure out the chain of calls, we can run the test under a debugger, break in the relevant call to GetClientCoords, and look at the backtrace. This can probably be done on desktop (this being mostly DOM code, the codepaths taken are likely the same between desktop and android).

Here is an alternative approach for getting the relevant backtrace from a local Android build:

• Apply the patch I posted in bug 1855070
• Locally add an NS_ASSERTION(false) in GetClientCoords
• Run the test locally and find the backtrace for the call we're interested in (the one with incorrect coordinates as input)

Note that the backtrace will likely need to be symbolicated by piping it through ~/.mozbuild/fix-stacks/fix-stacks -l <dir>, where <dir> is a directory containing a non-stripped version of the libxul.so file matching the build that produced the backtrace (for me that's <objdir>/dist/bin).

Comment 8

[Botond Ballo [:botond]]

Attachment: Call chain from DispatchSynthesizedMouseEvent to GetClientCoords

For reference, here is a stack trace of DispatchSynthesizedMouseEvent (called with a correct mRefPoint) calling into GetClientCoords (by which point mRefPoint is incorrect).

Comment 9

[Alex Jakobi [:ajakobi]]

Attachment: Call chain from DispatchSynthesizedMouseEvent to GetClientCoords with event coordinates and event address

Adding call chain from APZCCallbackHelper::DispatchSynthesizedMouseEvent to Event::GetClientCoords including the refpoint's coordinates and the address of the event object.

From this file it looks like the wrong coordinates are being written into the event's mRefPoint during the execution of PresShell::EventHandler::HandleEventUsingCoordinates.

Comment 10

[Alex Jakobi [:ajakobi]]

Found the place where the event's mRefPoint is being written into: PositionedEventTargeting.cpp#626 / FindFrameTargetedByInputEvent()

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Nice find! I confirmed the test passes with ui.mouse.radius.enabled=false.

So now we've identified that the issue is in our event retargeting system regardless whether fission. I'd suggest finishing bug 1841896 first.

Comment 12

[Botond Ballo [:botond]]

Moreover, the failure reproduces on desktop with --disable-fission --setpref ui.mouse.radius.enabled=true --setpref ui.mouse.radius.reposition=true, so we should be able to do further debugging on desktop.

Comment 13

[Botond Ballo [:botond]]

I uploaded an rr recording of a failure of the test on desktop to Pernosco here.

Comment 14

[Botond Ballo [:botond]]

Note that there is no actual retargeting of the event going on in this test case, i.e. the event retargeting code runs but the final target it comes up with is the same as the initial target.

In such a scenario, recomputing the event's position to reflect the new target is unnecessary work that could be skipped; I field bug 1928758 about this.

Note that while bug 1928758 would make this test pass, it would just be covering up the underlying bug (which is in the recomputation codepath), and the underlying bug would continue to affect scenarios where the event is in fact retargeted to a different frame.

To make it easier to reproduce and debug this bug, I will hold off on fixing bug 1928758 until we fix this first.

Comment 15

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #13)

I uploaded an rr recording of a failure of the test on desktop to Pernosco here.

Summary of our findings while debugging the Pernosco recording: this code is attempting to do the reverse of GetEventCoordinatesRelativeTo(), but it's not handling the case where GetEventCoordinatesRelativeTo() would take the transformFound branch.