_onLazyResize fires (and triggers sync reflow) for resize events that don't affect the browser window size

VERIFIED FIXED in Firefox 61

Status

()

P1
normal
VERIFIED FIXED
8 months ago
7 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Trunk
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 verified)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
STR:

0. install ohnoreflow
1. open browser window
2. open devtools
3. resize devtools like mad for 5 seconds
4. dump reflows

ER:
nothing from CUI, as browser width / main window size hasn't changed

AR:
bunch of sync reflows.
(Assignee)

Updated

8 months ago
Whiteboard: [fxperf]
Somehow this reminds me https://hg.mozilla.org/integration/mozilla-inbound/rev/3145f2c95016, maybe a similar fix is all we need.
Whiteboard: [fxperf] → [fxperf:p2]
(Assignee)

Updated

7 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 3

7 months ago
mozreview-review
Comment on attachment 8969662 [details]
Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu,

https://reviewboard.mozilla.org/r/238470/#review244218

::: commit-message-789e3:1
(Diff revision 1)
> +Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu, r?florian

As may be obvious, I slightly scope-creeped this from the bug report as written, and then scope-creeped it some more from this commit message which I forgot to update. Sorry.

Anyway, the upshot of this is that the only overflow-related case where we will still sync reflow is if you use customize mode in 1 window and have overflowed stuff in another window. Or something like that. I don't really think we need to care too much about that, and I don't know what stuff relies on that being handled sync, so it feels like the effort/reward balance doesn't justify tackling that here.

I verified by using ohnoreflow and resizing the window like a maniac. :-)
(Assignee)

Comment 4

7 months ago
If I had remembered, I would have fixed bug 1438490 first, but it's too late now. I'll do it after this lands, assuming I remember...
Blocks: 1438490
Comment hidden (mozreview-request)
(Assignee)

Comment 6

7 months ago
I removed the rangeParent check because it triggers sync reflow and (in my testing) is actually not effective at what it does, which means now the browser_resize test has no more remaining sync reflows.

If you put the bookmarks in the navbar and drag them from there, you can still trip this code (and make the bookmark toolbars end up in the overflow menu), but for me at least, the extant code is also not effective in preventing that from happening, so rather than having code that doesn't do anything useful but does cause sync reflow, I just removed it.

This problem might go away when we stop using XBL bindings for some of this stuff, which are causing all of the *Target properties on the event to be useless at detecting where the overflow comes from, but it might also help if we add end-margin on the bookmark toolbar items to avoid the drop indicator causing reflow in the parent (which I suspect it's doing today). Anyway, I didn't really want to fix that in this bug, we already have several bugs on file about it (e.g. bug 1403326 and others linked from there). I added back a half-baked attempt to stop overflow inside the bm toolbar from propagating, but as I said, it's not fully effective...
(Assignee)

Updated

7 months ago
Blocks: 1358388

Comment 8

7 months ago
mozreview-review
Comment on attachment 8969662 [details]
Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu,

https://reviewboard.mozilla.org/r/238470/#review244224

This avoids sync layout flushes at the cost of introducing slightly more flicker when opening small windows (I could see it when pressing Cmd+n for a few seconds on my fast macbook pro... it may be visible on very slow or overloaded machines), and causing us to repaint the toolbar a bit more. But I think the responsiveness win when resizing is worth it, so I'm leaning toward r+. There's just two questions below that I would like you to answer :-).

::: browser/components/customizableui/CustomizableUI.jsm:4508
(Diff revision 1)
>      while (this._list.firstChild) {
>        let child = this._list.firstChild;
>        let minSize = this._collapsed.get(child.id);
>  
>        if (!shouldMoveAllItems &&
> -          minSize &&
> +          minSize) {

nit: merge these 2 lines of the test.

Or back to the previous style with:

(targetWidth || this._target.clientWidth) <= minSize

::: browser/base/content/test/performance/browser_window_resize.js:15
(Diff revision 2)
>   * be modifying your code to avoid the reflow.
>   *
>   * See https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers
>   * for tips on how to do that.
>   */
> -const EXPECTED_REFLOWS = [
> +const EXPECTED_REFLOWS = [];

Nice :). Our other reflow tests typically have a comment urging people to not add any new whitelist entry when it's empty.

::: browser/components/customizableui/CustomizableUI.jsm:4415
(Diff revision 2)
> -  onOverflow(aEvent) {
> -    // The rangeParent check is here because of bug 1111986 and ensuring that
> -    // overflow events from the bookmarks toolbar items or similar things that
> -    // manage their own overflow don't trigger an overflow on the entire toolbar
> -    if (!this._enabled ||
> -        (aEvent && aEvent.target != this._toolbar.customizationTarget) ||
> +  /**
> +   * Avoid re-entrancy in the overflow handling by keeping track of invocations:
> +   */
> +  _lastOverflowCounter: 0,
> +
> +  /**

Can you explain why the (aEvent && aEvent.target != this._toolbar.customizationTarget) check is no longer needed?

::: browser/components/customizableui/CustomizableUI.jsm:4664
(Diff revision 2)
>        let minSize = this._collapsed.get(prevId);
>        this._collapsed.set(aNode.id, minSize);
>      } else {
>        // If it's now the first item in the overflow list,
>        // maybe we can return it:
> -      this._moveItemsBackToTheirOrigin();
> +      this._moveItemsBackToTheirOrigin(false, this._target.clientWidth);

Why is this better than letting the _moveItemsBackToTheirOrigin call the clientWidth getter itself?
(Assignee)

Comment 9

7 months ago
(In reply to Florian Quèze [:florian] from comment #8)
> Or back to the previous style with:
> 
> (targetWidth || this._target.clientWidth) <= minSize

Hmm. I changed it to avoid fetching it multiple times. If the check against minSize passes, we will break out of the loop so then it doesn't matter, but I guess it's possible that it doesn't at first, and does after we iterate several more times, each time putting items into the toolbar's customizationTarget, which dirties the DOM which then probably causes a layout flush (even though the _target's width won't really change so hopefully layout can optimize that some... but I digress).

So I guess I'll put the other bit on 1 line but keep the separate "lazy" assignment for `targetWidth` for when it's not passed. I could write `(targetWidth || (targetWidth = this._target.clientWidth)) <= minSize)` of course but that doesn't really make things neater, IMO...
 
> ::: browser/components/customizableui/CustomizableUI.jsm:4415
> (Diff revision 2)
> > -  onOverflow(aEvent) {
> > -    // The rangeParent check is here because of bug 1111986 and ensuring that
> > -    // overflow events from the bookmarks toolbar items or similar things that
> > -    // manage their own overflow don't trigger an overflow on the entire toolbar
> > -    if (!this._enabled ||
> > -        (aEvent && aEvent.target != this._toolbar.customizationTarget) ||
> > +  /**
> > +   * Avoid re-entrancy in the overflow handling by keeping track of invocations:
> > +   */
> > +  _lastOverflowCounter: 0,
> > +
> > +  /**
> 
> Can you explain why the (aEvent && aEvent.target !=
> this._toolbar.customizationTarget) check is no longer needed?

You added the same check to the actual listener, in toolbar.xml, in bug 1434945. :-)
So we already check this before the event gets here. Hence me referencing bug 1438490 - it'll be a lot easier to follow this stuff once it's all in 1 place, but either way there's no point checking the same thing twice...

> ::: browser/components/customizableui/CustomizableUI.jsm:4664
> (Diff revision 2)
> >        let minSize = this._collapsed.get(prevId);
> >        this._collapsed.set(aNode.id, minSize);
> >      } else {
> >        // If it's now the first item in the overflow list,
> >        // maybe we can return it:
> > -      this._moveItemsBackToTheirOrigin();
> > +      this._moveItemsBackToTheirOrigin(false, this._target.clientWidth);
> 
> Why is this better than letting the _moveItemsBackToTheirOrigin call the
> clientWidth getter itself?

It's not, I forgot to delete it when I was wavering about where to put definitive responsibility for having this value (caller or callee).

Originally I intended to make *all* the callers pass this, but then found that it doesn't make sense for the callers who pass `true` for `shouldMoveAllItems` to pass it, because it's never used, and if we made them fetch it those callers add extra sync reflows, which seems dumb. But it felt fragile to have some places pass it and not others, without having a fallback in the method itself, even if today that wouldn't be hit. So then I added a fallback. But anyway with the fallback obviously there's no point for this callsite to pass it as it doesn't fetch it without avoiding a reflow (and I don't want to change this particular callsite for the reasons in comment #3).


It also seems like there's try orange for browser/components/customizableui/test/browser_976792_insertNodeInWindow.js on windows and macOS debug. Probably a race condition of sorts. I'll look into this on Monday if not before...
(Assignee)

Comment 10

7 months ago
(In reply to :Gijs (he/him) from comment #9)
> (In reply to Florian Quèze [:florian] from comment #8)
> > Or back to the previous style with:
> > 
> > (targetWidth || this._target.clientWidth) <= minSize
> 
> Hmm. I changed it to avoid fetching it multiple times. If the check against
> minSize passes, we will break out of the loop so then it doesn't matter, but
> I guess it's possible that it doesn't at first, and does after we iterate
> several more times, each time putting items into the toolbar's
> customizationTarget, which dirties the DOM which then probably causes a
> layout flush (even though the _target's width won't really change so
> hopefully layout can optimize that some... but I digress).

Hmm. Though in fact, maybe we can just use dwu.getBoundsWithoutFlushing instead of .clientWidth to get the width of the customizationTarget, and that would avoid a flush here, too.
Comment hidden (mozreview-request)

Comment 12

7 months ago
mozreview-review
Comment on attachment 8969662 [details]
Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu,

https://reviewboard.mozilla.org/r/238470/#review245734

Thanks for the answers in the previous comments. r=me

::: browser/components/customizableui/CustomizableUI.jsm:4507
(Diff revision 3)
>        let child = this._list.firstChild;
>        let minSize = this._collapsed.get(child.id);
>  
> -      if (!shouldMoveAllItems &&
> -          minSize &&
> -          this._target.clientWidth <= minSize) {
> +      if (!shouldMoveAllItems && minSize) {
> +        if (!targetWidth) {
> +          let dwu = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);

nit: Looks like this is the only place where the 'win' variable is used, so it could be declared just above this line.
Attachment #8969662 - Flags: review?(florian) → review+
(Assignee)

Comment 13

7 months ago
(In reply to Florian Quèze [:florian] from comment #12)
> Comment on attachment 8969662 [details]
> Bug 1447619 - avoid reflowing when returning items to the toolbar from the
> overflow menu,
> 
> https://reviewboard.mozilla.org/r/238470/#review245734
> 
> Thanks for the answers in the previous comments. r=me
> 
> ::: browser/components/customizableui/CustomizableUI.jsm:4507
> (Diff revision 3)
> >        let child = this._list.firstChild;
> >        let minSize = this._collapsed.get(child.id);
> >  
> > -      if (!shouldMoveAllItems &&
> > -          minSize &&
> > -          this._target.clientWidth <= minSize) {
> > +      if (!shouldMoveAllItems && minSize) {
> > +        if (!targetWidth) {
> > +          let dwu = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> 
> nit: Looks like this is the only place where the 'win' variable is used, so
> it could be declared just above this line.

I moved it from just before:

    win.UpdateUrlbarSearchSplitterState();

at the bottom of the method, so unfortunately I can't do that because it'd be out of scope (let) or perhaps undefined (var, if the loop doesn't run at all). :-)

Comment 14

7 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1ce48405e58a
avoid reflowing when returning items to the toolbar from the overflow menu, r=florian

Comment 15

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ce48405e58a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

Updated

7 months ago
status-firefox61: fixed → ---
Target Milestone: Firefox 61 → ---

Updated

7 months ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

7 months ago
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: leave-open
Comment hidden (mozreview-request)

Comment 19

7 months ago
mozreview-review
Comment on attachment 8969662 [details]
Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu,

https://reviewboard.mozilla.org/r/238470/#review246386


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/customizableui/test/browser_901207_searchbar_in_panel.js:63
(Diff revision 4)
>    Services.prefs.setBoolPref("browser.search.widget.inNavBar", true);
>  
>    window.resizeTo(kForceOverflowWidthPx, window.outerHeight);
> -  await waitForCondition(() => navbar.getAttribute("overflowing") == "true");
> +  await waitForCondition(() => {
> +    return navbar.getAttribute("overflowing") == "true" &&
> +      !navbar.querySelector("#search-container"));

Error: Parsing error: unexpected token ) [eslint]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

7 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4525b4e9b22d
avoid reflowing when returning items to the toolbar from the overflow menu, r=florian

Comment 23

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4525b4e9b22d
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago7 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: qe-verify+
I reproduced this issue on Windows 10 x64, Ubuntu 17.04 x64 and Mac OS X 10.12 using Nightly 61.0a1 (2018-03-21).

I could not reproduce it on the latest Nightly 61.0a1 (2018-05-06) on Windows 10 x64, Ubuntu 17.04 x64 and Mac OS X 10.12, but I found the following error in the Browser Console "Error: browser.reflows is undefined".

Gijs, could you please take a look?
Flags: needinfo?(gijskruitbosch+bugs)
I suspect that this is because the latest versions of Firefox Nightly require you to set extensions.legacy.enabled to true in order to use Oh no! Reflow.

I have independently confirmed that this bug is fixed, fwiw. I feel comfortable marking it as VERIFIED.
Status: RESOLVED → VERIFIED
Flags: needinfo?(gijskruitbosch+bugs)
Based on comment 25, I will set the tracking flag for firefox61: verified.
status-firefox61: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.