1563622 - WebRender is not transmitting JS events from chrome to content

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[i.flooder06]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

1. With WebRender enabled and devtools.chrome.enabled = true, open Browser Console.

2. Run
   document.addEventListener('wheel', (e) => e.preventDefault());

3. Rotate the mousewheel inside a scrollable webpage (and not chrome://).

Actual results:

Page scrolls. Because JS events aren't being transmitted to content when WebRender is enabled.

Expected results:

Page shouldn't scroll as it always did without WebRender.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Did you modify any other prefs via about:config?

Comment 2

[Darkspirit]

Confirmed STR on Debian Testing, KDE, X11, Macbook Pro A1502.
Scrolling is only prevented if WebRender is force-disabled.

Is this considered as a bug or rather desired behavior?

mozregression --good 2018-01-15 --bad 2018-03-15 --pref browser.tabs.drawInTitlebar:true gfx.webrender.all:true devtools.chrome.enabled:true -a https://mozilla.org

6:43.89 INFO: Last good revision: 117e0c0d1ebe2cf5bdffc3474744add2416fc511 (2018-01-29)
6:43.89 INFO: First bad revision: 217fc14405e78d47fb60888b9f3d0527d0c2709a (2018-01-30)
6:43.89 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=117e0c0d1ebe2cf5bdffc3474744add2416fc511&tochange=217fc14405e78d47fb60888b9f3d0527d0c2709a

Other builds are gone. These sound related:

c8da37aaabfcaead6432da2cb151b413204f7f6c Nicolas Silva — Bug 1404477 - Merge ResourceUpdateQueue and TransactionBuilder. r=kats
593402ddfee584d2b00116a39c1d77aa0546517d Nicolas Silva — Bug 1404477 - WebRender document API bindings. r=kats

Or was it caused by gfx.webrender.hit-test?

5fc179e245d51289f1f7bcaba0c29189d3336fbf Kartikaya Gupta — Bug 1421380 - Enable gfx.webrender.hit-test by default. r=jrmuizel

Comment 3

[Darkspirit]

Is there a practical need for this?

Comment 4

[i.flooder06]

For native Firefox I don't know.

One of the settings of my Mouse Gestures is to switch between open tabs by right click + mousewheel. Since 68, when WebRender was enabled by default in my AMD machine, I noticed that pages are unexpectedly scrolling a bit. Then I realized that event.preventDefault() is not being transmitted from chrome process to content.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I can look into this. I'm not sure how the regressing changeset broke it, but I know the codepath whereby it's supposed to work so I can just debug it.

Comment 6

[Darkspirit]

Problem is not reproducible when setting gfx.webrender.hit-test on first bad build back to false:
mozregression --launch 2018-01-30 --pref browser.tabs.drawInTitlebar:true gfx.webrender.all:true devtools.chrome.enabled:true gfx.webrender.hit-test:false -a https://mozilla.org

5fc179e245d51289f1f7bcaba0c29189d3336fbf Kartikaya Gupta — Bug 1421380 - Enable gfx.webrender.hit-test by default. r=jrmuizel

Comment 7

[Kartikaya Gupta (email:[e-mail])]

That makes a lot more sense, thanks!

Comment 8

[Kartikaya Gupta (email:[e-mail])]

The fix for the bug as written is relatively simple, but I uncovered other issues while investigating. The problem here is that we're not respecting the ForceDispatchToContent override flag in the WR hit test. Doing that is simple. But we're also not respecting the ForceEmptyHitRegion flag, and fixing that is more involved. Also with the STR it seems that we return false from here when called by RecvSetTargetAPZC. That happens both with and without my fix. Not really sure why; it seems that the APZCTMP is expecting a 0x10000001 layers id but is getting something like 0x1000000b. I didn't verify but usually this means the APZCTMP is for the UI process, and is getting a content process guid as the hit result from the main-thread hit test.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1563622 - Check the ftdc flag when hit-testing with WR. r?botond

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Actually after spending some more time on this and trying to write a test, I'm not sure what the correct behaviour here is. smaug, can you clarify? The scenario is that we have e10s enabled, and a wheel event listener on the parent process' document (via browser console, see comment 0), and it does a preventDefault() on all wheel events. If the user mouses over the content area and does a wheel scroll, should the listener in the parent process cancel scrolling from that event?

The notes that I have from the B2G days seem to imply that the event shouldn't trigger listeners in multiple processes (it would propagate up to the TabChildGlobal of the target process and stop). But I also some notes that say the event gets dispatched in the parent process first, before going to the target process, so maybe it would trigger that parent process listener anyway? And in the general fission case it would only trigger listeners in the parent process + innermost content process?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Event surely can trigger listeners in multiple processes. Parent process in general handles events first and then they are dispatched to the child process. And yes, I would expect in Fission case parent process + the target child process.

https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/dom/events/EventStateManager.cpp#3023,3034
PostHandleEvent happens after normal DOM event dispatch in parent process.
And looks like the recent layer id usage there https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/dom/events/EventStateManager.cpp#1242-1247 forward event to the right child process.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Ok, thanks, that makes sense. I think I was just doing the wrong thing in my test, putting a listener in the top-level content but that one isn't supposed to get triggered for events dispatched to an OOPIF.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Another hiccup in my attempt to write a test is that when the parent process calls preventDefault on the event from chrome, it prevents the event from getting forwarded to the content process at all, because of this code.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I managed to finish my patches and test for this bug, but didn't get around to the ForceEmptyHitRegion handling. I filed bug 1566599 for that. Try push with patches for this bug:

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=24b875ba10d69d541a1b583d72aa1533f5b77157

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Will throw up my patches anyway while waiting for the try push. If it's not green then my investigation will be on a best-effort basis.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1563622 - Use the right layers id for events prevented by chrome. r?botond

This is latent bug in the code. The layers id used in the parent process'
call to SetTargetAPZC was always the one that the APZ hit-test produced.
But in the case where the parent process had a chrome event listener that
does a preventDefault on the event, that is the wrong layers id to use,
because we want to use the parent process' layers id instead of the content
process' layers id.

The reason the test in this bug hits this is because with WebRender enabled
the code in APZCTreeManagerParent that receives the SetTargetAPZC message
checks the layers id to see if it matches expectations (if it doesn't, it
assumes a malicious content process). In this scenario the layers id doesn't
match and causes an assertion failure. With this fix the layers id matches
expectations.

I don't believe this has any functional effect beyond the malicious content
process check.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1563622 - Allow adding additional properties per fission subtest. r?botond

Depends on D37314

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1563622 - Add a fission subtest to exercise the force-dispatch-to-content flag. r?botond

Depends on D38239

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1563622 - Miscellaneous useful logging. r?botond

Depends on D38240

Comment 20

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01bc9598174ebc4ba967fa531caaa1192674451e

That's rebased on top of bug 1565670 which should fix the TV failures on the last try push.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=7b90d18b40020510ad1fc2d4ff5cafc83bcb35e9

With review comments addressed

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b41a0c8b4a69
Use the right layers id for events prevented by chrome. r=botond
https://hg.mozilla.org/integration/autoland/rev/c0902b305f0e
Check the ftdc flag when hit-testing with WR. r=botond
https://hg.mozilla.org/integration/autoland/rev/cf403ec6d653
Allow adding additional properties per fission subtest. r=botond
https://hg.mozilla.org/integration/autoland/rev/45854fd7659c
Add a fission subtest to exercise the force-dispatch-to-content flag. r=botond
https://hg.mozilla.org/integration/autoland/rev/7ed96abef6d4
Miscellaneous useful logging. r=botond

Comment 23

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/b41a0c8b4a69
https://hg.mozilla.org/mozilla-central/rev/c0902b305f0e
https://hg.mozilla.org/mozilla-central/rev/cf403ec6d653
https://hg.mozilla.org/mozilla-central/rev/45854fd7659c
https://hg.mozilla.org/mozilla-central/rev/7ed96abef6d4

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Not worth uplifting, I think.