1576714 - Initiate subframe process switches from parent process

Status: RESOLVED FIXED

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

Description

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

Currently when a process switch would occur, the parent process asks the child process to trigger the process internally, and the subframe creates a BrowserBridgeChild directly.

As we always trigger the process switch from the parent process, we should start creating the new BrowserBridgeParent in the parent process, and send it down instead.

Comment 1

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

Attachment: Bug 1576714 - Part 1: Move BBC initialization into BrowserBridgeChild,

This is useful in part 3, where the initialization will need to be called from
multiple places.

Comment 2

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

Attachment: Bug 1576714 - Part 2: Remove mIPCOpen from PBrowserBridge actors,

Comment 3

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

Attachment: Bug 1576714 - Part 3: Initiate subframe process switches from the parent,

This flips the direction in which the BrowserBridge actor is generally created
such that it is generally created in the parent and sent down to a child
process.

This is done by making the decision about what kind of switch to perform in the
parent, and sending messages down to child processes async to orchestrate these
process changes.

Process launching is changed to use an async MozPromise-returning API in this
patch, though the actual process launching still occurs synchronously. A future
patch will enable performing async process launching through the
NewOrUsedBrowserProcess mechanism.

I know of at least a few timing issues which exist with the new logic,
especially around the state of the BrowsingContext during the process
transition. I decided to not try to fix all of these issues in this patch, as
many are complex and will require changing how we manage the lifecycle of
BrowsingContext substantially. I do, however, think that the new logic is more
reliable and has fewer timing issues than the previous logic.

Comment 4

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

Attachment: Bug 1576714 - Part 4: Delay canceling original http channel during process switch,

This patch changes when the original HttpChannelChild gets canceled during a
process switch to be after when the process switch is completed. This is needed
to prevent the load event firing too early in the original content process.

Comment 5

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

Attachment: Bug 1576714 - Part 5: Delay canceling original document channel during process switch,

This delays when the DocumentChannelChild is canceled during a process switch to
be after the switch has been completed, to prevent the load event firing too
early in the original content process.

Comment 6

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

Attachment: Bug 1576714 - Part 6: Remove browser_cross_process_redirect test,

This test was originally written to test HTTPResponseProcessSelection before it
was hooked up into the process switch machinery. It hooks into some parts of the
process switch process which should probably be removed in the future (such as
overriding the child listener component registration), and is broken under
fission anyway.

Comment 7

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

Attachment: Bug 1576714 - Part 7: Remove some fixed fail-if=fission annotations,

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a303fc193f60
Part 1: Move BBC initialization into BrowserBridgeChild, r=kmag
https://hg.mozilla.org/integration/autoland/rev/6996b7705f06
Part 2: Remove mIPCOpen from PBrowserBridge actors, r=kmag
https://hg.mozilla.org/integration/autoland/rev/b91221da32c7
Part 3: Initiate subframe process switches from the parent, r=kmag
https://hg.mozilla.org/integration/autoland/rev/88e3b4b7fbaf
Part 4: Delay canceling original http channel during process switch, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/b3467f1bdde7
Part 5: Delay canceling original document channel during process switch, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/083967e704d2
Part 6: Remove browser_cross_process_redirect test, r=valentin

Comment 9

[Cristian Brindusan [:cbrindusan]]

Backed out 6 changesets (Bug 1576714) for build bustages at nsFrameLoaderOwner.cpp.

Backout: https://hg.mozilla.org/integration/autoland/rev/ddba32e109519e2630eaec2f0494ecd3718dbbf7

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=083967e704d273762f70e90b3a4b62e7aa1e9b66&selectedJob=269333176

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269333176&repo=autoland&lineNumber=25724

Comment 10

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

Attachment: Bug 1576714 - Part 6: Remove now-passing fail-if,

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a5072019168
Part 1: Move BBC initialization into BrowserBridgeChild, r=kmag
https://hg.mozilla.org/integration/autoland/rev/7dce423417d8
Part 2: Remove mIPCOpen from PBrowserBridge actors, r=kmag
https://hg.mozilla.org/integration/autoland/rev/ebda40f96884
Part 3: Initiate subframe process switches from the parent, r=kmag
https://hg.mozilla.org/integration/autoland/rev/eece722082c7
Part 4: Delay canceling original http channel during process switch, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/2e156655c31e
Part 5: Delay canceling original document channel during process switch, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/faecc9f35b49
Part 6: Remove browser_cross_process_redirect test, r=valentin
https://hg.mozilla.org/integration/autoland/rev/d0c49f00eb91
Part 7: Remove now-passing fail-if, r=kmag

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/9a5072019168
https://hg.mozilla.org/mozilla-central/rev/7dce423417d8
https://hg.mozilla.org/mozilla-central/rev/ebda40f96884
https://hg.mozilla.org/mozilla-central/rev/eece722082c7
https://hg.mozilla.org/mozilla-central/rev/2e156655c31e
https://hg.mozilla.org/mozilla-central/rev/faecc9f35b49
https://hg.mozilla.org/mozilla-central/rev/d0c49f00eb91

Comment 13

[Natalia Csoregi [:nataliaCs]]

Backed out for fission permafailures on test_bug590812.html

backout: https://hg.mozilla.org/mozilla-central/rev/e966603209bba385cac292c937cd74d9426a3412

r/b range: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&fromchange=ecc7bad16aef565a06c3910218f915a1510401c3&searchStr=linux%2Cx64%2Cquantumrender%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux64-qr%2Fdebug-mochitest-fis-e10s-2%2Cm-fis%282%29&tochange=9ba22ed9e1faf20d7c6e08cd5aaf895377f300da&selectedJob=269835366

Comment 14

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

Attachment: test_bug590812.html oop process message diff

Comment 15

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

Attachment: test_bug590812.html APZC log

Comment 16

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

Attachment: test_bug590812.html layers.dump info

Comment 17

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

Attachment: test_bug590812.html screenshot

Comment 18

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

I spent a good amount of time trying to figure this one (comment 13) out, and I think I'm going to give it a break for a little bit.

The specific issue causing this failure is that the screenshot taken is slightly different, namely it is missing the blank white space which would normally be reserved for the scrollbar.

Unfortunately, this isn't just an issue with the screenshot. If the test is run with --keep-open=true, it's visible that the oop iframe has no scrollbar at all, and is unscrollable. It does participate in hit-testing correctly, however, as the text within the oop iframe is selectable. I've attached a screenshot: https://bug1576714.bmoattachments.org/attachment.cgi?id=9100235

The main change which occurred in this bug was around the timing of messages. With the patch applied, the PBrowser actor is created earlier than before, and more time passes before information such as dimensions is made available to it from the embedder process. I have tried moving around various messages to make sure they're delivered in as similar of an order as possible to the previous set-up, and it has unfortunately not been successful.

I performed a diff of the specific messages being received by the new content process between a version without my patch applied and one with my patch applied, and found relatively few differences (https://bug1576714.bmoattachments.org/attachment.cgi?id=9100228). The one major difference I noticed was that the working version (new) received an extra PAPZ::Msg_RequestContentRepaint message. This lead me to begin looking at APZ as a potential culprit.

I collected some APZC logs from during the process switch. I noticed that the behaviour between the two cases was somewhat different here, with the working case doing a bunch more stuff (https://bug1576714.bmoattachments.org/attachment.cgi?id=9100230). I'm still not sure if this actually means much.

I also dumped the layers information (https://bug1576714.bmoattachments.org/attachment.cgi?id=9100232).

I'm not entirely sure what's going on here yet, but I believe it has something to do with offset or other similar information being computed incorrectly for APZ early during the setup of the new PBrowser, before the dimension information becomes available. This makes me worried that the issue is an existing race in our oopif code, which is exacerbated by my changes.

Comment 19

[Jonathan Watt [:jwatt]]

Manually resizing the iframe with script using devtools after the page has loaded also doesn't clear things up.

Comment 20

[Jonathan Watt [:jwatt]]

I should also have said that this only appears to happen when the OOP sub-document is a generic XML document that Firefox doesn't recognize (i.e. when we use nsXMLPrettyPrinter and XMLPrettyPrint.xsl to generate the document that is displayed).

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to :Nika Layzell (ni? for response) from comment #18)

The specific issue causing this failure is that the screenshot taken is slightly different, namely it is missing the blank white space which would normally be reserved for the scrollbar.

So in the passing case the first iframe has a black white space where the scroll bars are on the other two iframes?

I'm not entirely sure what's going on here yet, but I believe it has something to do with offset or other similar information being computed incorrectly for APZ early during the setup of the new PBrowser, before the dimension information becomes available. This makes me worried that the issue is an existing race in our oopif code, which is exacerbated by my changes.

Hmm, my first suspicion would be layout code, apz doesn't have much to do with deciding whether or not we get scrollbars or allocated the space for them. I could see some scrollbar related decision changing in layout affecting the apz msgs we later send though. I think a good starting point to start looking at that code is nsHTMLScrollFrame::ReflowContents and see if the decision it makes is different and if so why (I hope I'm not leading you down the wrong path!).

Comment 22

[Jonathan Watt [:jwatt]]

Needinfo'ing Emilio per his request since he knows something about the XML pretty printer binding code.

Comment 23

[Jonathan Watt [:jwatt]]

That said, I brought this up during the Layout standup meeting today and we thought avoiding this XML pretty printer bug shouldn't block your patches, Nika (sites relying on it are probably very rare). If you agree, I think Emilio would prefer that you disable the test and land your patches to make it easier for him to look at the issue.

Comment 24

[Phabricator Automation]

Comment on attachment 9095596 [details]
Bug 1576714 - Part 6: Remove browser_cross_process_redirect test,

Revision D47313 was moved to bug 1578507. Setting attachment 9095596 [details] to obsolete.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1576714 - Don't update scrollbar visibility for non-top-level remote browsers.

This flag is only meant for window.open() stuff, so not relevant to iframes at
all.

This preserves the current fission behavior (which is quite broken) of always
showing scrollbars.

The way to control scrollbars for iframes (the scrolling attribute) is not
handled at all for Fission, I filed a bug and left a few FIXMEs.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

That was actually not too annoying to figure out :)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec7d3f3eab2a
Don't update scrollbar visibility for non-top-level remote browsers. r=nika

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21053f81f2cf
Part 1: Move BBC initialization into BrowserBridgeChild, r=kmag
https://hg.mozilla.org/integration/autoland/rev/7a8d759536e0
Part 2: Remove mIPCOpen from PBrowserBridge actors, r=kmag
https://hg.mozilla.org/integration/autoland/rev/7935fa1ac774
Part 3: Initiate subframe process switches from the parent, r=kmag
https://hg.mozilla.org/integration/autoland/rev/c5f3fab8d36d
Part 4: Delay canceling original http channel during process switch, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/6e6f09db8755
Part 5: Delay canceling original document channel during process switch, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/0594b5176839
Part 6: Remove now-passing fail-if, r=kmag

Comment 29

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/ec7d3f3eab2a
https://hg.mozilla.org/mozilla-central/rev/21053f81f2cf
https://hg.mozilla.org/mozilla-central/rev/7a8d759536e0
https://hg.mozilla.org/mozilla-central/rev/7935fa1ac774
https://hg.mozilla.org/mozilla-central/rev/c5f3fab8d36d
https://hg.mozilla.org/mozilla-central/rev/6e6f09db8755
https://hg.mozilla.org/mozilla-central/rev/0594b5176839