Closed Bug 1550467 Opened 5 years ago Closed 5 years ago

Add a mochitest to exercise the CollectTransformsForChromeMainThread code

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(4 files)

Bug 1550467 - Ensure that listeners in chrome scripts are cleaned up. r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1550467 - Add a basic browser-mochitest for testing APZ+fission codepaths. r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1550467 - Also install SimpleTest.is into the fission window. r?botond
47 bytes, text/x-phabricator-request
Details | Review
Bug 1550467 - Add a test for event untransforms. r?botond
47 bytes, text/x-phabricator-request
Details | Review

Before cleaning up the fix for bug 1548687 I want to land a mochitest that tests the fix in bug 1530661, so that we don't regress it. Henri described what he used to test that patch at https://bugzilla.mozilla.org/show_bug.cgi?id=1530661#c30 - for the purposes of this mochitest I think it would be sufficient to replicate that test page (maybe a bit simplified) and do the last bit of what Henri said (i.e. dispatch an event to it and verify the coordinates on it make sense).

I spent a bunch of time trying to produce a setup that can run mochitests that exercise fission/APZ codepaths, and was somewhat successful. The basic outline is that there's shell browser-mochitest that opens a new top-level browser window that is fission-enabled, and then loads a "real" test in a tab in that window.

At first the "real" test I was loading was a chrome:// URL, which works, but then if that page has an OOP iframe inside it, that OOP iframe can't communicate with the chrome:// page via postMessage, and I couldn't find a way around that.

I also tried making the "real" test a non-chrome URL, i.e. http://mochi.test:8888/.... The problem with this is that if there's an OOP iframe (say with a https://example.com/... URL) inside it, then the onload event for that iframe doesn't seem to fire. I assume this is just a fission bug that needs fixing, rather than some fundamental thing. So for now I can work around it by just doing a setTimeout and assuming the iframe will be loaded in some amount of time.

I also prefer this second approach because it seems more representative of real-world scenarios (i.e. the test page will be in a content process, and the OOP iframe will be in another content process). The first approach is different because the chrome:// test page gets loaded in the main process instead of a content process. However the second approach also means there's extra postMessage machinery needed for communicating between all these different processes/windows in the test.

Here's the "shell" browser-mochitest: https://hg.mozilla.org/try/file/c0a3c61fc3941de8ad10c8f550fda54d432fbb54/gfx/layers/apz/test/mochitest/browser_test_group_fission.js
Here's the "real" test that gets loaded with a http://mochi.test:8888/ URL: https://hg.mozilla.org/try/file/c0a3c61fc3941de8ad10c8f550fda54d432fbb54/gfx/layers/apz/test/mochitest/helper_fission_basic_content.html
Here's the page that gets loaded into the OOP iframe with a https://example.com/ URL: https://hg.mozilla.org/try/file/c0a3c61fc3941de8ad10c8f550fda54d432fbb54/gfx/layers/apz/test/mochitest/helper_fission_eval.html

Nika, can you confirm that the onload event for OOP iframes not firing is just a bug that needs fixing? Also if you have suggestions on better ways to architect this, or if I'm doing something that's not really supported, please let me know.

Flags: needinfo?(nika)
No longer blocks: 1550923
Depends on: 1550923

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)

I spent a bunch of time trying to produce a setup that can run mochitests that exercise fission/APZ codepaths, and was somewhat successful. The basic outline is that there's shell browser-mochitest that opens a new top-level browser window that is fission-enabled, and then loads a "real" test in a tab in that window.

At first the "real" test I was loading was a chrome:// URL, which works, but then if that page has an OOP iframe inside it, that OOP iframe can't communicate with the chrome:// page via postMessage, and I couldn't find a way around that.

I also tried making the "real" test a non-chrome URL, i.e. http://mochi.test:8888/.... The problem with this is that if there's an OOP iframe (say with a https://example.com/... URL) inside it, then the onload event for that iframe doesn't seem to fire. I assume this is just a fission bug that needs fixing, rather than some fundamental thing. So for now I can work around it by just doing a setTimeout and assuming the iframe will be loaded in some amount of time.

Yeah, onload isn't fired out of oop iframes yet unfortunately. I've worked around it by having the iframe listen to its own onload event, and window.parent.postMessage when it's done loading.

I also prefer this second approach because it seems more representative of real-world scenarios (i.e. the test page will be in a content process, and the OOP iframe will be in another content process). The first approach is different because the chrome:// test page gets loaded in the main process instead of a content process. However the second approach also means there's extra postMessage machinery needed for communicating between all these different processes/windows in the test.

Yeah, unfortunately you're testing completely different codepaths for an oop iframe in the parent process vs. an oop iframe in a content process :-/ - so it would be much nicer if you could instead use the real oop iframe.

Here's the "shell" browser-mochitest: https://hg.mozilla.org/try/file/c0a3c61fc3941de8ad10c8f550fda54d432fbb54/gfx/layers/apz/test/mochitest/browser_test_group_fission.js
Here's the "real" test that gets loaded with a http://mochi.test:8888/ URL: https://hg.mozilla.org/try/file/c0a3c61fc3941de8ad10c8f550fda54d432fbb54/gfx/layers/apz/test/mochitest/helper_fission_basic_content.html
Here's the page that gets loaded into the OOP iframe with a https://example.com/ URL: https://hg.mozilla.org/try/file/c0a3c61fc3941de8ad10c8f550fda54d432fbb54/gfx/layers/apz/test/mochitest/helper_fission_eval.html

Nika, can you confirm that the onload event for OOP iframes not firing is just a bug that needs fixing?

Yep. It's just not implemented yet.

Also if you have suggestions on better ways to architect this, or if I'm doing something that's not really supported, please let me know.

I think the """best""" way right now is probably to fire a postMessage out when the page detects its loaded.

Flags: needinfo?(nika)

I cleaned up the test a bit and pushed it to try:

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

it's showing leaked windows which sounds plausible but I don't know what's causing the leak. Will need to investigate more.

The leaking seems to be coming from the addMessageListener calls in the chromeProcessScript. Not clear to me why. There's no corresponding removeMessageListener function in the sandbox that script runs in, so I added one but it still didn't help.

This is no longer blocked on bug 1550923.

No longer depends on: 1550923

Apparently my removeMessageListener implementation wasn't good enough. I made it better and fixed the leak (and fixed another similar leak elsewhere) and now it's passing locally. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=564994017543517e7401b7ccd9c5b928a984fee5

Apparently leaving these listeners registered can leak DOM windows
(in some circumstances that I don't fully comprehend) which causes
test failures when running on debug builds. At any rate, unregistering
listeners on cleanup seems like a good thing to do.

This introduces the framework and helpers needed to do this kind of testing,
and adds a basic sanity test that ensures some basic functionality.

Depends on D32185

I split this out so it's more obvious what pieces need to be modified
to add additional functions.

Depends on D32187

This exercises the transforms propagated in bug 1530661, which is
WebRender-specific (because the events only have the target layers id
set on them if WR is enabled).

Depends on D32188

So I finally finished updating the patches to use the window actor thing as Nika suggested. The updated patches work locally, but then I did a try push and they fail in some configurations. I can reproduce the --verify failure on my local Linux debug build, and as far as I can tell the iframe just isn't getting loaded intermittently. I set the src attribute of the iframe element and sometimes it just doesn't load the iframe; it visually remains blank and the onload event inside the iframe doesn't fire.

This didn't happen with the first set of patches (which are currently in phabricator) so presumably the changes I made, or rebasing to master, exposed some intermittent bug. I don't think bisecting my changes is feasible and even if it were, it probably wouldn't help that much in terms of finding the root cause which I suspect is somewhere in the DOM/IPC code. I'll spin that out into a separate bug.

Type: enhancement → task

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=ac831431ba9965410c08eae3d5e8eb27765e99e0 is the most recent try run, it's green, except for the ESLint warning which I fixed after doing this try push.

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57a34b090ca9
Ensure that listeners in chrome scripts are cleaned up. r=botond,jmaher
https://hg.mozilla.org/integration/autoland/rev/6deee42954a6
Add a basic browser-mochitest for testing APZ+fission codepaths. r=botond,nika
https://hg.mozilla.org/integration/autoland/rev/4da9907d86f8
Also install SimpleTest.is into the fission window. r=botond
https://hg.mozilla.org/integration/autoland/rev/7bed70ba2b07
Add a test for event untransforms. r=botond
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: