Closed Bug 1602318 Opened 7 months ago Closed 3 months ago

Create DocumentChannel directly in the parent process for that initiate there

Categories

(Core :: Networking, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla77
Fission Milestone Future
Tracking Status
firefox77 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Whiteboard: [necko-triaged])

Attachments

(20 files, 3 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

We want to be able to create the parent-side of DocumentChannel (DocumentLoadListener) directly in the parent process when we initiate a load from there, and only connect to a content process once we've received a response.

We don't want to initiate front-end loads in the old content process, since this leaks the new URI to a potentially cross-origin content process.

We also don't want to perform a process switch before initiating the load in that process since redirects might mean this switch was to the wrong process. This is also broken if the new load is handled as a download, since we need to retain the old content in that case.

Bug 1601779 is proposing to switch our current behaviour from the second to the first (getting correct behaviour, but adding a low-impact data leak). This bug tracks doing the work for the best fix.

We also want this behaviour for session history, where the state will live in the parent, and we want to initiate loads to a node in the tree without notifying the wrong content process.

Depends on: 1602320
Priority: -- → P2
Whiteboard: [necko-triaged]
Blocks: fission-ipc

This bug is not a Fission MVP blocker.

Fission Milestone: --- → Future
Blocks: 1613484

We need a few things for this:

We need all the parameters that the content process would normally pass up to the parent for a URL load. I started writing a list of those, and how we could get most of them - https://docs.google.com/document/d/1u5GVULrkG0NDoiSi5mDGop6yIBCbmeEuO6VGN3_5s58

We need to synthesize any nsIWebProgressListener notifications that would normally come from the docshell and forwarded to the parent.

We also likely want to ensure that we pre-load a content process so that it can be ready at the same time as the load starts returning data.

Blocks: 1608826
No longer blocks: 1613484
Assignee: nobody → matt.woodrow
Status: NEW → ASSIGNED

Same process origin changes are handled by the docshell, which detects this during AsyncOnChannelRedirect and clears the mixed content permission.
Process switches load in a fresh docshell, so we need to make sure we appropriately set or clear the mixed content permission.

Depends on D67094

This was previously true, since we only used ResumeRedirectedLoad with a brand new docshell. This bug adds code for using it with existing docshells, which can have any Document (and associated timing object) loaded in them.

Depends on D67096

When we initiate URL bar loads from the parent, loads that are handled externally won't fire a load event from the content process docshell, so we should use the progress listener instead.

Depends on D67099

Depends on: 1590762

Depends on D70618

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc9da03cb3a3
Associate a current DocumentLoadListener with CanonicalBrowsingContext. r=nika,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/8015780587cd
Initialize mixed content channel for process-switches. r=ckerschb,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/78930a97c2df
Defer nsIRemoteWindowContext load requests to avoid re-entrancy. r=nika,farre,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/8f7360d827dc
Remove incorrect assertion. r=jya
https://hg.mozilla.org/integration/autoland/rev/57d568114c5a
Change search engine tests to listen for, and block loads in the parent. r=kmag
https://hg.mozilla.org/integration/autoland/rev/b5e2aa4741b2
Make sure we wait for the right load to finish. r=kmag
https://hg.mozilla.org/integration/autoland/rev/0fc27c2e09d9
Make download tests wait for the STOP progress event instead of load. r=kmag
https://hg.mozilla.org/integration/autoland/rev/0fc27502503a
Remove nsILoadContext from DocumentLoadListener. r=nika,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/7c1eeb299b78
Fix unified build issues. r=jya,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/94142944fb54
Make sure rv is initialized correctly in all branches of OpenInitializedChannel. r=nika
https://hg.mozilla.org/integration/autoland/rev/cbc308486b17
Fix race in browser_ext_tabs_onUpdated by installing the listener before adding the new tab. r=zombie
https://hg.mozilla.org/integration/autoland/rev/6d0783cd5e01
Make xpihandler tests wait for load before starting the test. r=kmag
https://hg.mozilla.org/integration/autoland/rev/b298015ee960
Make some browser tests wait on the test uri to load instead of any uri. r=kmag
https://hg.mozilla.org/integration/autoland/rev/79b540f8619d
Allow less a smaller number of pref checks. r=mconley
https://hg.mozilla.org/integration/autoland/rev/184da6309f0c
Make RemoteWebProgress:stop also stop any current loads in the parent process. r=nika
https://hg.mozilla.org/integration/autoland/rev/e496ab6c0857
Expose some new functionality on the ADocumentChannelBridge/DocumentLoadListener boundary. r=jya,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/a7091729d8c9
Start loads directly from CanonicalBrowsingContext when possible. r=nika,jya,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/7240b27afe28
Expose LoadContext on BrowsingContext webidl. r=nika,kmag
https://hg.mozilla.org/integration/autoland/rev/4b3345b2e33b
Disable parent-initiated loads when using devtools. r=nika,ochameau
Attachment #9139990 - Attachment is obsolete: true
Attachment #9139988 - Attachment is obsolete: true
Attachment #9139987 - Attachment is obsolete: true
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3b056ec6be3
Associate a current DocumentLoadListener with CanonicalBrowsingContext. r=nika,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/7d1c0d968a09
Initialize mixed content channel for process-switches. r=ckerschb,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/a1b9429b3298
Defer nsIRemoteWindowContext load requests to avoid re-entrancy. r=nika,farre,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/983e5a9abe02
Remove incorrect assertion. r=jya
https://hg.mozilla.org/integration/autoland/rev/1756c7584491
Change search engine tests to listen for, and block loads in the parent. r=kmag
https://hg.mozilla.org/integration/autoland/rev/db2832973382
Make sure we wait for the right load to finish. r=kmag
https://hg.mozilla.org/integration/autoland/rev/8f27319f2c34
Make download tests wait for the STOP progress event instead of load. r=kmag
https://hg.mozilla.org/integration/autoland/rev/21ad350f9617
Remove nsILoadContext from DocumentLoadListener. r=nika,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/028bd63e710d
Fix unified build issues. r=jya,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/fc4efd11e643
Make sure rv is initialized correctly in all branches of OpenInitializedChannel. r=nika
https://hg.mozilla.org/integration/autoland/rev/5d0fc0102a7f
Fix race in browser_ext_tabs_onUpdated by installing the listener before adding the new tab. r=zombie
https://hg.mozilla.org/integration/autoland/rev/6c24ba022911
Make xpihandler tests wait for load before starting the test. r=kmag
https://hg.mozilla.org/integration/autoland/rev/13bec3079540
Make some browser tests wait on the test uri to load instead of any uri. r=kmag
https://hg.mozilla.org/integration/autoland/rev/f5742e84912b
Allow less a smaller number of pref checks. r=mconley
https://hg.mozilla.org/integration/autoland/rev/5de6321939f2
Expose LoadContext on BrowsingContext webidl. r=nika,kmag,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/088ea9d20617
Add load identifier and copy-ctor to nsDocShellLoadState. r=nika
https://hg.mozilla.org/integration/autoland/rev/5f341ebd8591
Simplify DocumentChannelParent construction so that everything happens in Init. r=jya,necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/44e82f4339a1
Initiate document loads in the parent process in parallel with setting up the content process side. r=nika,jya
https://hg.mozilla.org/integration/autoland/rev/0e0bdebf223b
Disable parent-initiated loads when using devtools. r=nika,ochameau
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f71e3eff7a8c
Enable parent initiated loads pref. r=jya
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab38fb8e945d
Backed out 20 changesets for causing multiple types of failures. CLOSED TREE
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b56956c6242
Associate a current DocumentLoadListener with CanonicalBrowsingContext. r=nika,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/f64350664fbf
Initialize mixed content channel for process-switches. r=ckerschb,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/faa49f460dd5
Defer nsIRemoteWindowContext load requests to avoid re-entrancy. r=nika,farre,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/ebba2085a6b5
Remove incorrect assertion. r=jya
https://hg.mozilla.org/integration/autoland/rev/e571f05c40db
Change search engine tests to listen for, and block loads in the parent. r=kmag
https://hg.mozilla.org/integration/autoland/rev/d4dd2b03884d
Make sure we wait for the right load to finish. r=kmag
https://hg.mozilla.org/integration/autoland/rev/7e2902385a6e
Make download tests wait for the STOP progress event instead of load. r=kmag
https://hg.mozilla.org/integration/autoland/rev/bd6752b9c333
Remove nsILoadContext from DocumentLoadListener. r=nika,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/54aff65a2d4f
Fix unified build issues. r=jya,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/18451f8ef984
Make sure rv is initialized correctly in all branches of OpenInitializedChannel. r=nika
https://hg.mozilla.org/integration/autoland/rev/bf51916fa039
Fix race in browser_ext_tabs_onUpdated by installing the listener before adding the new tab. r=zombie
https://hg.mozilla.org/integration/autoland/rev/34931634acb7
Make xpihandler tests wait for load before starting the test. r=kmag
https://hg.mozilla.org/integration/autoland/rev/ab3a2afede33
Make some browser tests wait on the test uri to load instead of any uri. r=kmag
https://hg.mozilla.org/integration/autoland/rev/77aaa5f1a81b
Allow less a smaller number of pref checks. r=mconley
https://hg.mozilla.org/integration/autoland/rev/cc172a5fcbc9
Expose LoadContext on BrowsingContext webidl. r=nika,kmag,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/cf6bf750f7aa
Add load identifier and copy-ctor to nsDocShellLoadState. r=nika
https://hg.mozilla.org/integration/autoland/rev/6d02fda2ffc5
Simplify DocumentChannelParent construction so that everything happens in Init. r=jya,necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/beabc71edde2
Initiate document loads in the parent process in parallel with setting up the content process side. r=nika,jya
https://hg.mozilla.org/integration/autoland/rev/eda5ef391b25
Disable parent-initiated loads when using devtools. r=nika,ochameau
Regressions: 1633230
Regressions: 1633231
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d61dbc091c36
Enable parent initiated loads pref. r=jya

The pref flip patch got backed out, so reopening until that sticks.

Status: RESOLVED → REOPENED
Flags: needinfo?(matt.woodrow)
Resolution: FIXED → ---
Depends on: 1633310
Regressions: 1633032
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f45a8fc88cb
Enable parent initiated loads pref. r=jya
Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED

== Change summary for alert #25779 (as of Wed, 29 Apr 2020 18:34:13 GMT) ==

Regressions:

44% raptor-tp6-twitch-firefox-cold fcp linux64-shippable-qr opt 68.33 -> 98.58
36% raptor-tp6-twitch-firefox-cold fcp linux64-shippable-qr opt 72.88 -> 98.75

Improvements:

7% raptor-tp6-instagram-firefox-cold fcp linux64-shippable opt 205.67 -> 192.25
6% raptor-tp6-outlook-firefox-cold fcp linux64-shippable opt 216.33 -> 202.92
6% raptor-tp6-bing-firefox-cold fcp windows10-64-shippable-qr opt 245.58 -> 231.08
6% raptor-tp6-outlook-firefox-cold fcp linux64-shippable opt 215.88 -> 203.25
5% raptor-tp6-reddit-firefox-cold fcp linux64-shippable opt 270.50 -> 255.75
5% raptor-tp6-instagram-firefox-cold fcp windows10-64-shippable-qr opt 205.00 -> 193.92
5% raptor-tp6-outlook-firefox-cold linux64-shippable opt 289.72 -> 274.66
5% raptor-tp6-bing-firefox-cold fcp linux64-shippable opt 248.42 -> 235.92
5% raptor-tp6-bing-firefox-cold linux64-shippable opt 252.92 -> 240.51
5% raptor-tp6-yahoo-mail-firefox-cold fcp linux64-shippable opt 282.67 -> 269.25
4% raptor-tp6-instagram-firefox-cold linux64-shippable opt 387.82 -> 370.73
4% raptor-tp6-instagram-firefox-cold fcp windows10-64-shippable opt 202.62 -> 194.08
4% raptor-tp6-bing-firefox-cold windows10-64-shippable-qr opt 237.41 -> 227.86
4% raptor-tp6-google-firefox-cold fcp linux64-shippable opt 308.96 -> 296.58
4% raptor-tp6-outlook-firefox-cold linux64-shippable-qr opt 287.59 -> 276.68
4% raptor-tp6-instagram-firefox-cold windows10-64-shippable-qr opt 375.85 -> 362.63
4% raptor-tp6-bing-firefox-cold linux64-shippable-qr opt 255.09 -> 246.18
3% raptor-tp6-reddit-firefox-cold linux64-shippable opt 732.97 -> 708.02
3% raptor-tp6-yahoo-mail-firefox-cold linux64-shippable opt 499.44 -> 482.60
3% raptor-tp6-google-firefox-cold linux64-shippable opt 411.53 -> 397.99
3% raptor-tp6-bing-firefox-cold fcp linux64-shippable-qr opt 261.42 -> 253.33
3% raptor-tp6-yandex-firefox-cold fcp linux64-shippable-qr opt 313.42 -> 304.58
3% raptor-tp6-yahoo-mail-firefox-cold linux64-shippable-qr opt 513.37 -> 500.41
2% raptor-tp6-amazon-firefox-mitm5-cold linux64-shippable opt 818.35 -> 798.34
2% raptor-tp6-bing-firefox-cold loadtime windows10-64-shippable-qr opt 315.29 -> 307.75
2% raptor-tp6-office-firefox-cold fcp windows10-64-shippable-qr opt 400.17 -> 391.50
2% raptor-tp6-office-firefox-cold windows10-64-shippable-qr opt 383.06 -> 374.91
2% raptor-tp6-youtube-firefox-cold linux64-shippable-qr opt 780.02 -> 763.68
2% raptor-tp6-yahoo-mail-firefox-cold fcp windows10-64-shippable-qr opt 320.25 -> 313.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=25779

Hey Matt, there are lots of improvements from your patch but we also have two regressions for raptor-tp6-twitch.
Are these expected ?
Thanks!

Flags: needinfo?(matt.woodrow)

I'm not sure I entirely believe the twitch regression. That test has been extremely volatile, and I suspect in this case we're just seeing a slight timing improvement knock it from one mode to another. Especially given that the last significant improvement seemed to come from a test-only change that seemed to affect a lot of raptor tests in unexpected ways.

Yeah, given that it seems to be the exact reverse of an unexplained improvement from a few days ago, then I don't think the regression is super meaningful.

Flags: needinfo?(matt.woodrow)

I took a local profile of the twitch regression to try to understand it: https://bit.ly/3d1H4uI (sorry, xul symbolication didn't seem to work).

Looking at that it appears that the main document load of twitch returns, but we spend ~33ms waiting for the main-thread to become free, so that we can send it to the content process (which is roughly similar to the magnitude of the regression).

The main thread is busy in NeckoParent::RecvGetExtensionStream, and then we run a layout/paint cycle for the parent process content.

Looking at other profiles, it appears that have this same work on all pages, but twitch has an http response time of ~50ms, which is really quick and leads to it being ready before the other work completes. My local run of instagram had a response time of 180ms, so the main-thread was idle by the time we needed it.

I think this explains the regression, and why it comes and goes, since it wouldn't take much for the scheduling to work out better.

The extension work appears to be handling calls from raptor's 'postToControlServer' (I suspect the one inside updateTab, followed by the "status" message in nextCycle, but hard to be sure).

Henrik, is this something you've looked at?

I suspect reducing (or delaying) the number of postToControlServer calls during the time we're measuring might lead to better and more consistent results.

Flags: needinfo?(hskupin)
Regressions: 1634339
No longer regressions: 1634339

== Change summary for alert #25832 (as of Mon, 04 May 2020 07:14:14 GMT) ==

Improvements:

6% tabpaint linux64-shippable opt e10s stylo 53.41 -> 49.95
6% tabpaint linux64-shippable opt e10s stylo 53.42 -> 50.02
4% tsvg_static linux64-shippable opt e10s stylo 45.58 -> 43.81
3% tsvg_static linux64-shippable-qr opt e10s stylo 43.59 -> 42.20

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=25832

(In reply to Matt Woodrow (:mattwoodrow) from comment #44)

Henrik, is this something you've looked at?

I suspect reducing (or delaying) the number of postToControlServer calls during the time we're measuring might lead to better and more consistent results.

Thanks Matt! This is very useful information and hopefully helps us to get the performance regression on bug 1627621 investigated/solved. See that bug for further updates.

Flags: needinfo?(hskupin)

(In reply to Matt Woodrow (:mattwoodrow) from comment #44)

I took a local profile of the twitch regression to try to understand it: https://bit.ly/3d1H4uI (sorry, xul symbolication didn't seem to work).

(This was probably due to bug 1635139.)

You need to log in before you can comment on or make changes to this bug.