Closed Bug 1345387 Opened 3 years ago Closed 3 years ago

Label runnables in layout/painting/


(Core :: Web Painting, defect)

Not set





(Reporter: TYLin, Assigned: bmo)



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

User Story

See 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/.

(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:
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.

 - 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.

 - 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.

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

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
Blocks: 1347815
No longer blocks: 1347815
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.