1359259 - 0.92ms uninterruptible reflow at PopupNotifications_showPanel@resource://gre/modules/PopupNotifications.jsm:932:9

Status: NEW

(Toolkit :: PopupNotifications and Notification Bars, defect, P4)

Description

[Hazel Ward]

Here's the stack:

PopupNotifications_showPanel@resource://gre/modules/PopupNotifications.jsm:932:9
PopupNotifications_update@resource://gre/modules/PopupNotifications.jsm:1097:9
anchorVisibilityChange@resource://gre/modules/PopupNotifications.jsm:579:7
PopupNotifications_locationChange@resource://gre/modules/PopupNotifications.jsm:560:7
onLocationChange@chrome://browser/content/browser.js:5084:7
callListeners@chrome://browser/content/tabbrowser.xml:492:24
_callProgressListeners@chrome://browser/content/tabbrowser.xml:513:13
_callProgressListeners@chrome://browser/content/tabbrowser.xml:563:22
onLocationChange@chrome://browser/content/tabbrowser.xml:845:17
_callProgressListeners@resource://gre/modules/RemoteWebProgress.jsm:179:11
receiveMessage@resource://gre/modules/RemoteWebProgress.jsm:265:7

Comment 1

[Florian Quèze [:florian]]

http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/toolkit/modules/PopupNotifications.jsm#931

    // If the anchor element is hidden or null, fall back to the identity icon.
    if (!anchorElement || (anchorElement.boxObject.height == 0 &&
                           anchorElement.boxObject.width == 0)) {

I hope this test would still work with a getBoundsWithoutFlushing call instead, but I'm not sure how to verify that there's no edge case this would break.

Comment 3

[Johann Hofmann [:johannh]]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> http://searchfox.org/mozilla-central/rev/
> baf47b352e873d4516d7656186d6d7c7447d3873/toolkit/modules/PopupNotifications.
> jsm#931
> 
>     // If the anchor element is hidden or null, fall back to the identity
> icon.
>     if (!anchorElement || (anchorElement.boxObject.height == 0 &&
>                            anchorElement.boxObject.width == 0)) {
> 
> I hope this test would still work with a getBoundsWithoutFlushing call
> instead, but I'm not sure how to verify that there's no edge case this would
> break.

FWIW I think this needs the flushing to check if the anchor elements are visible. Maybe there's another way to do it but I don't think we can simply substitute with getBoundsWithoutFlushing here.

Comment 5

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

I can still reproduce this by visiting a page that opens up a permission panel - example, GeoLocation.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

(not startup related: fxperf:p2 -> fxperf:p3)

Comment 7

[Florian Quèze [:florian]]

This code was changed significantly in bug 1815769, it now uses .checkVisibility() instead of getBoundingClientRect().

Gijs, do you know if checkVisibility flushes? If not, we can close this bug.

Comment 8

[Gregory Pappas [:gregp]]

Certainly not Gijs, but

• The checkVisibility implementation
  https://searchfox.org/mozilla-central/rev/a06d5a8871b1796f2dbd588ab518eaa98507e018/dom/base/Element.cpp#703-742
  • Uses nsIContent::GetPrimaryFrame, which can cause flushes
    https://searchfox.org/mozilla-central/rev/a06d5a8871b1796f2dbd588ab518eaa98507e018/dom/base/Element.cpp#253-275
    • However, checkVisibility has a ChromeOnly flush option to control this behavior
      https://searchfox.org/mozilla-central/rev/a06d5a8871b1796f2dbd588ab518eaa98507e018/dom/webidl/Element.webidl#207

Comment 9

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] from comment #7)

This code was changed significantly in bug 1815769, it now uses .checkVisibility() instead of getBoundingClientRect().

Gijs, do you know if checkVisibility flushes? If not, we can close this bug.

As Greg notes, this will currently flush.

I imagine that in some cases we'll want to flush here (or at least wait for a flush to have happened before trying to show the popupnotification). I imagine that disentangling this may prove somewhat fragile. Do you have ideas for how to design the API such that consumers don't have to worry about this but also we don't cause sync flushes? Maybe we can make showing these async and use the relevant async APIs like promiseDocumentFlushed to obtain visibility information for everything?

(note that there is a fun twist here that PopupNotifications just uses tabbrowser.selectedTab, which could potentially change while the code is running, if we make the code async)

Comment 10

[Florian Quèze [:florian]]

It's likely that the way to avoid the sync flush would be to wait for an async one, but if we are waiting for the result to display something, we might either introduce flicker, or delay showing the popup by at least one frame. I'm not sure either is desirable. So no obvious idea about what to do, I was mostly checking if we could close the bug; looks like the answer is no.