1374691 - panelmultiview_XBL_Constructor is called at least twice when opening a new browser window

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Florian Quèze [:florian]]

I don't think the panelmultiview constructor should be called before the user opens the related panel.

See this profile: https://perf-html.io/public/25b18a57761fd44df2ff0b6224ca6722affbad9c/calltree/?implementation=js&range=4.4757_12.7395&search=panelmultiview_XBL_Constructor&thread=0

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1374691 - don't instantiate the panelmultiview and panelview bindings until necessary,

Review commit: https://reviewboard.mozilla.org/r/151648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151648/

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8880285 [details]
Bug 1374691 - don't instantiate the panelmultiview and panelview bindings until necessary,

https://reviewboard.mozilla.org/r/151648/#review156708

Thanks! The patch looks good to me, but I'm not very comfortable r+'ing this because I don't know the panelmultiview code, and so I don't fully understand if there may be unfortunate sideeffects for the calling JS code if it accesses a node without the binding attached.

You can land with my r+ if you are confident, or request an additional review (maybe from paolo or dao?).

::: browser/components/customizableui/content/panelUI.js:449
(Diff revision 1)
>        this._updateQuitTooltip();
>        this.panel.hidden = false;
> +      if (aCustomizing) {
> +        // Force initialization of the panel's XBL bindings or they will
> +        // race with customize mode and break it:
> +        getComputedStyle(this.multiView).MozBinding;

This likely causes an additional sync reflow, but I assume we don't care because it's only when entering customize mode.

Comment 3

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> Comment on attachment 8880285 [details]
> Bug 1374691 - don't instantiate the panelmultiview and panelview bindings
> until necessary,
> 
> https://reviewboard.mozilla.org/r/151648/#review156708
> 
> Thanks! The patch looks good to me, but I'm not very comfortable r+'ing this
> because I don't know the panelmultiview code, and so I don't fully
> understand if there may be unfortunate sideeffects for the calling JS code
> if it accesses a node without the binding attached.
> 
> You can land with my r+ if you are confident, or request an additional
> review (maybe from paolo or dao?).

Dão has told me in the past he wasn't comfortable either, Mike de Boer is out today, and I know Paolo is busy because I'm already waiting on another review from him... I'll ping mconley, also for the perf/customizableui crossover.

Mike, for context here: without the MozBinding getComputedStyle fetch (which AIUI should trigger a style flush but not necessarily a full layout flush?), what happens is:

- we unhide the panel. This doesn't immediately trigger the panelmultiview's binding to be constructed.
- we continue with the start of customizing (the panel unhiding via ensureReady is called from here: https://dxr.mozilla.org/mozilla-central/rev/f8bb96fb5c4f9960c5f754f877eceb677df18ddc/browser/components/customizableui/CustomizeMode.jsm#317-324 )
- customize mode moves the panel view into the panel holder, here: https://dxr.mozilla.org/mozilla-central/rev/f8bb96fb5c4f9960c5f754f877eceb677df18ddc/browser/components/customizableui/CustomizeMode.jsm#334-337
- at some point, a style flush happens and the binding is installed, which runs the constructor for the panelmultiview, which pulls the main view back in (!) to the panel, so then the panel part of customize mode is empty.

This broke automated tests, which is how I noticed. :-)

We could alternatively try to teach the constructor of the panelmultiview to not pull in the main view. I'm not sure whether that's actually better, I could be convinced either way.

I also realize, upon re-reading this patch, that I probably need to do the same thing in the photon case...

Comment 4

[:Gijs (he/him)]

Comment on attachment 8880285 [details]
Bug 1374691 - don't instantiate the panelmultiview and panelview bindings until necessary,

https://reviewboard.mozilla.org/r/151648/#review156730

r- because it should include the photon bits, but I'd like to have a better idea if the approach is sane before updating either way.

Comment 5

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

(In reply to :Gijs from comment #3)

> - we unhide the panel. This doesn't immediately trigger the panelmultiview's
> binding to be constructed.
> - we continue with the start of customizing (the panel unhiding via
> ensureReady is called from here:
> https://dxr.mozilla.org/mozilla-central/rev/
> f8bb96fb5c4f9960c5f754f877eceb677df18ddc/browser/components/customizableui/
> CustomizeMode.jsm#317-324 )
> - customize mode moves the panel view into the panel holder, here:
> https://dxr.mozilla.org/mozilla-central/rev/
> f8bb96fb5c4f9960c5f754f877eceb677df18ddc/browser/components/customizableui/
> CustomizeMode.jsm#334-337
> - at some point, a style flush happens and the binding is installed, which
> runs the constructor for the panelmultiview, which pulls the main view back
> in (!) to the panel, so then the panel part of customize mode is empty.
> 

Huh... so it sounds like we might need to have CustomizeMode.jsm wait for the binding to finish being constructed before slurping it out into customize mode. It's already waiting for PanelUI to report that it's ready... could we not somehow wait for the "natural" style and layout flush to complete before resolving the ensureReady Promise? Perhaps using this pattern:

requestAnimationFrame(() => {
  Services.tm.dispatchToMainThread(() => {
    // This will fire very early after the next "natural" flush
  });
});

Comment 6

[:Gijs (he/him)]

Comment on attachment 8880285 [details]
Bug 1374691 - don't instantiate the panelmultiview and panelview bindings until necessary,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151648/diff/1-2/

Comment 7

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #5)
> could we not somehow wait for the "natural" style and layout flush
> to complete before resolving the ensureReady Promise? Perhaps using this
> pattern:
> 
> requestAnimationFrame(() => {
>   Services.tm.dispatchToMainThread(() => {
>     // This will fire very early after the next "natural" flush
>   });
> });

Excellent idea. I put this in the customize mode code instead, because the normal panel opening doesn't need to wait, as far as I can tell. It "just works". So I kind of assume that we only really need this for customize mode - not a lot else directly cares about the panelmultiview.

Comment 8

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

Comment on attachment 8880285 [details]
Bug 1374691 - don't instantiate the panelmultiview and panelview bindings until necessary,

https://reviewboard.mozilla.org/r/151648/#review157532

::: browser/components/customizableui/CustomizeMode.jsm:317
(Diff revision 2)
>          }
>          window.PanelUI.menuButton.addEventListener("command", this);
>          window.PanelUI.menuButton.open = true;
>          window.PanelUI.beginBatchUpdate();
>  
> -        // The menu panel is lazy, and registers itself when the popup shows. We
> +        // The menu panel is lazy, and registers itself when the popup shows. 

Nit - trailing ws

::: browser/components/customizableui/CustomizeMode.jsm:330
(Diff revision 2)
> +            window.requestAnimationFrame(() => {
> +              Services.tm.dispatchToMainThread(resolve);
> +            });

For future reference (I just learned this myself), requestIdleCallback can be used for this sort of thing as well:

https://developer.mozilla.org/en-US/docs/Web/API/Background_Tasks_API#Getting_the_most_out_of_idle_callbacks

"By the time your callback is run, the current frame has already finished drawing, and all layout updates and computations have been completed. If you make changes that affect layout, you may force a situation in which the browser has to stop and do recalculations that would otherwise be unnecessary. If your callback needs to change the DOM, it should use Window.requestAnimationFrame() to schedule that."

I'll leave it up to you whether or not you want to use requestIdleCallback here instead - that might actually be better, tbh.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8880285 [details]
Bug 1374691 - don't instantiate the panelmultiview and panelview bindings until necessary,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151648/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f944066c0604
don't instantiate the panelmultiview and panelview bindings until necessary, r=florian,mconley

Comment 11

[Carsten Book [:Tomcat]]

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