2.00ms uninterruptible reflow at attach@resource:///modules/ExtensionPopups.jsm:488:5

RESOLVED FIXED in Firefox 67

Status

defect
P1
normal
RESOLVED FIXED
a year ago
27 days ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

(Blocks 1 bug, Regressed 1 bug, {perf})

unspecified
mozilla67
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [ohnoreflow][qf:p3:responsiveness][fxperf:p3])

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

a year ago
While looking into Bug 1359408 I noticed that ohnoreflow is also catching a reflow triggered from the ViewPopup's attach function (defined in ExtensionPopups.jsm).

Here's the stack:

attach@resource:///modules/ExtensionPopups.jsm:488:5
async*onViewShowing@chrome://browser/content/ext-browserAction.js:212:33
async*wrapWidgetEventHandler/aWidget[aEventName]@resource:///modules/CustomizableUI.jsm:2516:16
dispatchCustomEvent@resource:///modules/PanelMultiView.jsm:202:5
dispatchCustomEvent@resource:///modules/PanelMultiView.jsm:1252:38
dispatchAsyncEvent/this._blockersPromise<@resource:///modules/PanelMultiView.jsm:236:20
promise callback*dispatchAsyncEvent@resource:///modules/PanelMultiView.jsm:234:36
async*_openView@resource:///modules/PanelMultiView.jsm:761:26
async*_showMainView@resource:///modules/PanelMultiView.jsm:727:17
async*openPopup/this._openPopupPromise<@resource:///modules/PanelMultiView.jsm:510:21
promise callback*openPopup@resource:///modules/PanelMultiView.jsm:493:37
async*openPopup@resource:///modules/PanelMultiView.jsm:280:57
async*showSubView@chrome://browser/content/customizableui/panelUI.js:439:27
async*handleWidgetCommand@resource:///modules/CustomizableUI.jsm:1651:7
EventListener.handleEvent*buildWidget@resource:///modules/CustomizableUI.jsm:1493:7
getWidgetNode@resource:///modules/CustomizableUI.jsm:986:16
insertNodeInWindow@resource:///modules/CustomizableUI.jsm:1236:26
insertNode@resource:///modules/CustomizableUI.jsm:1222:7
onWidgetAdded@resource:///modules/CustomizableUI.jsm:1029:5
notifyListeners@resource:///modules/CustomizableUI.jsm:2212:11
createWidget@resource:///modules/CustomizableUI.jsm:2322:9
createWidget@resource:///modules/CustomizableUI.jsm:3382:7
build@chrome://browser/content/ext-browserAction.js:147:18
onManifestEntry@chrome://browser/content/ext-browserAction.js:127:5
async*asyncEmitManifestEntry@resource://gre/modules/ExtensionCommon.jsm:1177:14
async*runManifest@resource://gre/modules/Extension.jsm:1478:23
_startup@resource://gre/modules/Extension.jsm:1645:13
async*startup@resource://gre/modules/Extension.jsm:1585:27
startup@resource://gre/modules/Extension.jsm:1102:12
callBootstrapMethod@resource://gre/modules/addons/XPIProvider.jsm:4422:20
installAddonFromLocation@resource://gre/modules/addons/XPIProvider.jsm:3644:5
async*installTemporaryAddon@resource://gre/modules/addons/XPIProvider.jsm:3513:12
installTemporaryAddon@resource://gre/modules/AddonManager.jsm:2166:12
installTemporaryAddon@resource://gre/modules/AddonManager.jsm:3547:12
installAddon@resource://devtools/shared/base-loader.js -> resource://devtools/client/aboutdebugging/components/addons/Controls.js:80:5
loadAddonFromFile/<@resource://devtools/shared/base-loader.js -> resource://devtools/client/aboutdebugging/components/addons/Controls.js:70:7
Assignee

Updated

a year ago
Component: Untriaged → WebExtensions: Frontend
Product: Firefox → Toolkit
Assignee

Updated

a year ago
See Also: → 1359408

Comment 2

a year ago
mozreview-review
Comment on attachment 8959211 [details]
Bug 1446027 - Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm.

https://reviewboard.mozilla.org/r/228092/#review235540

LGTM assuming it passes existing resize tests.  But it might still be worth having Kris look at in case there is some reason for the prior sequence of the code.
Attachment #8959211 - Flags: review+
Assignee

Updated

a year ago
Attachment #8959211 - Flags: review?(kmaglione+bmo)
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][qf:f61][fxteam]

Comment 3

a year ago
perf?
Assignee: nobody → lgreco
Keywords: perf

Comment 4

a year ago
mozreview-review
Comment on attachment 8959211 [details]
Bug 1446027 - Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm.

https://reviewboard.mozilla.org/r/228092/#review250834

Sorry for the delay. I didn't notice this in my queue.

::: browser/components/extensions/ExtensionPopups.jsm:496
(Diff revision 1)
> +    this.browser.swapDocShells(browser);
> +    this.destroyBrowser(browser);
> +
> +    let popupRect;
> +
> +    await this.window.promiseDocumentFlushed(() => {

Please do something like `let flushPromise = this.window.promise...` before you call `createBrowser` and then await it afterwards. Inserting the browser will dirty the layout state, which means we're more likely to need to wait another refresh cycle before we can calculate the bounds.

::: browser/components/extensions/ExtensionPopups.jsm:508
(Diff revision 1)
> -    let win = this.window;
> +      let win = this.window;
> -    let popupBottom = win.mozInnerScreenY + popupRect.bottom;
> +      let popupBottom = win.mozInnerScreenY + popupRect.bottom;
> -    let popupTop = win.mozInnerScreenY + popupRect.top;
> +      let popupTop = win.mozInnerScreenY + popupRect.top;
>  
> -    let screenBottom = win.screen.availTop + win.screen.availHeight;
> +      let screenBottom = win.screen.availTop + win.screen.availHeight;
> -    this.extraHeight = {
> +      this.extraHeight = {
> -      bottom: Math.max(0, screenBottom - popupBottom),
> +        bottom: Math.max(0, screenBottom - popupBottom),
> -      top:  Math.max(0, popupTop - win.screen.availTop),
> +        top:  Math.max(0, popupTop - win.screen.availTop),
> -    };
> +      };

Please do these calculations inside the layout flush callback.
Attachment #8959211 - Flags: review?(kmaglione+bmo)

Comment 5

a year ago
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #4)
> Sorry for the delay. I didn't notice this in my queue.

Haha. Bug pun. (Sorry, couldn't resist!)
Comment hidden (mozreview-request)
Assignee

Comment 7

a year ago
mozreview-review-reply
Comment on attachment 8959211 [details]
Bug 1446027 - Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm.

https://reviewboard.mozilla.org/r/228092/#review250834

Thanks for the your review comments Kris, I've updated the patch with the suggested changes (and rebased on a recent mozilla-central tip).

Updated

a year ago
Whiteboard: [ohnoreflow][qf:p1][qf:f61][fxteam] → [ohnoreflow][qf:p1][qf:f64][fxteam]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:p1][qf:f64][fxteam] → [ohnoreflow][qf:p1:f64][fxteam]

Updated

11 months ago
Product: Toolkit → WebExtensions
If this is updated, can we get a r?
Flags: needinfo?(kmaglione+bmo)
Duplicate of this bug: 1483272
Priority: -- → P2
Setting this to P1 because it's been lingering and ... 64.
Priority: P2 → P1
Assignee

Comment 12

6 months ago
Attachment 9025745 [details] is the same patch from attachment 8959211 [details] (no actual changes, just rebased on a recent mozilla-central tip and re-submitted on phabricator, as mozreview has been decommissioned).
Flags: needinfo?(kmaglione+bmo)

Updated

4 months ago
Whiteboard: [ohnoreflow][qf:p1:f64][fxteam] → [ohnoreflow][qf:p3:responsiveness][fxteam]
Whiteboard: [ohnoreflow][qf:p3:responsiveness][fxteam] → [ohnoreflow][qf:p3:responsiveness][fxperf]

Hey rpl, I think kmag might be a bit swamped to review this. Any other candidates?

Flags: needinfo?(lgreco)
Whiteboard: [ohnoreflow][qf:p3:responsiveness][fxperf] → [ohnoreflow][qf:p3:responsiveness][fxperf:p3]
Assignee

Updated

4 months ago
Attachment #8959211 - Attachment is obsolete: true
Attachment #8959211 - Flags: review?(kmaglione+bmo)
Assignee

Comment 14

4 months ago

(In reply to Mike Conley (:mconley) (:⚙️) from comment #13)

Hey rpl, I think kmag might be a bit swamped to review this. Any other candidates?

Kris completed the review \o/

I've rebased the patch on a recent mozilla-central tip and I've been running browserAction tests on them one more time.

I noticed some new errors logged while running the browserAction popup tests with these changes applied and I'm looking into those.

The errors logged doesn't seem to make any of those tests to fail (e.g. all the browserAction tests have been running successfully in this push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a8ea2afb971865abfc0440b86e2f9658228cb55), but they are clearly related to these changes (due to a browserAction popup that is being quickly closed during tests, which is likely triggering these additional error because the popup is being destroyed while we are awaiting on the "document flushed promise").

Even if these errors are not triggering failures on the existing tests (as the popup is being closed in any case and so the error is not compromising the test run), I think that it would be better to look into them right now before landing these changes (and apply to the patch the additional changes needed to take this scenario into account).

Flags: needinfo?(lgreco)
Attachment #9040191 - Attachment is obsolete: true
Assignee

Comment 16

4 months ago

Comment on attachment 9040191 [details]
Bug 1446027 - Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm.

revision created by mistake, arc didn't recognized it as the updated version of attachment 9025745 [details], marking as obsolete.

Comment 17

3 months ago
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/4e0d6226ba5f
Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm. r=kmag

Comment 18

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Comment 19

2 months ago

Can you please add some STRs to this issue or mark it as "qe-verify- " if no manual testing is needed ?

Flags: needinfo?(lgreco)
Flags: needinfo?(lgreco) → qe-verify-
Assignee

Updated

27 days ago
See Also: → 1544206
You need to log in before you can comment on or make changes to this bug.