Closed Bug 841163 Opened 7 years ago Closed 7 years ago

Crash with svg.text.css-frames.enabled, document.normalize()

Categories

(Core :: SVG, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: jruderman, Assigned: heycam)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files)

With:
  user_pref("svg.text.css-frames.enabled", true);

The testcase crashes [@ mozilla::TextNodeCorrespondenceRecorder::TraverseAndRecord]
Attached file stack
On Windows: bp-eb813db8-2efc-4289-a1db-949592130213
Crash Signature: [@ mozilla::TextNodeCorrespondenceRecorder::TraverseAndRecord(nsIFrame*) ]
OS: Mac OS X → All
Hardware: x86_64 → All
The crash is because the frame tree looks a bit weird at the point that we're trying to reflow it, because:

1. We begin with <g filter="..."><text>AB</text></g>.
2. We call TextNode.splitText(1), to split this into an "A" and a "B" text frame.
3. We call document.normalize(), which is in the middle of re-joining those two
   text nodes, and it has stuck the "B" back on the end of the first text node
   and has just removed the second text node.
4. The removal of the second text node gets us into
   nsSVGTextFrame2::MutationObserver::ContentRemoved, which wants to invalidate
   the nsSVGTextFrame2, which in the process of computing the filtered area of the
   <g>, results in the existing frame tree under the nsSVGTextFrame2 being reflowed,
   while it looks a bit weird.

It looks like this:

SVGText2(text)(1)@0x124a74050 {-15,-720,1364,960} [state=0009880000011200] [content=0x12600ee80] [sc=0x124a73e90]<
  Block(text)(1)@0x124a74288 [content=0x12600ee80] {0,0,3946,2352} [state=0008900000d00000] [vis-overflow=-30,0,4006,2352] [scr-overflow=0,0,3946,2352] sc=0x124a73fb0(i=1,b=0) pst=:-moz-svg-text<
    line 0x124a74508: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,3946,2352} vis-overflow={-30,0,4006,2352} scr-overflow={0,0,3946,2352} <
      Text(0)"AB"@0x124a74498 [run=0x1153bd170][0,2,T]  next=0x124a75120 {0,0,2666,1920} [state=20088000d0200000] [content=0x12600efd0] [vis-overflow=-30,0,2726,1920] [scr-overflow=0,0,2666,1920] sc=0x124a743b0 pst=:-moz-non-element
      Text(-1)"B"@0x124a75120 [run=0x1153bd170][0,1,T]  {2666,0,1280,1920} [state=20008000c0400000] [content=0x126308da0] [vis-overflow=-30,0,1340,1920] [scr-overflow=0,0,1280,1920] sc=0x124a743b0 pst=:-moz-non-element
    >
  >
>

So we still have that second nsTextFrame hanging around.  What's the right thing to do here?  Should we be flushing layout from inside nsSVGTextFrame2::MutationObserver::ContentRemoved to make sure the frame tree has been reconstructed properly?  Is it an acceptable time to be doing that?
Those two text frames are for different nodes. Although this is a weird DOM state, I don't see the problem with the frame tree ... I think it's what I'd expect for node "AB" followed by node "B". Why are we crashing?
The second text frame corresponds to the nsTextNode that was just removed from the tree, resulting in the DOM only having a single nsTextNode for "AB".  The "B" for the nsTextFrame remains in the frame tree because we haven't redone the frame construction yet.

The TextNodeCorrespondenceRecorder stuff relies on the DOM and frame tree being in sync, which is why it's crashing.

I wonder if this happens when we do normal DOM manipulations that land us in nsSVGTextFrame2::MutationObserver when we are being filtered...
Maybe you should be flushing frame construction as well as reflowing?
There's nothing special about document.normalize() it seems; I can get a crash by doing the same steps (starting with two separate text nodes, modifying one, and then removing the other).

It does seem then that since when we're filtered we'll need to have flushed frame construction.  Calling doc->FlushPendingNotifications(Flush_Frames) however doesn't do anything since there is a script blocker set (not sure from where).  Based on the stack does it make sense that there would be a script blocker?

I am not really sure if it is sensible that nsSVGUtils::InvalidateBounds could require the frames to be up to date due to the filtered region bbox calculations needing it.  Is my nsSVGUtils::InvalidateBounds() call I in nsSVGTextFrame2::NotifyGlyphMetricsChange() meant to invalidate the *current* bounds or the new bounds?  The new bounds won't be available yet, since we need to do the reflow etc.  But the old bounds, if they depend on looking at the state of the frame tree, won't be computable either...
(In reply to Cameron McCormack (:heycam) from comment #7)
> It does seem then that since when we're filtered we'll need to have flushed
> frame construction.  Calling doc->FlushPendingNotifications(Flush_Frames)
> however doesn't do anything since there is a script blocker set (not sure
> from where).  Based on the stack does it make sense that there would be a
> script blocker?

Maybe. That's OK, you could probably move the update work you need to do to a scriptrunner (nsContentUtils::AddScriptRunner).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Maybe. That's OK, you could probably move the update work you need to do to
> a scriptrunner (nsContentUtils::AddScriptRunner).

But we'll need the result of the reframing to be done immediately, since we're inside the nsSVGTextFrame2::GetBBoxContribution call, and we need the frames to be up to date so we can return an accurate bbox.

I wonder if it would be OK to return a looser bounding box, say mRect?  This wouldn't require looking over all the text frames (and thus wouldn't need the reframing).  Alternatively we could store the bbox value to return from the last time we computed it.  Would this actually be the more correct thing to do?  Is it really that we should be invalidating the old area covered by the element at this point?
I mean run nsSVGTextFrame2::NotifyGlyphMetricsChange in a scriptrunner.
Oh, right, that could work.  It would mean, I think, that each consecutive DOM mutation to a filtered text element would result in the frames being reconstructed etc. for each change.
Attached patch patchSplinter Review
By using a script runner to do the current operations NotifyGlyphMetricsUpdated does, it looks like we don't need to flush frame construction -- by the time we get back to the runnable, our frames have been reconstructed.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #714155 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/a8bdef089671
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.