Last Comment Bug 779971 - "ASSERTION: Must not call under nsISVGChildFrame::ReflowSVG"
: "ASSERTION: Must not call under nsISVGChildFrame::ReflowSVG"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Robert Longson
:
Mentors:
: 738555 (view as bug list)
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2012-08-02 13:22 PDT by Jesse Ruderman
Modified: 2012-11-10 08:18 PST (History)
4 users (show)
jwatt: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
testcase (416 bytes, image/svg+xml)
2012-08-02 13:22 PDT, Jesse Ruderman
no flags Details
stack traces (8.26 KB, text/plain)
2012-08-02 13:22 PDT, Jesse Ruderman
no flags Details
repeating portion of the crash stack (969 bytes, text/plain)
2012-10-21 09:01 PDT, Jesse Ruderman
no flags Details
patch (5.14 KB, patch)
2012-10-28 14:02 PDT, Robert Longson
no flags Details | Diff | Splinter Review
patch (8.21 KB, patch)
2012-11-02 15:37 PDT, Robert Longson
jwatt: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-08-02 13:22:22 PDT
Created attachment 648460 [details]
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 2012-08-02 13:22:37 PDT
Created attachment 648461 [details]
stack traces
Comment 2 Jesse Ruderman 2012-10-21 09:00:11 PDT
Now this crashes with too-much-recursion.
Comment 3 Jesse Ruderman 2012-10-21 09:01:30 PDT
Created attachment 673699 [details]
repeating portion of the crash stack
Comment 4 Robert Longson 2012-10-28 07:41:28 PDT
http://hg.mozilla.org/mozilla-central/rev/5947680feefb makes it an infinite loop (one of the patches fromm Bug 539356).
Comment 5 Robert Longson 2012-10-28 14:02:21 PDT
Created attachment 675985 [details] [diff] [review]
patch
Comment 6 Robert Longson 2012-10-28 14:49:57 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d6d2fcbae0ef
Comment 7 Jonathan Watt [:jwatt] 2012-10-30 10:23:29 PDT
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 2012-10-30 11:27:11 PDT
> 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 9 Robert Longson 2012-11-02 06:45:34 PDT
*** Bug 738555 has been marked as a duplicate of this bug. ***
Comment 10 Robert Longson 2012-11-02 15:37:04 PDT
Created attachment 677910 [details] [diff] [review]
patch

How about this instead then?
Comment 11 Jonathan Watt [:jwatt] 2012-11-06 11:06:26 PST
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 2012-11-06 11:15:19 PST
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] 2012-11-06 11:16:54 PST
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] 2012-11-06 11:17:33 PST
For future reference, just feel free to push such simple fixes yourself without r=
Comment 15 Jonathan Watt [:jwatt] 2012-11-07 01:53:00 PST
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] 2012-11-07 01:58:36 PST
As discussed on IRC, pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/99a2125bd365
Comment 17 Jonathan Watt [:jwatt] 2012-11-07 07:51:57 PST
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] 2012-11-07 07:55:24 PST
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] 2012-11-07 08:29:01 PST
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] 2012-11-07 09:33:15 PST
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] 2012-11-07 15:15:57 PST
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] 2012-11-07 17:32:48 PST
https://hg.mozilla.org/mozilla-central/rev/fd836a1f49c9
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-07 17:36:00 PST
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 2012-11-09 10:29:17 PST
Tracking and sending email - this needs to get landed on branches asap.
Comment 25 Daniel Holbert [:dholbert] 2012-11-09 11:47:06 PST
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] 2012-11-09 11:53:27 PST
(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] 2012-11-09 11:59:27 PST
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] 2012-11-09 12:00:41 PST
Flagging as needinfo for jwatt/longsonr for comment 25-26, regarding what to do about beta.
Comment 29 Daniel Holbert [:dholbert] 2012-11-09 12:57:14 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.