1488871 - AsyncScrollPortEvent as an WillPaintObserver causes extra layout flushes during page load

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

I was profiling https://www.nationalgeographic.com/ page load and saw odd looking  layout flush coming from RunWillPaintObservers.
Only user of that API is https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/layout/generic/nsGfxScrollFrame.cpp#5273

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Other browsers don't seem to support the scrollport events. Can we make them chromeonly / remove them?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

That I don't know. They are a layout feature.

Do we use them somewhere, especially, do we use them in child process?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Looks like devtools uses it, so should be feasible to remove... That being said, that flush there looks a bit hackish.

It's needed so the scrollport size and such is the correct one at the time Run() runs, but actually the rect we should be interested in is the one that we had after reflow from PostOverflowEvent (and we should always fire the event I think, unconditionally on what the current state of the frame is).

I could write a patch for that tomorrow, I think.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Asynchronousness and flushing is there to avoid extra events, no?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Asynchronousness and flushing is there to avoid extra events, no?

Maybe? Oh, I guess we reflow the scroll frame way too often to check for different scrollbar combinations... But probably reflowing is not needed if we stash the relevant rects in the AsyncScrollPortEvent.

Comment 6

[Mats Palmgren (inactive)]

This is the overflow/underflow events, right?
WebKit used to support an "overflowchanged" event - did they remove that?
If not, then I suspect removing our events is likely to cause web-compat issues.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #6)
> This is the overflow/underflow events, right?
> WebKit used to support an "overflowchanged" event - did they remove that?

Blink did: https://bugs.chromium.org/p/chromium/issues/detail?id=460822

Comment 8

[Mats Palmgren (inactive)]

I'm pleasantly surprised, that's great!  Then I think we can remove
ours too (assuming Blink haven't introduced some new feature for this).

The dev guides I found on the issue, like:
http://www.backalleycoder.com/2013/03/14/oft-overlooked-overflow-and-underflow-events/
relies on overflowchanged *or* overflow/underflow so if the former
is now broken then it seems the web-compat risk is low.
("intranet apps" being the dark horse as usual)

We should announce an intent-to-remove on this though.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1488871 - Don't flush layout in AsyncScrollPortEvent::Run. r=mats

I'd claim that we don't need it because, in order to enqueue the event, we
already need to have overflowed the event in a normal reflow.

For now this should not break anything (or anything that wasn't already racy
depending on when we paint).

The only reason the flush is there is according to roc is to decide whether to
fire the event, and because it needs the layout information:

  https://bugzilla.mozilla.org/show_bug.cgi?id=771822#c4

In practice, however, all the layout information we need we have already
computed by the time we post the event.

We don't expose the rects via the event details, which is what could get
out-of-date, so this patch could only mean that we fire the event slightly more
often in cases where people remove stuff from the DOM, right after we do layout
and the content has overflowed. But that's actually pretty unlikely.

This event in general is pretty problematic because it exposes when we do
layout and when we paint, which is not great. Its test coverage is also pretty
low (test_overflow_event.html, which of course still passes without this).

I still want to do this change first since it's trivial to back out if needed.

Then I'd want to change how it fires to match the scrolled area change event
(which would allow us to remove the WillPaintObserver stuff), after verifying
that chrome consumers are still fine with that, and then put behind a pref and
hide it from content, while we leave time for chrome consumers to migrate away
from it, and allow us to revert if something breaks. Does this sound reasonable?
If so, I'll file bugs for that with patches.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 9006702 [details]
Bug 1488871 - Don't flush layout in AsyncScrollPortEvent::Run. r=mats

Mats Palmgren (:mats) has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc5ee8a04645
Don't flush layout in AsyncScrollPortEvent::Run. r=mats

Comment 12

[Andrei Ciure[:aciure]]

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

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/1552d735c49f
Backed out changeset fc5ee8a04645 for causing perma bc failures in browser/base/content/test/performance/browser_windowopen.js. a=backout

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Pulsebot from comment #13)
> Backout by [e-mail]:
> https://hg.mozilla.org/mozilla-central/rev/1552d735c49f
> Backed out changeset fc5ee8a04645 for causing perma bc failures in
> browser/base/content/test/performance/browser_windowopen.js. a=backout

Would've been nice to reopen the bug. Do you have a link to the failure?

Comment 15

[Eliza Balazs [:ebalazs_]]

Backed out changeset fc5ee8a04645 (Bug 1488871) for causing perma bc failures in browser/base/content/test/performance/browser_windowopen.js. a=backout

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fc5ee8a046456b4d2affd2142954ea8e3b685322&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=197787808

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=197787808&repo=autoland&lineNumber=13750

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=1552d735c49f5479e468a0b9e2ac11ed03575219&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1488871 - Make browser_windowopen.js workaround account for the toolbar background. r=mconley,florian

With the other patch in this bug, we get two rects, not > 5. See:

  https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fc5ee8a046456b4d2affd2142954ea8e3b685322&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=197787808

It's not clear if this workaround is getting hit on other builds other than OSX
debug, probably I could try to make this more specific... I'll fire a couple
try runs with the condition changed.

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 9007152 [details]
Bug 1488871 - Make browser_windowopen.js workaround account for the toolbar background. r=mconley,florian

Florian Quèze [:florian] has approved the revision.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9007152 [details]
Bug 1488871 - Make browser_windowopen.js workaround account for the toolbar background. r=mconley,florian

Mike Conley (:mconley) (:⚙️) has approved the revision.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39b3a22e2552
Don't flush layout in AsyncScrollPortEvent::Run. r=mats
https://hg.mozilla.org/integration/autoland/rev/078929426a6e
Make browser_windowopen.js workaround account for the toolbar background. r=mconley,florian

Comment 20

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/39b3a22e2552
https://hg.mozilla.org/mozilla-central/rev/078929426a6e

Comment 21

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_windowopen.js

backout: https://hg.mozilla.org/integration/autoland/rev/c7209352c57a979b096a9e1b08d74372c490cd57

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=078929426a6e1f65e0526b526ceb2bb8fcdc7124&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=198129458

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198129461&repo=autoland&lineNumber=1924

13:33:39     INFO - Console message: OpenGL compositor Initialized Succesfully.
13:33:39     INFO - Version: 2.1 INTEL-10.6.33
13:33:39     INFO - Vendor: Intel Inc.
13:33:39     INFO - Renderer: Intel Iris OpenGL Engine
13:33:39     INFO - FBO Texture Target: TEXTURE_2D
13:33:39     INFO - Buffered messages logged at 13:33:39
13:33:39     INFO - TEST-FAIL | browser/base/content/test/performance/browser_windowopen.js | known reflow at rect@chrome://browser/content/browser-tabsintitlebar.js was encountered 4 times - 
13:33:39     INFO - Full stack:
13:33:39     INFO -   rect@chrome://browser/content/browser-tabsintitlebar.js:173:23
13:33:39     INFO -   _layOutTitlebar@chrome://browser/content/browser-tabsintitlebar.js:209:26
13:33:39     INFO -   update@chrome://browser/content/browser-tabsintitlebar.js:162:5
13:33:39     INFO -   whenWindowLayoutReady@chrome://browser/content/browser-tabsintitlebar.js:46:5
13:33:39     INFO -   onDOMContentLoaded@chrome://browser/content/browser.js:1285:5
13:33:39     INFO -   EventListener.handleEvent*@chrome://browser/content/browser.xul:114:3
13:33:39     INFO -   
13:33:39     INFO - Full stack:
13:33:39     INFO -   rect@chrome://browser/content/browser-tabsintitlebar.js:173:23
13:33:39     INFO -   _layOutTitlebar@chrome://browser/content/browser-tabsintitlebar.js:212:34
13:33:39     INFO -   update@chrome://browser/content/browser-tabsintitlebar.js:162:5
13:33:39     INFO -   whenWindowLayoutReady@chrome://browser/content/browser-tabsintitlebar.js:46:5
13:33:39     INFO -   onDOMContentLoaded@chrome://browser/content/browser.js:1285:5
13:33:39     INFO -   EventListener.handleEvent*@chrome://browser/content/browser.xul:114:3
13:33:39     INFO -   
13:33:39     INFO - Full stack:
13:33:39     INFO -   rect@chrome://browser/content/browser-tabsintitlebar.js:173:23
13:33:39     INFO -   _layOutTitlebar@chrome://browser/content/browser-tabsintitlebar.js:216:31
13:33:39     INFO -   update@chrome://browser/content/browser-tabsintitlebar.js:162:5
13:33:39     INFO -   whenWindowLayoutReady@chrome://browser/content/browser-tabsintitlebar.js:46:5
13:33:39     INFO -   onDOMContentLoaded@chrome://browser/content/browser.js:1285:5
13:33:39     INFO -   EventListener.handleEvent*@chrome://browser/content/browser.xul:114:3
13:33:39     INFO -   
13:33:39     INFO - Full stack:
13:33:39     INFO -   rect@chrome://browser/content/browser-tabsintitlebar.js:173:23
13:33:39     INFO -   _layOutTitlebar@chrome://browser/content/browser-tabsintitlebar.js:228:33
13:33:39     INFO -   update@chrome://browser/content/browser-tabsintitlebar.js:162:5
13:33:39     INFO -   whenWindowLayoutReady@chrome://browser/content/browser-tabsintitlebar.js:46:5
13:33:39     INFO -   onDOMContentLoaded@chrome://browser/content/browser.js:1285:5
13:33:39     INFO -   EventListener.handleEvent*@chrome://browser/content/browser.xul:114:3
13:33:39     INFO -   
13:33:39     INFO - TEST-PASS | browser/base/content/test/performance/browser_windowopen.js | 0 unexpected reflows - true == true - 
13:33:39     INFO - comparing 3 frames
13:33:39     INFO - Buffered messages finished
13:33:39     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_windowopen.js | unexpected changed rect: ({x1:0, x2:105, y1:0, y2:32, w:106, h:33}), window width: 1280 - 
13:33:39     INFO - Stack trace:
13:33:39     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reportUnexpectedFlicker/rects<:573
13:33:39     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reportUnexpectedFlicker:564
13:33:39     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:withPerfObserver:632
13:33:39     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/browser_windowopen.js:null:109
13:33:39     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1102
13:33:39     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1093
13:33:39     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:995
13:33:39     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
13:33:39     INFO - Not taking screenshot here: see the one that was previously logged

Comment 22

[Mats Palmgren (inactive)]

Looks like the test failure was resolved.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Nope, this was backed out but somehow not reopened... I still need to land this.

Comment 24

[Daniel Holbert [:dholbert]]

Hi Emilio! Just want to be sure this doesn't fall too far off your radar -- is this likely as simple as just re-landing (per comment 22)?

(I guess some rebasing may be needed, but hopefully this is set to go otherwise?)

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Here is a try run based on the latest trunk with the patch for bug 1488953.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8028e81e5c707b4383705e4b59086a43704afdec

It's mostly green on most platforms, but there are three failures, performance/browser_windowopen.js is timed out on Win10 x64 ASAN, browser_UrlbarInput_unit.js fails on Windows 7 and performance/browser_tabswitch.js fails on Windows 7. So I suspect there is something Windows 7 specific issue, but I no longer have any Windows 7 machines.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

browser_UrlbarInput_unit.js creates a mock of the UrlbarInput, and it seems that it relies on the flush we are going to drop in this bug. Adding an explicit flush fixed the failure.

Drew, does adding the explicit flush sound reasonable?

Comment 27

[Drew Willcoxon :adw]

Yes, it doesn't seem like that's related to what the test is testing, so that should be OK.

Comment 28

[Drew Willcoxon :adw]

Oh could you include this bug number in the code comment please? Makes it a little easier to understand why it's there than searching through blame.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Thanks Drew! I will add some comments there and send a review request to you soon.

(Though in this bug there are other issues need to be solved)

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1488871 - Add an explicit style flush to make sure the added textbox and popupset is styled before proceeding tests in browser_UrlbarInput_unit.js. r?adw

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Maybe the test failures will be disappeared by bug 1513325. CCing :ntim.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Doubtful, the urlbar already uses <html:input>

Comment 33

[Hiroyuki Ikezoe (:hiro)]

I did push a new try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73561caefc2529469e9f314acaaa814bcfa983ca&selectedJob=248154218

The failure on Windows 7 have been passed for some reasons, instead browser_windowopen.js failed both on Windows 10/7 opt builds.

After poking around relevant places, I noticed that withPerfObserver doesn't work as expected, doesn't work as what browser normally does, because recordFrames calls drawWindow to take a screenshot and the drawWindow ends up calling FlushWillPaintObservers, thus it calls AsyncScrollPortEvent::Run then it calls FlushPendingNotifications which is exactly what this bug is going to drop. So, we inadvertently invokes an additional flush in withPerfObserver and the flush has been wallpapering some flickers which have been appeared (but unnoticeable I guess) on normal browsing.

As a proof of the wallpapering, I did push another try with skipping the FlushWillPaintObversers calls (and without any changes for this bug) when the snapshot is taken in recordFrames.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f955c05730c934016f5dae4da642bc8a6a18136&selectedJob=248918813

browser_windowopen.js actually failed and the result is pretty much the same as the failure with the changes for this bug.

mconley, does this sound a reasonable reason to add an exception in browser_windowopen.js test? (And I suppose the placeholder text movement is caused by the same root cause)

A note for Emilio : I didn't include the patch for bug 1488953 in tries since it causes another failure I am going to tackle the failures one by one.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

If I understand correctly, we were always shipping this flicker, and for the reasons you identified, we weren't recording it properly for this test.

If that's the case, then yes, I think it makes sense to add an exception.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Thank you! I filed bug 1555842 for further investigation what the root cause of the urlbar flicker.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1488871 - Add an exception for the placeholder in the urlbar flickers to browser_windowopen.js. r?mconley

recordFrames has been wallpapering these flickers because the function ends up
calling FlushPendingNotifications in AsyncScrollPortEvent::Run() and we hadn't
noticed the wallpaper until we tried to remove the FlushPendingNotifications in
AsyncScrollPortEvent::Run().

Comment 37

[Hiroyuki Ikezoe (:hiro)]

The browser_windowopen.js failure appeared on MacOSX and Linux opt builds too ([1] for example), and oddly it appeared on Linux ASAN build. (I am wondering why it doesn't fail on any debug builds?) Anyways, I am going to apply the exception in D33226 to those platforms.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=41c7e1814ebdefe7546e6e588f54363c6a559d82&selectedJob=249505514

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f35e6fc8f91
Make browser_windowopen.js workaround account for the toolbar background. r=florian,mconley
https://hg.mozilla.org/integration/autoland/rev/eddcd90c1ffa
Add an exception for the placeholder in the urlbar flickers to browser_windowopen.js. r=mconley
https://hg.mozilla.org/integration/autoland/rev/cfac49a18cf1
Don't flush layout in AsyncScrollPortEvent::Run. r=mats
https://hg.mozilla.org/integration/autoland/rev/a132736ab17e
Add an explicit style flush to make sure the added textbox and popupset is styled before proceeding tests in browser_UrlbarInput_unit.js. r=adw

Comment 39

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5f35e6fc8f91
https://hg.mozilla.org/mozilla-central/rev/eddcd90c1ffa
https://hg.mozilla.org/mozilla-central/rev/cfac49a18cf1
https://hg.mozilla.org/mozilla-central/rev/a132736ab17e

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Thanks hiro!