The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 17

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: Robert Longson)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla19
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed, firefox18+ fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Created attachment 648461 [details]
stack traces
(Reporter)

Comment 2

5 years ago
Now this crashes with too-much-recursion.
(Reporter)

Comment 3

5 years ago
Created attachment 673699 [details]
repeating portion of the crash stack
(Assignee)

Comment 4

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

Comment 5

5 years ago
Created attachment 675985 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #675985 - Flags: review?(jwatt)
(Assignee)

Comment 6

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d6d2fcbae0ef
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?
(Assignee)

Comment 8

5 years ago
> 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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 738555
(Assignee)

Comment 10

5 years ago
Created attachment 677910 [details] [diff] [review]
patch

How about this instead then?
Attachment #675985 - Attachment is obsolete: true
Attachment #675985 - Flags: review?(jwatt)
Attachment #677910 - Flags: review?(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
(Assignee)

Comment 12

4 years ago
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.
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
For future reference, just feel free to push such simple fixes yourself without r=
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.
Attachment #677910 - Flags: review?(jwatt) → review+
As discussed on IRC, pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/99a2125bd365
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86_64 → All
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.
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.
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.
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 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
Attachment #677910 - Flags: approval-mozilla-beta?
Attachment #677910 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/fd836a1f49c9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment on attachment 677910 [details] [diff] [review]
patch

since this passed the inbound-> central gauntlet, approving for branch uplift.
Attachment #677910 - Flags: approval-mozilla-beta?
Attachment #677910 - Flags: approval-mozilla-beta+
Attachment #677910 - Flags: approval-mozilla-aurora?
Attachment #677910 - Flags: approval-mozilla-aurora+
Tracking and sending email - this needs to get landed on branches asap.
tracking-firefox17: --- → +
tracking-firefox18: --- → +
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.
(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...?)
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.)
Flagging as needinfo for jwatt/longsonr for comment 25-26, regarding what to do about beta.
status-firefox18: --- → fixed
Flags: needinfo?(jwatt)
(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.
status-firefox17: --- → affected
(Assignee)

Comment 30

4 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/cfe771ad41d4
Flags: needinfo?(jwatt)
status-firefox17: affected → fixed
You need to log in before you can comment on or make changes to this bug.