Open Bug 1359259 Opened 7 years ago Updated 2 months ago

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

Categories

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

defect

Tracking

()

Performance Impact low

People

(Reporter: rjward0, Unassigned)

References

Details

(Keywords: perf, perf:frontend, perf:responsiveness, Whiteboard: [ohnoreflow])

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
Flags: qe-verify?
Priority: -- → P2
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.
Component: Untriaged → Notifications and Alerts
Product: Firefox → Toolkit
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
(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.
Flags: qe-verify? → qe-verify-
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:p2][reserve-photon-performance]
Priority: P3 → P4
Keywords: perf
I can still reproduce this by visiting a page that opens up a permission panel - example, GeoLocation.
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf:p2]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p2] → [ohnoreflow][qf:f64][qf:p3][fxperf:p2]
Whiteboard: [ohnoreflow][qf:f64][qf:p3][fxperf:p2] → [ohnoreflow][qf:p3:f64][fxperf:p2]
Whiteboard: [ohnoreflow][qf:p3:f64][fxperf:p2] → [ohnoreflow][qf:p3][fxperf:p2]
(not startup related: fxperf:p2 -> fxperf:p3)
Whiteboard: [ohnoreflow][qf:p3][fxperf:p2] → [ohnoreflow][qf:p3][fxperf:p3]
Whiteboard: [ohnoreflow][qf:p3][fxperf:p3] → [ohnoreflow][qf:p3:responsiveness][fxperf:p3]
Performance Impact: --- → P3
Whiteboard: [ohnoreflow][qf:p3:responsiveness][fxperf:p3] → [ohnoreflow][fxperf:p3]
Severity: normal → S3
Component: Notifications and Alerts → PopupNotifications and Notification Bars

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.

Flags: needinfo?(gijskruitbosch+bugs)
Keywords: perf:frontend
Whiteboard: [ohnoreflow][fxperf:p3] → [ohnoreflow]

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

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(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.

Flags: needinfo?(florian)
You need to log in before you can comment on or make changes to this bug.