1345387 - Label runnables in layout/painting/

Status: RESOLVED DUPLICATE of bug 1347815

(Core :: Web Painting, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8) (Away March 2 - 20)]

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

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

(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

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Astley Chen (inactive)]

Scanned and the only todo in this bug is LayerActivityTracker which is currently being addressed in bug 1345464.

Comment 5

[Astley Chen (inactive)]

Class LayerActivityTracker will be handled in bug 1347815.