1559841 - [Fission] The 'load' event should wait for OOP-iframes to load

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

[Jonathan Watt [:jwatt]]

Apart from this being a bug, it also prevents the Fission web crawler from reliably being able to take screenshots of OOP-iframes so that we can try to automate the discovery of visual Fission breakage on the Web.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 2

[Jonathan Watt [:jwatt]]

From what I can tell, the 'load' event that is dispatched for a document happens when nsDocLoader::DocLoaderIsEmpty is called. If mDocumentRequest is null, then the event is created and dispatched by that method itself. If it's not null, that method sends out a state change notification and the eLoad event is dispatched by nsDocumentViewer::LoadComplete under the stack:

  nsDocumentViewer::LoadComplete
  nsDocShell::EndPageLoad
  nsDocShell::OnStateChange
  nsDocLoader::DoFireOnStateChange
  nsDocLoader::doStopDocumentLoad    (sends STATE_STOP | STATE_IS_DOCUMENT)
  nsDocLoader::DocLoaderIsEmpty

It's not obvious to me how to change things for OOP-iframes. For example, should the nsDocShell::EndPageLoad call be delayed, or just the event dispatch that nsDocumentViewer::LoadComplete does?

Smaug, do you have any advice on how this should all work when we have OOP-iframes?

Comment 3

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

Could iframe tell to its parent when it isn't loading anymore, and then IsBusy method in nsDocLoader would use that information. And then rest of this would just work.
DocLoader couldn't use the current mChildList to check loading state, but it would need to look at relevant BrowsingContexts.

Comment 4

[Jonathan Watt [:jwatt]]

Thanks, Olli.

I have been look into this so assigning to myself. That said, I'm about to go on PTO for a couple of weeks so in the unlikely event that someone from DOM has time to fix this, please feel free to take it!

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: Bug 1559841. Make the 'load' event wait for OOP-iframes to load. r=kmag

Comment 7

[Intermittent Failures Robot]

10 failures in 4839 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 10

Platform breakdown:

• linux64: 2
• windows7-32: 4
• windows10-64: 4

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1559841&startday=2019-08-12&endday=2019-08-18&tree=all

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hey Jonathan, over on bug 1559592 I'm adding some new Wdspec tests for taking screenshots of OOP iframes. Those currently fail with fission enabled, and it would be great to verify your fix. Given that the revision has been reviewed what's blocking you from getting it landed?

Comment 9

[Andrew McCreight [:mccr8]]

Is there a test for this? I'm asking because I have applied the patch here, then run this test dom/bindings/test/test_crossOriginWindowSymbolAccess.html where the iframe is instead http://example.org/tests/js/xpconnect/tests/mochitest/file_empty.html, and then run it with Fission enabled, and the load event handler is run before the iframe has loaded, which doesn't seem expected to me, but maybe I'm doing something wrong.

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #8)

what's blocking you from getting it landed?

I discovered a few intermittent failures when I repeated some of my Try runs. Most of those are solved now but there is still one instance of a failure due to the embedding document not having a docShell when the BrowserBridgeChild is created for an iframe it contains. Anyone who's curious can see that here:

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=263175092&searchStr=m-fis&revision=062914baebd8d5fc63bc08bb62d286b2b7f384f5

I'll wallpaper that and land for now and file a bug to follow-up on that issue since I think for most folks the patch should meet their needs.

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Andrew McCreight [:mccr8] from comment #9)

Is there a test for this?

The primary test that I've been using is this one:

https://sub1.jwatt.org/oop-iframe.html

That has an 'alert()' that will notify you when the document's 'load' event fires. It embeds a cross-origin document that never finishes loading, so the alert isn't put up until you cancel the load by hitting the Escape key, or Firefox times it out.

I'm asking because I have applied the patch here, then run this test dom/bindings/test/test_crossOriginWindowSymbolAccess.html where the iframe is instead http://example.org/tests/js/xpconnect/tests/mochitest/file_empty.html, and then run it with Fission enabled, and the load event handler is run before the iframe has loaded, which doesn't seem expected to me, but maybe I'm doing something wrong.

I'm not sure offhand (it's late) exactly how that test should behave differently with/without the patch applied. Can you describe what indicates that the load event isn't waiting for the subdoc load?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20abb86e0049
Make the 'load' event wait for OOP-iframes to load. r=farre

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/20abb86e0049

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please note that also the cross-origin iframe tests for bug 1559592 still fail, with this patch in Nightly now. Not sure if this is related to your filed bug 1576296, or something still remaining to do. Could you please check?

Comment 15

[Andrew McCreight [:mccr8]]

(In reply to Jonathan Watt [:jwatt] from comment #11)

I'm asking because I have applied the patch here, then run this test dom/bindings/test/test_crossOriginWindowSymbolAccess.html where the iframe is instead http://example.org/tests/js/xpconnect/tests/mochitest/file_empty.html, and then run it with Fission enabled, and the load event handler is run before the iframe has loaded, which doesn't seem expected to me, but maybe I'm doing something wrong.

I'm not sure offhand (it's late) exactly how that test should behave differently with/without the patch applied. Can you describe what indicates that the load event isn't waiting for the subdoc load?

Maybe a better example would be these tests: https://searchfox.org/mozilla-central/search?q=1559841&path=

They have a loop that waits for a bit to give the OOP iframe a chance to load. The hope was that with this patch those loops can be deleted. I haven't actually tried out if this patch fixes that or not. I'll do that once I get a build.

Comment 16

[Andrew McCreight [:mccr8]]

I looked through the tests that refer to this bug. browser/base/content/test/permissions/browser_temporary_permissions.js fails with the loop labelled with the bug 1559841 comment removed, dom/base/test/test_bug590812.html and layout/svg/tests/test_filter_crossorigin.html pass. dom/base/test/test_x-frame-options.html was failing anyways.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given the amount of regressions filed for this bug, shouldn't we consider a backout?

Comment 18

[Kris Maglione [:kmag]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #17)

Given the amount of regressions filed for this bug, shouldn't we consider a backout?

Given that it also doesn't seem to fix a lot of the cases we particularly care about, I think that may be a good idea.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, I checked with the sheriffs, and they will get it backed out.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for several fission regressions, e.g. Bug 1576413:

https://hg.mozilla.org/mozilla-central/rev/4c09da80722fcf62db02f19dc67f3f1a6b88f84d

Comment 21

[Jonathan Watt [:jwatt]]

The latest revision of this is getting pretty close now:

https://treeherder.mozilla.org/#/jobs?repo=try&collapsedPushes=550789%2C549957%2C551077&searchStr=m-fis&revision=c1f4ce0675d8ef9f483a7e4226f46c96f96fe1d6

Probably good enough to be able to retest if you're keen.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan Watt [:jwatt] from comment #21)

The latest revision of this is getting pretty close now:

https://treeherder.mozilla.org/#/jobs?repo=try&collapsedPushes=550789%2C549957%2C551077&searchStr=m-fis&revision=c1f4ce0675d8ef9f483a7e4226f46c96f96fe1d6

Thanks Jonathan! In case of Marionette you would only have to run the Wd jobs with fission enabled. I triggered these jobs on the given try build.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The Wd2 job which includes the screenshot tests is still broken for OOP iframes. The screenshot is still blank (white):

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266245573&repo=try&lineNumber=101270

Comment 24

[Andrew McCreight [:mccr8]]

Thanks. I'm at a team meetup and on PTO next week, but I'll check it out after that.

Comment 25

[Kris Maglione [:kmag]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #23)

The Wd2 job which includes the screenshot tests is still broken for OOP iframes. The screenshot is still blank (white):

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266245573&repo=try&lineNumber=101270

Has the test been updated to use the new snapshot API? Using a canvas to draw a cross-process iframe from a child process is never going to work, so even if it's loaded, it will still appear blank in that case.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Kris Maglione [:kmag] from comment #25)

Has the test been updated to use the new snapshot API? Using a canvas to draw a cross-process iframe from a child process is never going to work, so even if it's loaded, it will still appear blank in that case.

Yes, that was done about 2 weeks ago via bug 1559592. The tests can be made work when you add a sleep right after navigation, as such I'm 99% sure it's related to this bug. Note that those tests make use of data URLs, in case that this makes a difference.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78c3f293893a
Make the 'load' event wait for OOP-iframes to load. r=kmag

Comment 28

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/78c3f293893a

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Good news is that this fixes most of the Wdspec screenshot failures, and while the screenshots look identical they are still reported as different. I filed bug 1583462 to get this investigated.