0.92ms uninterruptible reflow at PopupNotifications_showPanel@resource://gre/modules/PopupNotifications.jsm:932:9
Categories
(Toolkit :: PopupNotifications and Notification Bars, defect, P4)
Tracking
()
Performance Impact | low |
People
(Reporter: rjward0, Unassigned, NeedInfo)
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
Updated•7 years ago
|
Comment 1•7 years ago
|
||
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.
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Comment 3•7 years ago
|
||
(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.
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Comment 5•7 years ago
|
||
I can still reproduce this by visiting a page that opens up a permission panel - example, GeoLocation.
Updated•7 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 6•5 years ago
|
||
(not startup related: fxperf:p2 -> fxperf:p3)
Updated•5 years ago
|
Updated•5 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•10 months ago
|
Comment 7•1 month ago
|
||
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•1 month ago
|
||
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
- However, checkVisibility has a ChromeOnly
- Uses nsIContent::GetPrimaryFrame, which can cause flushes
Comment 9•27 days ago
|
||
(In reply to Florian Quèze [:florian] from comment #7)
This code was changed significantly in bug 1815769, it now uses
.checkVisibility()
instead ofgetBoundingClientRect()
.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)
Description
•