Closed Bug 1358415 Opened 4 years ago Closed 4 years ago

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

Categories

(WebExtensions :: Frontend, defect, P1)

54 Branch
defect

Tracking

(firefox-esr52 unaffected)

RESOLVED FIXED
mozilla55
Iteration:
55.4 - May 1
Tracking Status
firefox-esr52 --- unaffected

People

(Reporter: rjward0, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [ohnoreflow][qf][photon-performance])

Attachments

(1 file)

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
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 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+
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
Closed: 4 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
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.