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

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1446027 - Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm.

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

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.

Comment 3

[Worcester12345]

perf?

Comment 4

[Kris Maglione [:kmag]]

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.

Comment 5

[Worcester12345]

(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 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

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

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

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).

Comment 8

[David Durst [:ddurst]]

If this is updated, can we get a r?

Comment 10

[David Durst [:ddurst]]

Setting this to P1 because it's been lingering and ... 64.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1446027 - Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm.

MozReview-Commit-ID: Kjmn67ZfNWd

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

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).

Comment 13

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

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

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

(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).

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1446027 - Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e0d6226ba5f
Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm. r=kmag

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4e0d6226ba5f

Comment 19

[Madalin Cotetiu]

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