839958 - Prevent infinite repainting of SVG that references a mask/clipPath/pattern that contains SVG text (then re-enable pattern-content.svg)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

Three reftests for the new SVG text support are currently disabled: mask-content.svg, clipPath-content.svg and pattern-content.svg in layout/reftests/svg/text/.  They all exhibit an infinite invalidation loop.

Comment 1

[Cameron McCormack (:heycam)]

Jonathan, if you want to see the problem that I'm having here, uncomment mask-content.svg from layout/reftests/svg/text/reftest.list, and stick a

  printf("paint\n");

at the top of nsSVGTextFrame2::PaintSVG.  Running that reftest then with

  ./mach reftest layout/reftests/svg/reftest.list --filter mask-content

will output

  paint
  paint
  paint
  ...

forever.  I haven't begun to look into the problem more deeply (again), but will do so soon.

Comment 2

[Robert Longson [:longsonr]]

Attachment: WIP

The pattern/mask/clipPath calls nsSVGTextFrame2::PaintSVG which calls UpdateGlyphPositioning which reflows the text, that causes an invalidate which via the effects mechanism makes all the observers rerender and we're back to step 1.

This patch stops the loop but those 3 reftests fail. The geometricPrecision change to the pattern test helps a bit. Note that the pattern test fails in Opera too.

Not sure what's going on with the other 2 tests.

Should the tests be adjusted Cameron or is there still some other code change required?

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #2)
> The pattern/mask/clipPath calls nsSVGTextFrame2::PaintSVG which calls
> UpdateGlyphPositioning which reflows the text, that causes an invalidate

Can you expand on this? Where is this invalidate call, and what is the stack of the invalidate call (starting at PaintSVG)?

Comment 4

[Jonathan Watt [:jwatt]]

To be clear, I don't think we should be invalidating at this time. For non-display SVG text we should only call nsSVGEffects::InvalidateRenderingObservers for changes that occur to the non-display <text> or its descendants. We should not be calling it when we're laying out the text for the context of a specific resource consumer. Hence why I'd like to know more about this invalidate call that is causing the infinite loop.

Comment 5

[Robert Longson [:longsonr]]

nsSVGPatternFrame::PaintPattern
nsSVGTextFrame2::PaintSVG
nsSVGTextFrame2::UpdateGlyphPositioning (because we're NONDISPLAY_CHILD)
nsSVGTextFrame2::DoReflow
kid->Reflow
...
nsFrame:InvalidateInternal
...
nsSVGEffects::InvalidateViaReferencedElement
nsSVGEffects::InvalidateRenderingObservers
...
nsSVGPatternFrame::PaintPattern

Comment 6

[Robert Longson [:longsonr]]

There's an async bit so we don't run out of stack but this is what's happening.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: pseudo-patch to stop InvalidateFrame calling InvalidateDirectRenderingObservers when it shouldn't

The stack that seems interesting is:

  nsSVGUtils::InvalidateBounds
  nsSVGPaintingProperty::DoUpdate
  nsSVGRenderingObserver::InvalidateViaReferencedElement
  nsSVGRenderingObserverList::InvalidateAll
  nsSVGEffects::InvalidateDirectRenderingObservers
  nsSVGEffects::InvalidateDirectRenderingObservers
  InvalidateFrameInternal
  nsIFrame::InvalidateFrame
  nsTextFrame::InvalidateFrame
  nsTextFrame::ReflowText
  nsLineLayout::ReflowFrame
  nsBlockFrame::ReflowInlineFrame
  nsBlockFrame::DoReflowInlineFrames
  nsBlockFrame::ReflowInlineFrames
  nsBlockFrame::ReflowLine
  nsBlockFrame::ReflowDirtyLines
  nsBlockFrame::Reflow
  nsSVGTextFrame2::DoReflow
  nsSVGTextFrame2::UpdateGlyphPositioning
  nsSVGTextFrame2::PaintSVG
  nsSVGUtils::PaintFrameWithEffects
  nsSVGDisplayContainerFrame::PaintSVG
  nsSVGUtils::PaintFrameWithEffects
  nsSVGPatternFrame::PaintPattern
  nsSVGPatternFrame::GetPaintServerPattern
  nsSVGPaintServerFrame::SetupPaintServer
  nsSVGUtils::SetupCairoFillPaint
  nsSVGPathGeometryFrame::Render
  nsSVGPathGeometryFrame::PaintSVG
  nsDisplaySVGPathGeometry::Paint

The nsSVGUtils::InvalidateBounds is invalidating the same nsSVGPathGeometryFrame that is being painted at the bottom of the stack, causing the never ending repainting.

Really we don't want nsTextFrame::ReflowText to be calling InvalidateFrame() on descendants of non-display SVG, since we know that SVG resources don't paint directly, and that reflowing of SVG resources is only done at paint time in order to paint the resource in one particular consumer's context. Then again I guess we generally do want rendering observers of resources to be invalidated if something changes in the resource that would require the changed part of the resource to reflow if it was a normal part of the document. So I guess a better course of action would be to make nsIFrame::InvalidateFrame a no-op except for invalidating rendering observers if the frame is part of an SVG resource, and not even invalidate rendering observers if the call is made while we're painting. Something along the lines of the pseudo-patch I'm attaching now, perhaps.

I'm not sure exactly what the "under-a-paint-call" part of the pseudo-patch should be. Maybe I should add a flag to PresShell that is set while we're under a PresShell::Paint call?

Comment 8

[Jonathan Watt [:jwatt]]

(longsonr, regarding your WIP patch, it sounds from bug 864000 like we should be doing that, but as an early return for perf reasons and not as a loop-breaker. So I think that should be done as a separate bug, and not as the solution to this bug. Probably it should also land after any fix for this bug in order that it doesn't mask the real fix for this issue.)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't nsSVGTextFrame2::UpdateGlyphPositioning call nsSVGTextFrame2::DoReflow only once, so that subsequent UpdateGlyphPositioning calls find that the frame subtree isn't dirty and we don't need to do any more reflows, thus stopping the endless invalidation?

Comment 10

[Cameron McCormack (:heycam)]

With longsonr's patch, we correctly only call nsSVGTextFrame2::DoReflow once, stopping the endless invalidation.  But we do still paint the <text> (on to the pattern surface) one too many times, due to the additional invalidation that occurs as a result of the DoReflow call.

Comment 11

[Cameron McCormack (:heycam)]

I think it makes sense that if we are reflowing the <text> in a <pattern> because we're painting a referencing element (the rendering observer) that uses that pattern, then we shouldn't invalidate that referencing element.

Inside nsSVGPatternFrame::PaintPattern, we could temporarily disable aSource from receiving invalidations from InvalidateDirectRenderingObservers.  Similarly for aParent in nsSVGMaskFrame::ComputeMaskAlpha.  Would this work?

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 739933 [details] [diff] [review]
WIP

The code changes are fine.  I'll play around with the pattern example to come up with something that doesn't have slight rendering differences.

The clipPath-content example shows a bug, though.  We're not reflowing the second <text> element when the Ahem font becomes available.  That's why the line-through decoration shows up too short.

Comment 13

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #12)
> The clipPath-content example shows a bug, though.  We're not reflowing the
> second <text> element when the Ahem font becomes available.  That's why the
> line-through decoration shows up too short.

I'll look at that in bug 865958.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: WIP 2

(In reply to Cameron McCormack (:heycam) from comment #12)
> The code changes are fine.

Hold up.  I think this might be over-eagerly preventing reflow, causing bug 865958.  There are cases where our frames will be marked dirty, but mPositioningDirty is still false: for example, when a <tspan> is restyled.  In that case, we don't want UpdateGlyphPositioning() to return early.  I think instead we should be doing this.  WDYT?

Comment 15

[Cameron McCormack (:heycam)]

Attachment: patch to prevent continuous invalidation

How about this, for comment 11?

Comment 16

[Robert Longson [:longsonr]]

Comment on attachment 742177 [details] [diff] [review]
WIP 2

>   if (needsReflow) {
>     nsPresContext::InterruptPreventer noInterrupts(PresContext());
>     DoReflow(aForceGlobalTransform);
>   }
> 
>-  DoGlyphPositioning();
>+  if (mPositioningDirty || needsReflow)
>+    DoGlyphPositioning();
> }

We can make this just mPositioningDirty if we keep the lines below.

> 
> void
> nsSVGTextFrame2::DoReflow(bool aForceGlobalTransform)
> {
>-  mPositioningDirty = true;
>-

i.e. keep this. After all if we do need to reflow then the positioning does need recalculating.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Cameron McCormack (:heycam) from comment #11)
> I think it makes sense that if we are reflowing the <text> in a <pattern>
> because we're painting a referencing element (the rendering observer) that
> uses that pattern, then we shouldn't invalidate that referencing element.
> 
> Inside nsSVGPatternFrame::PaintPattern, we could temporarily disable aSource
> from receiving invalidations from InvalidateDirectRenderingObservers. 
> Similarly for aParent in nsSVGMaskFrame::ComputeMaskAlpha.  Would this work?

I think so, but I'm skeptical that it's worth doing.

Comment 18

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> I think so, but I'm skeptical that it's worth doing.

It turns out that with my attachment 742177 [details] [diff] [review] patch to correctly handle mPositioningDirty, we do still land in this indefinite invalidation cycle.  So I think we still will need to do this or something like Jonathan's suggestion.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Cameron McCormack (:heycam) from comment #18)
> It turns out that with my attachment 742177 [details] [diff] [review] patch
> to correctly handle mPositioningDirty, we do still land in this indefinite
> invalidation cycle.

What is causing this cycle?

Comment 20

[Cameron McCormack (:heycam)]

Hmm.  So I think it might be this:

* our nsSVGTextFrame2 is initially dirty
* in DoReflow(), we copy NS_FRAME_IS_DIRTY onto the child anonymous block frame
* we reflow it, in the process causing the unnecessary invalidation on the <rect> that uses the pattern
* after this, we don't clear NS_FRAME_IS_DIRTY off the nsSVGTextFrame2 -- this would only otherwise happen in ReflowSVG(), but we don't call ReflowSVG() for non-display SVG elements
* the next time around painting due to the unnecessary invalidation, we find the nsSVGTextFrame2 is still NS_FRAME_IS_DIRTY, and reflow the anonymous block child unnecessarily

I stuck a

  mState &= ~NS_FRAME_IS_SVG_TEXT;

at the top of DoReflow(), and that breaks the cycle.

This doesn't alleviate the initial double invalidation, though.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Cameron McCormack (:heycam) from comment #20)
> I stuck a
> 
>   mState &= ~NS_FRAME_IS_SVG_TEXT;

I hope you mean ~NS_FRAME_IS_DIRTY!

> This doesn't alleviate the initial double invalidation, though.

I'm just not very worried about that :-).

Comment 22

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> I hope you mean ~NS_FRAME_IS_DIRTY!

Yes -- muscle memory. :)

> > This doesn't alleviate the initial double invalidation, though.
> 
> I'm just not very worried about that :-).

It is double painting as well as invalidation, but OK.

Comment 23

[Jonathan Watt [:jwatt]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Shouldn't nsSVGTextFrame2::UpdateGlyphPositioning call
> nsSVGTextFrame2::DoReflow only once, so that subsequent
> UpdateGlyphPositioning calls find that the frame subtree isn't dirty and we
> don't need to do any more reflows, thus stopping the endless invalidation?

As far as I can tell, calling Reflow on the anonymous block frame should always result in it and its descendants getting the same positioning values regardless of the context of the element that is referencing the pattern/etc. (In other words, we don't need to reflow the anonymous block frame each time we paint a referencing element that is painted during a given paint cycle (or even wait until we have a referencing element to do the reflow).) So yes, I agree that nsSVGTextFrame2::UpdateGlyphPositioning should only call nsSVGTextFrame2::DoReflow once. I'm not sure if we can do that by using the NS_SUBTREE_DIRTY() macro on the anonymous block child (given that we generally don't remove the dirty bits from non-display SVG), but that would be ideal.

We still don't want to make the InvalidateDirectRenderingObservers() call during the first reflow though, since that will mean the first referencing element to paint will be painted twice.

(In reply to Cameron McCormack (:heycam) from comment #11)
> I think it makes sense that if we are reflowing the <text> in a <pattern>
> because we're painting a referencing element (the rendering observer) that
> uses that pattern, then we shouldn't invalidate that referencing element.

Or _any_ other elements that reference the pattern.

> Inside nsSVGPatternFrame::PaintPattern, we could temporarily disable aSource
> from receiving invalidations from InvalidateDirectRenderingObservers. 
> Similarly for aParent in nsSVGMaskFrame::ComputeMaskAlpha.  Would this work?

Not in the case of multiple elements referencing the same pattern. If element A paints, then B, then when B paints it would invalidate A. Next time A paints it would invalidate B. Then when B paints it would invalidate A...and so on.

So I don't think the patches to add an NS_FRAME_PAINTING_WITH_EFFECT bit to the frame that's currently being painted are sufficient. I think this really would need to be a per-doc/per-presShell flag.

Can you start with roc's suggestion to make sure nsSVGTextFrame2::DoReflow isn't called when it doesn't need to be? I'd suggest you start by making it a no-op in the case that NS_SUBTREE_DIRTY(kid) (that's the kid, not the frame itself!) is false, and see how well that flies. The aForceGlobalTransform headache may make it a bit less simple than that though.

Comment 24

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #20)
> Hmm.  So I think it might be this:
> 
> * our nsSVGTextFrame2 is initially dirty
> * in DoReflow(), we copy NS_FRAME_IS_DIRTY onto the child anonymous block
> frame

Why do we do that? Much better would be to make sure that whatever change requires it to be dirty is sure to do the dirtying. I suspect we are probably over invalidating due to copying NS_FRAME_IS_DIRTY onto the anon kid.

> * we reflow it, in the process causing the unnecessary invalidation on the
> <rect> that uses the pattern
> * after this, we don't clear NS_FRAME_IS_DIRTY off the nsSVGTextFrame2 --
> this would only otherwise happen in ReflowSVG(), but we don't call
> ReflowSVG() for non-display SVG elements

I think we should leave the NS_FRAME_IS_DIRTY on the non-display nsSVGTextFrame2, but allow DidReflow() to remove it from the anon kid. (That happens, right?)

> * the next time around painting due to the unnecessary invalidation, we find
> the nsSVGTextFrame2 is still NS_FRAME_IS_DIRTY, and reflow the anonymous
> block child unnecessarily

We should do the reflow based on whether the _kid_ is dirty, I think.

> This doesn't alleviate the initial double invalidation, though.

I'd suggest ignoring the initial double invalidation for now.

Comment 25

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #24)
> > * in DoReflow(), we copy NS_FRAME_IS_DIRTY onto the child anonymous block
> > frame
> 
> Why do we do that? Much better would be to make sure that whatever change
> requires it to be dirty is sure to do the dirtying. I suspect we are
> probably over invalidating due to copying NS_FRAME_IS_DIRTY onto the anon
> kid.

It's to handle style changes on the nsSVGTextFrame2 that would need a reflow.  For example this:

  <g font-size="16px"><text>ab<tspan>cd</tspan></text></g>
  <script>
    g.setAttribute("font-size", "24px");
  </script>

ends up setting NS_FRAME_IS_DIRTY on the nsSVGTextFrame2 in nsSVGDisplayContainerFrame::ReflowSVG.  This must not be sufficient for the anonymous block frame to reflow correctly.

Another similar case is this:

  <text font-size="16px">abc</text>
  <script>
    text.setAttribute("font-size", "24px");
  </script>


I am not sure about the over-invalidation here: what situations are there where the nsSVGTextFrame2 has been marked dirty but the anonymous block shouldn't be?

> > * we reflow it, in the process causing the unnecessary invalidation on the
> > <rect> that uses the pattern
> > * after this, we don't clear NS_FRAME_IS_DIRTY off the nsSVGTextFrame2 --
> > this would only otherwise happen in ReflowSVG(), but we don't call
> > ReflowSVG() for non-display SVG elements
> 
> I think we should leave the NS_FRAME_IS_DIRTY on the non-display
> nsSVGTextFrame2, but allow DidReflow() to remove it from the anon kid. (That
> happens, right?)

I believe so.

> > * the next time around painting due to the unnecessary invalidation, we find
> > the nsSVGTextFrame2 is still NS_FRAME_IS_DIRTY, and reflow the anonymous
> > block child unnecessarily
> 
> We should do the reflow based on whether the _kid_ is dirty, I think.

In this case, we need to make sure all places where we mark the nsSVGTextFrame2 as dirty that we also mark the anonymous kid dirty.  I'm not sure what other cases there are, apart from the one in nsSVGDisplayContainerFrame::ReflowSVG.

Comment 26

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #23)
> Can you start with roc's suggestion to make sure nsSVGTextFrame2::DoReflow
> isn't called when it doesn't need to be? I'd suggest you start by making it
> a no-op in the case that NS_SUBTREE_DIRTY(kid) (that's the kid, not the
> frame itself!) is false, and see how well that flies. The
> aForceGlobalTransform headache may make it a bit less simple than that
> though.

I think only making this change won't work, for the reasons above re setting NS_FRAME_IS_DIRTY inside nsSVGDisplayContainerFrame::ReflowSVG.  I think the two possible solutions are:

(a) leave the NS_FRAME_IS_DIRTY bit copying on to the kid, but make sure we clear that bit on the nsSVGTextFrame2 after we reflow the kid

(b) ensure that whenever we set NS_FRAME_IS_DIRTY on nsSVGTextFrame2 -- be it in PresShell::FrameNeedsReflow, nsSVGDisplayContainerFrame::ReflowSVG, or whereever else -- that we also set it on the anonymous kid, then change UpdateGlyphPositioning to only look NS_SUBTREE_DIRTY(kid)

The former seems simpler.

Comment 27

[Cameron McCormack (:heycam)]

Attachment: patch

So I think we should still just do this.

Comment 28

[Jonathan Watt [:jwatt]]

Comment on attachment 744993 [details] [diff] [review]
patch

Review of attachment 744993 [details] [diff] [review]:
-----------------------------------------------------------------

r=jwatt with the following addressed as we discussed. I'll file follow-ups tomorrow for the other issues that we discussed and those I raised above.

::: layout/svg/nsSVGTextFrame2.cpp
@@ +4791,5 @@
>  
>    if (needsReflow) {
>      nsPresContext::InterruptPreventer noInterrupts(PresContext());
>      DoReflow(aForceGlobalTransform);
>    }

Please make this:

  if (mState & (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)) {
    if (mState & NS_FRAME_IS_DIRTY) {
      // If we require a full reflow, ensure our kid is marked fully dirty.
      // (Note that our anonymous nsBlockFrame is not an nsISVGChildFrame, so
      // even when we are called via our ReflowSVG this will not be done for us
      // by nsSVGDisplayContainerFrame::ReflowSVG.)
      kid->AddStateBits(NS_FRAME_IS_DIRTY);
    }
    nsPresContext::InterruptPreventer noInterrupts(PresContext());
    DoReflow(aForceGlobalTransform);
  }

(And then remove the redundant code above.)

@@ +4812,5 @@
> +  // UpdateGlyphPositioning(), then we need to clear the flag from this
> +  // frame so that we don't necessarily reflow it again the next time we
> +  // call UpdateGlyphPositioning().  Normally, this flag would be cleared
> +  // in ReflowSVG(), but we don't call that for non-display frames.
> +  mState &= ~NS_FRAME_IS_DIRTY;

Remove both dirty bits.

And please make this something like:

  if (mState & NS_STATE_SVG_NONDISPLAY_CHILD) {
  	// Normally, this flag would be cleared in ReflowSVG(), but that doesn't
  	// get called for non-display frames. We don't want to reflow our
  	// descendants every time nsSVGTextFrame2::PaintSVG makes sure that we have
  	// valid positions by calling UpdateGlyphPositioning(), so we need to clear
  	// these dirty bits. Note that this also breaks an invalidation loop where
  	// our descendants invalidate as they reflow, which invalidates rendering
  	// observers, which reschedules the frame that is currently painting by
  	// referencing us to paint again. See bug 839958 comment 7. Hopefully we
  	// will break that loop more convincingly at some point.
  	mState &= ~(NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN);
  }

Comment 29

[Cameron McCormack (:heycam)]

There is an invalidation problem (but not a loop) with mask-content.svg and clipPath-content.svg, so I'll land this without re-enabling those two tests and deal with that in another bug.

Comment 30

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #29)
> There is an invalidation problem (but not a loop) with mask-content.svg and
> clipPath-content.svg, so I'll land this without re-enabling those two tests
> and deal with that in another bug.

That's bug 873806.

Comment 31

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d00f9ead1848

Comment 32

[Cameron McCormack (:heycam)]

Backed out for Windows reftest failures on pattern-content.svg:

https://hg.mozilla.org/integration/mozilla-inbound/rev/82a243758a1d

Comment 33

[Cameron McCormack (:heycam)]

Relanded with test tweak:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d03196559835

Comment 34

[Cameron McCormack (:heycam)]

And a followup since I didn't write the fuzzy-if() correctly:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0d43b4533d

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d03196559835
https://hg.mozilla.org/mozilla-central/rev/ec0d43b4533d