Closed Bug 1446027 Opened 2 years ago Closed 1 year ago

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

Categories

(WebExtensions :: Frontend, defect, P1)

defect

Tracking

(firefox67 fixed)

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Keywords: perf, Whiteboard: [ohnoreflow][qf:p3:responsiveness][fxperf:p3])

Attachments

(1 file, 2 obsolete files)

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
Component: Untriaged → WebExtensions: Frontend
Product: Firefox → Toolkit
See Also: → 1359408
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+
Attachment #8959211 - Flags: review?(kmaglione+bmo)
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][qf:f61][fxteam]
perf?
Assignee: nobody → lgreco
Keywords: perf
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)
(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 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).
Whiteboard: [ohnoreflow][qf:p1][qf:f61][fxteam] → [ohnoreflow][qf:p1][qf:f64][fxteam]
Whiteboard: [ohnoreflow][qf:p1][qf:f64][fxteam] → [ohnoreflow][qf:p1:f64][fxteam]
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
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)
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]
Attachment #8959211 - Attachment is obsolete: true
Attachment #8959211 - Flags: review?(kmaglione+bmo)

(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

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.

Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/4e0d6226ba5f
Prevents synchronous uninterruptible reflow from ExtensionPopups.jsm. r=kmag
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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-
See Also: → 1544206
You need to log in before you can comment on or make changes to this bug.