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

RESOLVED FIXED in mozilla55

Status

defect
P1
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: rjward0, Assigned: kmag)

Tracking

(Blocks 1 bug, {regression})

54 Branch
mozilla55
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected)

Details

(Whiteboard: [ohnoreflow][qf][photon-performance])

Attachments

(1 attachment)

Reporter

Description

2 years ago
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
Flags: qe-verify?
Priority: -- → P2
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?
Blocks: 1260548
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
Assignee

Comment 2

2 years ago
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).
Assignee: nobody → kmaglione+bmo
Blocks: 1240326
Component: WebExtensions: General → WebExtensions: Frontend
Keywords: regression
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
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...
Attachment #8860510 - Flags: review?(aswan) → review+
Assignee

Comment 5

2 years ago
mozreview-review-reply
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.
https://hg.mozilla.org/mozilla-central/rev/9b23450ae23c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Iteration: --- → 55.4 - May 1
Should we consider this for Beta uplift or let it ride the 55 train?
Flags: needinfo?(kmaglione+bmo)
Version: unspecified → 54 Branch
Flags: qe-verify? → qe-verify-
Duplicate of this bug: 1357577
Assignee

Comment 10

2 years ago
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.
Flags: needinfo?(kmaglione+bmo)
Attachment #8860510 - Flags: approval-mozilla-beta?
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.
Attachment #8860510 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1374597

Updated

11 months ago
Product: Toolkit → WebExtensions

Updated

7 months ago
You need to log in before you can comment on or make changes to this bug.