1917458 - Only send resizes for remote browsers once per frame.

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1917458 - Only send resizes for remote browsers once per frame. r=smaug,#layout,mstange

Do it at the same time as UpdateRemoteFrameEffects() (so, intersection
observer timing). Otherwise we can see flickering sometimes from the
resize with the previous patch (and this should be less work, when we
resize).

Non-remote frames need to synchronously communicate their resizes
(because JS could access the frame), but that's not an issue for
non-remote frames.

This should also more reliably prevent issues like bug 1910887 or like
bug 1764655, and paves the way for fixing bug 1750189 (which stalled) in
a similar fashion.

I tested this in a build with a couple hundred tabs open and it doesn't
measurably show up. I think we should consider not communicating resizes
to background tabs / hidden remote iframes, at least for top levels.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1917458 - Don't send resizes to inactive toplevels. r=smaug,#layout,mstange

See incoming patch for reasoning.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1917458 - Remove bookmarks-toolbar-overlapping-browser code. r=#theme

After the previous patch, we don't jank all remote tabs when resizing
the browser, so this can go. This should also make general window
resizing faster.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1917458 - Factor out remote effects update. r=smaug,#layout,mstange

No behavior change, just moving code around for the following patches.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1917458 - Update remote effects on top descendants. r=smaug,#layout,mstange

This is currently not an issue because for top levels we don't look at
the effects info. But:

• It is useful, because it might allow to remove some special-cases
  around here[1].

• For the next patch I want to also communicate resizes, where we'll
  need to handle them.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b1c3c2aef15
Factor out remote effects update. r=mstange

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4cca274eb5a
Update remote effects on top descendants. r=mstange

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65384dcd3d9a
Only send resizes for remote browsers once per frame. r=mstange,extension-reviewers,tabbrowser-reviewers,dao,robwu

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f6d758ac9c6
Don't send resizes to inactive toplevels. r=mstange

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/157eb2ada2ca
Remove bookmarks-toolbar-overlapping-browser code. r=desktop-theme-reviewers,tabbrowser-reviewers,dao

Comment 11

[Cristian Tuns]

Backed out for causing wpt failures in no_window_open_when_term_nesting_level_nonzero.window.html

• Backout link
• Push with failures
• Failure Log
• Failure line: PROCESS-CRASH | MOZ_ASSERT(false) (MOZ_ASSERT_UNREACHABLE: Top level BrowserChild w/ non-top level Document?) [@ mozilla::dom::GetOopIframeMetrics] | /html/browsers/the-window-object/open-close/no_window_open_when_term_nesting_level_nonzero.window.html
  TEST-UNEXPECTED-CRASH | /html/browsers/the-window-object/open-close/no_window_open_when_term_nesting_level_nonzero.window.html | expected ERROR

Comment 12

[Cristian Tuns]

We also have this failure coming from this bug: https://treeherder.mozilla.org/logviewer?job_id=474367350&repo=autoland if you could take a look at it before you land the fix.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1917458 - Fix puppeteer/remote tests after D221995. r=whimboo,#webdriver-reviewers

Calling setViewport on a background tab would wait forever without this
tweak after D221995, because we won't resize it until it becomes active.

Some tests like the ones mentioned in comment 12 call newPage() multiple
times at once, and newPage() calls setViewport() if there is a default
viewport set, which triggered this bug.

Make this work by reusing the AppWindow logic for sizing popup windows,
with a tweak so that we also resize background tabs.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3957d03459ec
Factor out remote effects update. r=mstange

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3dc5d769ffe
Update remote effects on top descendants. r=mstange
https://hg.mozilla.org/integration/autoland/rev/22eeb2b5ca5b
Only send resizes for remote browsers once per frame. r=mstange,extension-reviewers,tabbrowser-reviewers,dao,robwu

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/3957d03459ec
https://hg.mozilla.org/mozilla-central/rev/c3dc5d769ffe
https://hg.mozilla.org/mozilla-central/rev/22eeb2b5ca5b

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b365189821a
Don't send resizes to inactive toplevels. r=mstange
https://hg.mozilla.org/integration/autoland/rev/05355ea5dd7c
Remove bookmarks-toolbar-overlapping-browser code. r=desktop-theme-reviewers,tabbrowser-reviewers,dao
https://hg.mozilla.org/integration/autoland/rev/308329f23acd
Fix puppeteer/remote tests after D221995. r=webdriver-reviewers,webidl,smaug,jdescottes

Comment 18

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/1b365189821a
https://hg.mozilla.org/mozilla-central/rev/05355ea5dd7c
https://hg.mozilla.org/mozilla-central/rev/308329f23acd

Comment 20

[Andrej (:aglavic)]

(In reply to Pulsebot from comment #15)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3dc5d769ffe
Update remote effects on top descendants. r=mstange
https://hg.mozilla.org/integration/autoland/rev/22eeb2b5ca5b
Only send resizes for remote browsers once per frame.
r=mstange,extension-reviewers,tabbrowser-reviewers,dao,robwu

Perfherder has detected a talos performance change from push 22eeb2b5ca5b2d4a8e80ed34a1159d60f5cd594f.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
19%	tresize	linux1804-64-shippable-qr	e10s fission stylo webrender	27.47 -> 22.33
18%	tresize	linux1804-64-shippable-qr	e10s fission stylo webrender	27.98 -> 22.91
13%	tresize	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	28.44 -> 24.73
10%	tresize	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	7.03 -> 6.34
3%	tresize	windows11-64-shippable-qr	e10s fission stylo webrender-sw	5.27 -> 5.09

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 2175

For more information on performance sheriffing please see our FAQ.