Closed
Bug 779971
Opened 12 years ago
Closed 12 years ago
"ASSERTION: Must not call under nsISVGChildFrame::ReflowSVG"
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: jruderman, Assigned: longsonr)
References
Details
(Keywords: assertion, testcase)
Attachments
(4 files, 1 obsolete file)
416 bytes,
image/svg+xml
|
Details | |
8.26 KB,
text/plain
|
Details | |
969 bytes,
text/plain
|
Details | |
8.21 KB,
patch
|
jwatt
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
###!!! 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•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
Now this crashes with too-much-recursion.
Reporter | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 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•12 years ago
|
||
Assignee: nobody → longsonr
Attachment #675985 -
Flags: review?(jwatt)
Assignee | ||
Comment 6•12 years ago
|
||
Comment 7•12 years ago
|
||
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•12 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 | ||
Comment 10•12 years ago
|
||
How about this instead then?
Attachment #675985 -
Attachment is obsolete: true
Attachment #675985 -
Flags: review?(jwatt)
Attachment #677910 -
Flags: review?(jwatt)
Comment 11•12 years ago
|
||
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•12 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.
Comment 13•12 years ago
|
||
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•12 years ago
|
||
For future reference, just feel free to push such simple fixes yourself without r=
Comment 15•12 years ago
|
||
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+
Comment 16•12 years ago
|
||
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
Comment 17•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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?
Comment 22•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 23•12 years ago
|
||
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+
Comment 24•12 years ago
|
||
Tracking and sending email - this needs to get landed on branches asap.
tracking-firefox17:
--- → +
tracking-firefox18:
--- → +
Comment 25•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
Flagging as needinfo for jwatt/longsonr for comment 25-26, regarding what to do about beta.
status-firefox18:
--- → fixed
Flags: needinfo?(jwatt)
Comment 29•12 years ago
|
||
(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•12 years ago
|
||
Flags: needinfo?(jwatt)
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•