Closed Bug 1345387 Opened 3 years ago Closed 3 years ago

Label runnables in layout/painting/

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1347815

People

(Reporter: TYLin, Assigned: bmo)

References

Details

(Whiteboard: [QDL][TDC-MVP][LAYOUT])

User Story

See https://wiki.mozilla.org/Quantum/DOM#Labeling for the story.
According to the offline discussion with Bevis, LayerActivityTracker [1] (inheriting from nsExpirationTracker) dispatches high frequency runnables via [2]. So this should be labeled.

There might be more such cases in layout/painting/.

[1] http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/layout/painting/ActiveLayerTracker.cpp#118
[2] 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&regexp=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
Blocks: 1347815
No longer blocks: 1347815
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1347815
Class LayerActivityTracker will be handled in bug 1347815.
Whiteboard: [QDL][TDC-MVP][LAYOUT]
You need to log in before you can comment on or make changes to this bug.