1535390 - Fix remaining bugs in FrameLoader process rebuilding

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Bug 1522713 landed pref'd off due to continuing test failures. They need to be fixed so we can remove the pref and have this on all the time.

List taken from https://bugzilla.mozilla.org/show_bug.cgi?id=1522713#c9:

• Python tests (Marionette, Firefox Functional Tests)
• browser/components/preferences/in-content/tests/[a bunch of tests] (Not sure why but updating the prefs dialog in tests is broken in quite a few ways)
• browser/base/content/test/general/browser_bug495058.js (Probably event or remote type based)
• browser/base/content/test/general/browser_contentSearchUI.js (already has an intermittent filed but I think this patch makes it permaorange?)
• Some devtools breakage with responsive mode

Try with pref turned on:

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=232736513&revision=fc9eaf4eb8ceeebb953a5374c2626fc2caa88453

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Latest try after getting back from PTO, looks pretty much the same:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57d15f8093312fd47c8e4f0158b51548d9154127

Almost all tests outside of the devtools tests seem to be problems around input events and/or page display. We're failing on a lot of mouse click or key hit events, either by not simulating them at the right time or not having the correct element up to run the event on. From watching some of the failing tests run, it seems like there's some point where pages aren't repainting correctly or something when they come up, which would explain the issue of elements not being found to simulate input events on. Those issues look the same between the marionette and b-c failures, so hoping there's one fix for both.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I'm pretty stumped on this. I think I've blown up something in layout, but I've tried forcing repainting/reflow of the parent frame element after frameloader swap and it doesn't seem to be doing much.

STR:

1. Load current nightly
2. Set the 'fission.rebuild_frameloaders_on_remoteness_change' to 'true' (will need to be added, isn't a default pref)
3. Open new tab
4. Type "https://example.org" in the URL bar
5. Hit enter

Expected:

Example page comes up

Actual:

Page loads, but isn't visible. Going to any other page (google.com, etc) is similarly not visible. Once browser has what I'm guessing is a reflow trigger, like a window resize, things show up and seem to work fine from then on.

I could use some help from the frontend/layout side here, as I'm not familiar with this at all, so ni?'ing rhunt and jwatt.

Comment 3

[Ryan Hunt [:rhunt]]

(In reply to Kyle Machulis [:qdot] [:kmachulis] from comment #2)

I'm pretty stumped on this. I think I've blown up something in layout, but
I've tried forcing repainting/reflow of the parent frame element after
frameloader swap and it doesn't seem to be doing much.

STR:

1. Load current nightly
2. Set the 'fission.rebuild_frameloaders_on_remoteness_change' to 'true'
   (will need to be added, isn't a default pref)
3. Open new tab
4. Type "https://example.org" in the URL bar
5. Hit enter

Expected:

Example page comes up

Actual:

Page loads, but isn't visible. Going to any other page (google.com, etc) is
similarly not visible. Once browser has what I'm guessing is a reflow
trigger, like a window resize, things show up and seem to work fine from
then on.

I could use some help from the frontend/layout side here, as I'm not
familiar with this at all, so ni?'ing rhunt and jwatt.

Looking into this now.

It looks like you're correctly updating the frame loader cached in nsSubdocumentFrame [1], but we may need to also call Show() [2] and trigger an invalidation.

I'm spinning up a build to test this.

[1] https://searchfox.org/mozilla-central/rev/532e4b94b9e807d157ba8e55034aef05c1196dc9/dom/base/nsFrameLoaderOwner.cpp#43
[2] https://searchfox.org/mozilla-central/rev/532e4b94b9e807d157ba8e55034aef05c1196dc9/layout/generic/nsSubDocumentFrame.cpp#176

Comment 4

[Ryan Hunt [:rhunt]]

I have a patch.

Comment 5

[Ryan Hunt [:rhunt]]

Hmm, airport internet is preventing me from doing try runs or uploading through phabricator. Will try again in a couple hours.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

If you'd like you can also just email me the patch.

Comment 7

[Ryan Hunt [:rhunt]]

Here's the try run.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c87f91d6381aa5d96d3c2560a2f28f821c22c95

Comment 8

[Ryan Hunt [:rhunt]]

Attachment: Bug 1535390 - Ensure remote browser has dimensions set after recreating frame loader. r=jwatt

nsFrameLoaderOwner::UpdateRemoteness will recreate the nsFrameLoader for a
piece of content. As part of this, it will unset the cached nsFrameLoader for
the content's nsSubdocumentFrame. However we need to run ShowViewer() for the
new nsFrameLoader as the frame has already been initialized. In addition,
dimensions and position on the new nsFrameLoader need to be set. Usually this
is done after a reflow, but there's no guarantee a reflow will happen after
a UpdateRemoteness operation.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Try run with pref on (actually, removed completely): https://treeherder.mozilla.org/#/jobs?repo=try&revision=7111e6f1e3c80479fb178bfbcf7a3ca778f65049

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21358d3572a7
Ensure remote browser has dimensions set after recreating frame loader. r=jwatt

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/21358d3572a7

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/21358d3572a7