779971 - "ASSERTION: Must not call under nsISVGChildFrame::ReflowSVG"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Must not call under nsISVGChildFrame::ReflowSVG!: '!OuterSVGIsCallingReflowSVG(aFrame)', file layout/svg/base/src/nsSVGUtils.cpp, line 787

###!!! ASSERTION: aDuringUpdate lies!: 'aDuringUpdate == OuterSVGIsCallingReflowSVG(aFrame)', file layout/svg/base/src/nsSVGUtils.cpp, line 612

###!!! ASSERTION: Must not InvalidateRenderingObservers() under nsISVGChildFrame::ReflowSVG!: '!OuterSVGIsCallingReflowSVG(aFrame)', file layout/svg/base/src/nsSVGUtils.cpp, line 624

###!!! ASSERTION: Do not call under nsISVGChildFrame::ReflowSVG!: '!OuterSVGIsCallingReflowSVG(aFrame)', file layout/svg/base/src/nsSVGUtils.cpp, line 720

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Jesse Ruderman]

Now this crashes with too-much-recursion.

Comment 3

[Jesse Ruderman]

Attachment: repeating portion of the crash stack

Comment 4

[Robert Longson [:longsonr]]

http://hg.mozilla.org/mozilla-central/rev/5947680feefb makes it an infinite loop (one of the patches fromm Bug 539356).

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch

Comment 6

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=d6d2fcbae0ef

Comment 7

[Jonathan Watt [:jwatt]]

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

>-    if (mFrame->PresContext()->PresShell()->IsReflowLocked()) {
>-      nsIReflowCallback* cb = new nsAsyncNotifyGlyphMetricsChange(mFrame);
>-      mFrame->PresContext()->PresShell()->PostReflowCallback(cb);
>-    } else {
>+    // Don't need to request ReflowFrame if we're being reflowed.
>+    if (!(mFrame->GetStateBits() & NS_FRAME_IN_REFLOW) &&
>+        !mFrame->PresContext()->PresShell()->IsReflowLocked()) {
>       nsSVGTextPathFrame* textPathFrame = static_cast<nsSVGTextPathFrame*>(mFrame);
>       textPathFrame->NotifyGlyphMetricsChange();
>     }

If |!mFrame->PresContext()->PresShell()->IsReflowLocked()|, then we know |!(mFrame->GetStateBits() & NS_FRAME_IN_REFLOW)| too. (I.e. the NS_FRAME_IN_REFLOW check is redundant.)

Essentially what this change does is make us do nothing if reflow is underway somewhere in the tree. The fact that an SVG effects property exists after a change tells us that we didn't invalidate it previously though, so not invalidating and reflowing for the change here means we're not going to do it at all, which is wrong.

I think the fundamental issue here is that our mechanism for handling invalidation and reflow for SVG effects is currently pretty messed up (especially after DLBI). I'm working on trying to get my head fully around that code to try and sort out the mess.

>-  if (invalidate) {
>-    // XXXSDL Let FinishAndStoreOverflow do this.
>-    nsSVGUtils::InvalidateBounds(this, true);
>-  }

Why?

>     if (textPath) {
>-      if (!textPath->GetPathFrame()) {
>-        // invalid text path, give up
>-        return;
>-      }

Why?

Comment 8

[Robert Longson [:longsonr]]

> Essentially what this change does is make us do nothing if reflow is
> underway somewhere in the tree. The fact that an SVG effects property exists
> after a change tells us that we didn't invalidate it previously though, so
> not invalidating and reflowing for the change here means we're not going to
> do it at all, which is wrong.

The testcase is an infinite loop otherwise though :-( I'll look into why the effects property exists.

> 
> I think the fundamental issue here is that our mechanism for handling
> invalidation and reflow for SVG effects is currently pretty messed up
> (especially after DLBI). I'm working on trying to get my head fully around
> that code to try and sort out the mess.
> 
> >-  if (invalidate) {
> >-    // XXXSDL Let FinishAndStoreOverflow do this.
> >-    nsSVGUtils::InvalidateBounds(this, true);
> >-  }
> 
> Why?

Call to parent (nsSVGDisplayContainer) does this too so currently we're calling InvalidateBounds twice.

> 
> >     if (textPath) {
> >-      if (!textPath->GetPathFrame()) {
> >-        // invalid text path, give up
> >-        return;
> >-      }
> 
> Why?

If we have <text><textPath href="invalid"/><tspan>xxx</tspan></text> we won't determine where the xxx is or draw it because we'll skip out at the textPath failure.

Comment 10

[Robert Longson [:longsonr]]

Attachment: patch

How about this instead then?

Comment 11

[Jonathan Watt [:jwatt]]

I pulled the two unrelated changes out and pushed them separately.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2db705ff2ec3
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca190f83001

Comment 12

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2db705ff2ec3 is completely wrong. The marker update function now says filter and the filter code says marker.

Comment 13

[Jonathan Watt [:jwatt]]

Yeah, just noticed that after failing to get an expected merge conflict applying your patch. Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/575a66ec14c7

Comment 14

[Jonathan Watt [:jwatt]]

For future reference, just feel free to push such simple fixes yourself without r=

Comment 15

[Jonathan Watt [:jwatt]]

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

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

::: layout/base/nsCSSFrameConstructor.cpp
@@ +7788,5 @@
> +          needInvalidatingPaint = true;
> +          // Invalidate and update our area:
> +          nsSVGTextPathFrame* textPathFrame =
> +            static_cast<nsSVGTextPathFrame*>(aFrame);
> +          textPathFrame->NotifyGlyphMetricsChange();

Since you only use textPathFrame once just make this:

static_cast<nsSVGTextPathFrame*>(aFrame)->NotifyGlyphMetricsChange();

@@ +8205,5 @@
>          nsSVGEffects::UpdateEffects(frame);
>        }
> +      if (hint & nsChangeHint_UpdateTextPath) {
> +        frame->Properties().Delete(nsSVGEffects::HrefProperty());
> +      }

This is wrong - just drop this part. nsChangeHint_UpdateTextPath comes from a DoUpdate() call, meaning from a change to the referenced content. Properties only need to be deleted when the _referencing_ element has changed.

Comment 16

[Jonathan Watt [:jwatt]]

As discussed on IRC, pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/99a2125bd365

Comment 17

[Jonathan Watt [:jwatt]]

Ed backed this out due to it causing a new intermittent orange in reftests/svg/smil/anim-pathLength-01.svg:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb51f488b03

I've been digging into what's causing that (no, it's not the dropping of the |Properties().Delete| bit of the original patch) and should have a patch soon.

Comment 18

[Jonathan Watt [:jwatt]]

The issue is that in ApplyRenderingChangeToTree there's a check for |shell->IsPaintingSuppressed()| that removes the nsChangeHint_RepaintFrame hint if that's true. In this case when it goes on to call DoApplyRenderingChangeToTree we can't reach our new nsChangeHint_UpdateTextPath handling code because that's inside a check for nsChangeHint_RepaintFrame.

Comment 19

[Jonathan Watt [:jwatt]]

I'm not sure exactly why painting is suspended during restyle processing for anim-pathLength-01.svg, but it seems to be something to do with the way that it calls setTimeAndSnapshot onload, which puts that into a race with first reflow I guess. If the setCurrentTime from setTimeAndSnapshot is separated out from the pauseAnimations call to have setCurrentTime called some time after load there is no problem.

Anyway, even if painting is suppressed, we still want to do the reflow for nsChangeHint_UpdateTextPath so that needs to be separated out from nsChangeHint_RepaintFrame. Testing a patch just now.

Comment 20

[Jonathan Watt [:jwatt]]

Pushed a tweaked version of Robert's patch with an adjustment taking account of the info in the previous three comments:

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

Comment 21

[Jonathan Watt [:jwatt]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: bad things
Testing completed (on m-c, etc.): landed on m-i and seems to have passed tests ok
Risk to taking this patch (and alternatives if risky): less risk than not taking it
String or UUID changes made by this patch: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd836a1f49c9

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

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

since this passed the inbound-> central gauntlet, approving for branch uplift.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking and sending email - this needs to get landed on branches asap.

Comment 25

[Daniel Holbert [:dholbert]]

I'll land this on Aurora (Firefox 18) today, but I can't land it on beta, because the m-c cset has non-trivial merge-conflicts when applied to beta.

In particular, the nsSVGEffects.cpp chunk removes/reworks code that was added in bug 539356, and that bug's patch only landed back to Firefox 18 (currently aurora), not 17 (currently beta). 

It might be that we can just rip out the old nsSVGTextPathProperty::DoUpdate() impl on beta and swap in the new one, but I don't want to just assume that -- longsonr or jwatt are better equipped to make that call, as the patch-author / reviewer.

Comment 26

[Daniel Holbert [:dholbert]]

(Alternately: do we know that this is reproducible on beta & the patch is actually needed there?  If the patched chunk from bug 539356 is necessary for triggering the bug, then maybe not...?)

Comment 27

[Daniel Holbert [:dholbert]]

Landed on aurora:
 https://hg.mozilla.org/releases/mozilla-aurora/rev/b6c41959679d

(fixed bitrot in nsChangeHint by using the next-available bit on that branch's version of the file -- 0x4000 instead of 0x10000.)

Comment 28

[Daniel Holbert [:dholbert]]

Flagging as needinfo for jwatt/longsonr for comment 25-26, regarding what to do about beta.

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> (Alternately: do we know that this is reproducible on beta

Just to follow up on this point: I do hit all of the assertions from comment 0 in an up-to-date beta debug build, when I load this bug's testcase, so this is reproducible there. Setting "status-firefox17" to "affected" to make that explicit.

Comment 30

[Robert Longson [:longsonr]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cfe771ad41d4