Closed Bug 1345387 Opened 3 years ago Closed 3 years ago
Label runnables in layout/painting/
See https://wiki.mozilla.org/Quantum/DOM#Labeling for the story.
According to the offline discussion with Bevis, LayerActivityTracker  (inheriting from nsExpirationTracker) dispatches high frequency runnables via . So this should be labeled. There might be more such cases in layout/painting/.  http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/layout/painting/ActiveLayerTracker.cpp#118  http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/xpcom/ds/nsExpirationTracker.h#363-364
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #0) > There might be more such cases in layout/painting/. I just found more use cases of nsExpirationTracker under layout: http://searchfox.org/mozilla-central/search?q=nsExpirationTracker&case=false®exp=false&path=layout
Replying to billm from bug 1345464 comment 3: > Looking at LayerActivityTracker, I'm not sure if it's safe to label it as > SystemGroup. > > Daniel, suppose that we were in the middle of a reflow (maybe at the top of > nsBlockFrame::Reflow) and we interrupted it and started running > LayerActivityTracker::NotifyExpired. Would that cause problems? I'm mostly > worried about the SchedulePaint call, which seems to do some SVG stuff. (I think the SVG stuff you're talking about is nsSVGEffects::InvalidateDirectRenderingObservers.) I'm actually not familiar with LayerActivityTracker -- mattwoodrow is perhaps a better authority on that, but here are my observations: - LayerActivityTracker::NotifyExpired calls f->SchedulePaint() on a nsIFrame. And several stacklevels down, via SchedulePaintInternal, that ends up calling nsIPresShell::SetNeedLayoutFlush(). So: this could make us mark our document as needing a layout-flush, while we're flushing layout. That's kinda confusing, but maybe not bad. http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/layout/base/PresShell.cpp#3730 - That same SchedulePaint() function also calls InvalidateRenderingObservers, which deletes an entry from frame property-tables (nsSVGUtils::ObjectBoundingBoxProperty), though we don't seem to care about that frame-property during layout, so it's probably fine. It looks like it's just the cached return value of a DOM getter. http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/layout/svg/nsSVGEffects.cpp#888 - InvalidateRenderingObservers also sets a frame state bit, NS_FRAME_UPDATE_LAYER_TREE, but I don't think we care about that bit during layout, so I think it'd be OK. http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/layout/generic/nsFrame.cpp#6233 - LayerActivityTracker::NotifyExpired itself deletes a frame property-table entry and toggles an associated frame state bit, but I don't think we care about those during layout. (NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY and LayerActivityProperty()) http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/layout/painting/ActiveLayerTracker.cpp#180-183 So: the scenario that Bill describes *might* be OK -- I haven't found anything definitely-not-OK yet. But I'm not 100% sure, so (if it's not too copmlex) it might be better to treat it as not-OK to avoid introducing some fragility here. mattwoodrow can perhaps offer a more concrete OK/Not-OK call here, too - CC'ing him.
Scanned and the only todo in this bug is LayerActivityTracker which is currently being addressed in bug 1345464.
Assignee: nobody → aschen
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1347815
Class LayerActivityTracker will be handled in bug 1347815.
You need to log in before you can comment on or make changes to this bug.