1542415 - Def FrameLoader rebuild preferences to on

Status: RESOLVED FIXED

(Core :: DOM: Navigation, task, P2)

Description

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

Once bug 1535390 lands, we can default the FrameLoader rebuild prefs to on, or possibly remove them completely.

Comment 1

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

:nika, do we want to keep this pref around, or should I just remove it completely?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

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

:nika, do we want to keep this pref around, or should I just remove it completely?

Let's keep the pref around until we ship this to release just in case we hit something really bad in the process. Once this lands on release and we don't have issues, we should kill the pref entirely.

I'm mostly just being defensive here, because I don't want to discover something is super broken & need to write a backout patch on release.

Comment 3

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

How defensive do we want to be? Pref on for Nightly-only, or do we want this to ride into Release early?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

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

How defensive do we want to be? Pref on for Nightly-only, or do we want this to ride into Release early?

I want to pref on everywhere, but keep the pref around so we can turn it off if it causes regressions on the way to release.

Comment 5

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

Attachment: Bug 1542415 - Pref on frameloader rebuilding by default; r!nika

Turn on frameloader rebuilding on process switch for all browsers.

Comment 6

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

Try run:

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a49a58dae129
Pref on frameloader rebuilding by default r=nika

Comment 8

[Stefan Hindli [:stefan_hindli]]

Backed out changeset a49a58dae129 (Bug 1542415) for causing bc permafailures in browser/components/urlbar/tests/browser/browser_urlbarAboutHomeLoading.js CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=240852295

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=240852295&repo=autoland&lineNumber=14390

05:42:10 INFO - TEST-INFO | started process screencapture
05:42:10 INFO - TEST-INFO | screencapture: exit 0
05:42:10 INFO - Buffered messages logged at 05:42:05
05:42:10 INFO - Entering test bound clearURLBarAfterParentProcessURL
05:42:10 INFO - Buffered messages logged at 05:42:07
05:42:10 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_urlbarAboutHomeLoading.js | URL bar should be empty -
05:42:10 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_urlbarAboutHomeLoading.js | The browser should have no recorded userTypedValue -
05:42:10 INFO - Leaving test bound clearURLBarAfterParentProcessURL
05:42:10 INFO - Entering test bound clearURLBarAfterParentProcessURLInExistingTab
05:42:10 INFO - Buffered messages finished
05:42:10 INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_urlbarAboutHomeLoading.js | uncaught exception - TypeError: gAppUpdater is undefined at onUnload@chrome://browser/content/aboutDialog-appUpdater.js:23:7
05:42:10 INFO - handleEvent@chrome://browser/content/preferences/in-content/main.js:1440:9
05:42:10 INFO - updateBrowserRemoteness@chrome://browser/content/tabbrowser.js:1728:16
05:42:10 INFO - restoreTabContent@resource:///modules/sessionstore/SessionStore.jsm:4229:20
05:42:10 INFO - restoreTab@resource:///modules/sessionstore/SessionStore.jsm:4143:14
05:42:10 INFO - _asyncNavigateAndRestore@resource:///modules/sessionstore/SessionStore.jsm:3288:10
05:42:10 INFO - async*navigateAndRestore@resource:///modules/sessionstore/SessionStore.jsm:3189:24
05:42:10 INFO - navigateAndRestore@resource:///modules/sessionstore/SessionStore.jsm:369:33
05:42:10 INFO - LoadInOtherProcess@chrome://browser/content/browser.js:1286:16
05:42:10 INFO - _loadURI@chrome://browser/content/browser.js:1253:7
05:42:10 INFO - loadTabs@chrome://browser/content/tabbrowser.js:1546:17
05:42:10 INFO - loadOneOrMoreURIs@chrome://browser/content/browser.js:2373:14

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7339545396ae
Backed out changeset a49a58dae129 for causing bc permafailures in browser/components/urlbar/tests/browser/browser_urlbarAboutHomeLoading.js CLOSED TREE

Comment 10

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

Ok I have no idea what happened here. This passes try, passes locally, I can't seem to make this stack happen again. I'm gonna try relanding and just hope I caught something at the wrong time?

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98adabf295d0
Pref on frameloader rebuilding by default; r!nika r=nika

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/98adabf295d0

Comment 13

[Andrew McCreight [:mccr8]]

I asked a sheriff to back this out for the regressions.

Comment 14

[Stefan Hindli [:stefan_hindli]]

:mccr8 backed out the bug for you.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=ed50266b84e2a489c5e97cf68b98b116d6ce4501

Comment 15

[Stefan Hindli [:stefan_hindli]]

[Tracking Requested - why for this release]:

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc66462519e1
Pref on frameloader rebuilding by default; r!nika r=nika

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/dc66462519e1

Comment 18

[Julien Cristau [:jcristau]]

Nika should we back this out of 68 for beta so the regressions can be taken care of during the 69 cycle?

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Based on :qdot's comment in bug 1551643 comment 5, this probably won't be fixed immediately, so we should probably back out the pref flip for now.

Ni? :qdot to see if they think this will be straightforward for me to fix.

Comment 20

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

You can usually assume I will never say anything about this part of the codebase is straightforward to fix. :)

Pref should most likely be backed out while this is worked on. Not really sure what gating should be for turning it back on, as this seems to be great for causing weird, hard-to-diagnose layout/frontend issues.

Comment 21

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

Causing multiple regressions, reopening since bug 1551993 can be considered a backout.

Comment 22

[Kris Maglione [:kmag]]

Attachment: Bug 1542415: Part 1 - Move BrowsingContext ownership checks into a common helper method. r=nika

Comment 23

[Kris Maglione [:kmag]]

Attachment: Bug 1542415: Part 2 - Treat in-flight messages from incorrect owner as warnings. r=nika

Comment 24

[Kris Maglione [:kmag]]

Attachment: Bug 1542415: Part 3 - Create new BrowserChild in the same tab group as its opener. r=nika

When we initially create a FrameLoader for a new tab created by a
window.open() call or a link click, it automatically winds up in the correct
tab group, because it's created in the child.

The problem comes when the window navigates to a new origin which requires a
process switch, and then navigates back to the same origin as its opener (for
instance, using history.back()). In that case, the parent rebuilds the frame
loader in a new tab group. But, since the child knows that it has an opener,
and that opener is in the same process, it (rightly) expects it to be in the
same tab group as its opener, and puts it there. It then asserts that this
matches the tab group of its actor, and explodes.

This patch fixes that, so that the parent checks whether its BrowsingContext
has an opener, and if does, tells the child to join its tab group.

Comment 25

[Kris Maglione [:kmag]]

Attachment: Bug 1542415: Part 4 - Always rebuild frameloaders on remoteness change in Fission windows. r=nika

Comment 26

[Kris Maglione [:kmag]]

Attachment: Bug 1542415: Part 5 - Re-enable frameloader rebuilding on process switch. r=nika

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe585687b994
Part 1 - Move BrowsingContext ownership checks into a common helper method. r=nika
https://hg.mozilla.org/integration/autoland/rev/45cf594a3635
Part 2 - Treat in-flight messages from incorrect owner as warnings. r=nika
https://hg.mozilla.org/integration/autoland/rev/ee8276f03363
Part 3 - Create new BrowserChild in the same tab group as its opener. r=nika
https://hg.mozilla.org/integration/autoland/rev/e200560d0fee
Part 4 - Always rebuild frameloaders on remoteness change in Fission windows. r=nika
https://hg.mozilla.org/integration/autoland/rev/b5ef2b8f7c2b
Part 5 - Re-enable frameloader rebuilding on process switch. r=nika

Comment 28

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/fe585687b994
https://hg.mozilla.org/mozilla-central/rev/45cf594a3635
https://hg.mozilla.org/mozilla-central/rev/ee8276f03363
https://hg.mozilla.org/mozilla-central/rev/e200560d0fee
https://hg.mozilla.org/mozilla-central/rev/b5ef2b8f7c2b

Comment 30

[Neha Kochar [:neha]]

Pref'ed off by default for non-fission windows in https://bugzilla.mozilla.org/show_bug.cgi?id=1580604#c3 .