1718082 - Clean up `"Browser:WindowCreated"` actor messaging and responses to it

Status: RESOLVED FIXED

(Firefox :: Security, task)

Description

[:Gijs (he/him)]

Nika pointed out in https://phabricator.services.mozilla.com/D118618#inline-657168 that this is kind of weird - the parent should know the user context id, we don't need to wait for a message from the child to deal with this.

Additionally, it appears that network code has some concept of a singular "topmost usercontextid" which the BrowserWindowTracker unconditionally updates whenever a tab selection changes via net:current-top-browsing-context-id. This is bogus in the presence of multiple browser windows - background windows can have their selected tab change without the foreground window changing (e.g. by web-JS-opened tabs closing), and background tabs can create new DOM windows through navigation, which trips the window created callbacks.

We should remove the convoluted event+message-passing based tracking, and ensure that the network singleton is doing the right thing. It's not actually clear to me what that is - from a casual reading of bug 1698661 I can't tell why network needs the top browsing context id (I think it's to do with prioritizing requests in the visible tab?), and what the right behaviour is supposed to be if multiple windows are on-screen and visible. Nihanth or Dragana, can you clarify?

Comment 1

[Dragana Damjanovic [:dragana]]

The top browse context id is used to prioritize network request for visible tabs. I think the original implementation wanted to prioritize only he focused window. We may change behavior to all visible tabs (that is probably not easy)

Kershaw, you worked on this project, correct me if I am wrong.

Comment 2

[Kershaw Chang [:kershaw]]

We should remove the convoluted event+message-passing based tracking, and ensure that the network singleton is doing the right thing. It's not actually clear to me what that is - from a casual reading of bug 1698661 I can't tell why network needs the top browsing context id (I think it's to do with prioritizing requests in the visible tab?), and what the right behaviour is supposed to be if multiple windows are on-screen and visible. Nihanth or Dragana, can you clarify?

The top browser context id is used to prioritize the network requests not from all visible tabs, but from the top most focused window.
So, I think the right behavior when there are multiple visible windows is keeping the network code informed about the browser context id of the top most focused window.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1718082 - move listening to initial paint in a browser content window into the parent process, r?mconley

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1718082 - remove expired container/userContextId telemetry, r?pbz

Depends on D171409

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1718082 - make updating apptab and usercontext information solely tabbrowser's responsibility, r?mconley,pbz

I'm hoping to remove the apptab bits anyway, but trying to do this all one bit at a time.

Depends on D171410

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1718082 - move isAppTab from docshell to browsing context, r?nika!

Depends on D171411

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1718082 - move RPM initialization into ASRouter as it's only used for newtab, r?mconley

The one-off test here is kind of icky. I intend to file a follow-up to
transition the new tab page to the new RPM actor architecture too, like
we previously transitioned all the other pages that used to use
RemotePages and RPM, but I didn't want to block this work on that -
new tab only ever loads in the privileged about process, and so there's
no need for the overhead of the actor on the vast majority of processes
and browsingcontexts, and this patch gets us there, at least.

Depends on D171412

Comment 8

[:Gijs (he/him)]

The patch set I'm uploading is as-yet incomplete. I still need to deal with HasSiblings and with the network tracking of the topmost browsing context. The goal of this bug will be to completely remove BrowserTabChild and stop instantiating BrowserTab actors for every toplevel browsingcontext. The hope is that this will provide some minor perf gains, as well as some maintenance improvements (most of these patches are net code removal/simplification).

Comment 9

[:Gijs (he/him)]

Note to self: more recent versions of this patchset appears to fix bug 1777870.

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1718082 - move hasSiblings from BrowserChild to BC and update as a synced field, r?nika

Depends on D171413

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1718082 - track current tab using browserId instead of top browsing context id for network prioritization purposes, r?nika!,mconley!,valentin!

Depends on D171645

Comment 12

[:Gijs (he/him)]

I spent a significant amount of time poking at perfherder yesterday to see what this nets us. Honestly some of the results make for fairly confusing reading.

The expected and stable-metrics (if a little underwhelming) part is that there are small (just shy of 1%) improvements on Base Content JS and similar metrics that measure footprint of content processes. It's netting us about 15KiB. This is still nice because of course the effect is multiplied by # of content processes in practice.

What I think would be more interesting is improvements on pageload due to the content process running less actor JS. The browsertime metrics are a bit of a mixed bag. Here are the browsertime results. Some tentative conclusions I'm drawing there:

• on wikipedia and yahoo mail there are straight up improvements of a few % on visual content metrics.
• instagram and fandom (?) show improvements on Windows, and some regressions on Linux. Talking to the perf team on matrix, these tests are bi- or even tri-modal, and so it's not clear that any of those are "real". If anything, from "squinting at graphs", we expect this to be a small improvement (e.g. hitting the "lower" case of the bi/trimodal state more often).
• perfstat tests show a pretty random set of comparatively minor improvements/regressions on various internal operations that this patch doesn't touch, like webrender, styling, compositing, etc., and some changes on operations that could theoretically be impacted by this patch if it was broken somehow, but that don't really make sense either, like networking. I assume this means that the lack of browser JS running on pageload means we're ordering/measuring operations slightly differently, as I don't see how this changeset could otherwise have materially impacted e.g. displaylist stuff.
• Either way considering the improvements in paint / user-perceived content display times, and net code reduction/simplification, I guess we should just take this. A decent example of the contrast here is wikipedia which shows significant improvements on FirstVisualChange and (Perceptual)SpeedIndex, and regressions on the perfstats tests for the "individual parts" that allow us to paint the page.

There are also some confusing talos results - it looks like we dropped a significant amount of mainthread IO on Windows during tp5n, for reasons I don't really understand. There are also changes to glterrain and svg tests, which I again don't really understand. Given the absolute magnitude of the glterrain test (and the fact that the pattern is Windows-specific) I am not super concerned. The SVG thing also looks... less meaningful... cross-platform.

raptor doesn't show any confident changes.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8de827ad727e
move listening to initial paint in a browser content window into the parent process, r=mconley
https://hg.mozilla.org/integration/autoland/rev/35f0940c57f8
remove expired container/userContextId telemetry, r=pbz
https://hg.mozilla.org/integration/autoland/rev/7b642392f995
make updating apptab and usercontext information solely tabbrowser's responsibility, r=mconley
https://hg.mozilla.org/integration/autoland/rev/2eaccbf2a59f
move isAppTab from docshell to browsing context, r=nika,rpl
https://hg.mozilla.org/integration/autoland/rev/923aad9004a2
move RPM initialization into ASRouter as it's only used for newtab, r=mconley
https://hg.mozilla.org/integration/autoland/rev/d2a64db81d1c
move hasSiblings from BrowserChild to BC and update as a synced field, r=nika
https://hg.mozilla.org/integration/autoland/rev/75bc3f6b3110
track current tab using browserId instead of top browsing context id for network prioritization purposes, r=nika,mconley,necko-reviewers,kershaw,valentin

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/8de827ad727e
https://hg.mozilla.org/mozilla-central/rev/35f0940c57f8
https://hg.mozilla.org/mozilla-central/rev/7b642392f995
https://hg.mozilla.org/mozilla-central/rev/2eaccbf2a59f
https://hg.mozilla.org/mozilla-central/rev/923aad9004a2
https://hg.mozilla.org/mozilla-central/rev/d2a64db81d1c
https://hg.mozilla.org/mozilla-central/rev/75bc3f6b3110

Comment 15

[Acasandrei Beatrice (needinfo me)]

== Change summary for alert #37692 (as of Fri, 17 Mar 2023 09:52:15 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
1%	Base Content JS	macosx1015-64-shippable-qr	fission	1,632,749.33 -> 1,617,517.33
1%	Base Content JS	linux1804-64-shippable-qr	fission	1,632,266.67 -> 1,617,447.33
1%	Base Content JS	windows11-64-2009-shippable-qr	fission	1,635,064.67 -> 1,620,331.33
1%	Base Content JS	macosx1015-64-shippable-qr	fission	1,632,509.33 -> 1,617,752.00

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37692