1358415 - 1.08ms uninterruptible reflow at get width@chrome://browser/content/ext-utils.js:528:5

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Hazel Ward]

Here's the stack:

get width@chrome://browser/content/ext-utils.js:528:5
convert@resource://gre/modules/ExtensionTabs.jsm:469:7
convert@resource://gre/modules/ExtensionTabs.jsm:1639:12
getAPI/create/<@chrome://browser/content/ext-tabs.js:397:20
promise callback*create@chrome://browser/content/ext-tabs.js:301:18
call@resource://gre/modules/ExtensionParent.jsm:612:24
async*receiveMessage@resource://gre/modules/ExtensionParent.jsm:524:11

Comment 1

[Florian Quèze [:florian]]

There's a call to the clientWidth getter at http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/browser/components/extensions/ext-utils.js#527 from http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/toolkit/components/extensions/ExtensionTabs.jsm#469

Calling .clientWidth (or clientHeight a few lines above) is very expensive as it triggers a synchronous reflow. Could this either be made lazy (so that it's computed only if the add-on really needs this value), or would it be OK to use stale data from getBoundsWithoutFlushing?

Comment 2

[Kris Maglione [:kmag]]

We actually fixed this in bug 1240326, but it got regressed when bug 1260548 landed (since the patch on that was started before the original fix).

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1358415: Don't trigger reflow just to compute tab geometry.

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

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8860510 [details]
Bug 1358415: Don't trigger reflow just to compute tab geometry.

https://reviewboard.mozilla.org/r/132518/#review135400

::: browser/components/extensions/ext-tabs.js:115
(Diff revision 1)
>            };
>          }).api(),
>  
>          onCreated: new SingletonEventManager(context, "tabs.onCreated", fire => {
>            let listener = (eventName, event) => {
> -            fire.async(tabManager.convert(event.nativeTab));
> +            fire.async(tabManager.convert(event.nativeTab, event.currentTab));

do we need the same change for fennec?

::: browser/components/extensions/ext-utils.js:250
(Diff revision 1)
> +
>          // We need to delay sending this event until the next tick, since the
>          // tab does not have its final index when the TabOpen event is dispatched.
>          Promise.resolve().then(() => {
>            if (event.detail.adoptedTab) {
> -            this.emitAttached(event.originalTarget);
> +            this.emitAttached(event.originalTarget, currentTab);

this appears to not be needed/used

::: toolkit/components/extensions/ExtensionTabs.jsm:291
(Diff revision 1)
>  
>    /**
> +   * @property {nsIFrameLoader} browser
> +   *        Returns the frameloader for the given tab.
> +   *        @readonly
> +   *        @abstract

this is not abstract...

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8860510 [details]
Bug 1358415: Don't trigger reflow just to compute tab geometry.

https://reviewboard.mozilla.org/r/132518/#review135400

> do we need the same change for fennec?

Ideally, but it's less of an issue on fennec, since it doesn't have much XUL UI.

> this is not abstract...

Yeah, I already fixed that.

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b23450ae23c1776a8ad3a5f80377e14dd05a562
Bug 1358415: Don't trigger reflow just to compute tab geometry. r=aswan

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9b23450ae23c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Should we consider this for Beta uplift or let it ride the 55 train?

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8860510 [details]
Bug 1358415: Don't trigger reflow just to compute tab geometry.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1260548
[User impact if declined]: This has the potential to cause responsiveness issues and jank during tab animations.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low-risk
[Why is the change risky/not risky?]: This is a relatively minor change that restores the logic that we used prior to a refactoring.
[String changes made/needed]: None.

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8860510 [details]
Bug 1358415: Don't trigger reflow just to compute tab geometry.

Fix a regression. Beta54+. Should be in 54 beta 6.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cd779fa35a3e