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

VERIFIED FIXED in Firefox 61

Status

()

defect
P1
normal
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Depends on 1 bug)

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

Firefox Tracking Flags

(firefox61 verified)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 attachment)

(Assignee)

Description

a year 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

a year 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

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

Comment 3

a year 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

a year 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

a year 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

a year ago
Blocks: 1358388

Comment 8

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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
https://hg.mozilla.org/mozilla-central/rev/1ce48405e58a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Target Milestone: Firefox 61 → ---
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

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

Comment 19

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4525b4e9b22d
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.
Flags: qe-verify+

Updated

2 months ago
Depends on: 1528656
You need to log in before you can comment on or make changes to this bug.