Closed Bug 472782 Opened 11 years ago Closed 11 years ago

Crash due to too much recursion involving <svg:textPath>

Categories

(Core :: SVG, defect, P3, critical)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, verified1.9.1)

Attachments

(2 files)

Loading the testcase makes Firefox crash due to too much recursion:

...
412  nsSVGTextPathProperty::DoUpdate() + 198 (nsSVGEffects.cpp:250)
413  nsSVGRenderingObserver::InvalidateViaReferencedFrame() + 44 (nsSVGEffects.cpp:141)
414  nsSVGRenderingObserverList::InvalidateAll() + 109 (nsSVGEffects.cpp:406)
415  nsSVGEffects::InvalidateRenderingObservers(nsIFrame*) + 166 (nsSVGEffects.cpp:469)
416  nsSVGUtils::UpdateGraphic(nsISVGChildFrame*) + 42 (nsSVGUtils.cpp:606)
417  nsSVGGlyphFrame::SetGlyphPosition(float, float) + 56 (nsSVGGlyphFrame.cpp:828)
418  nsSVGTextFrame::UpdateGlyphPositioning(int) + 1296 (nsSVGTextFrame.cpp:450)
419  nsSVGTextFrame::NotifyGlyphMetricsChange() + 32 (nsSVGTextFrame.cpp:302)
420  nsSVGTextContainerFrame::NotifyGlyphMetricsChange() + 37 (nsSVGTextContainerFrame.cpp:60)
421  nsSVGTextPathProperty::DoUpdate() + 198 (nsSVGEffects.cpp:250)
...
Attached patch patchSplinter Review
FWIW the patch in bug 472135 would fix this too, although I suppose this patch is neater and more efficient.
Assignee: nobody → longsonr
Attachment #356602 - Flags: superreview?(roc)
Attachment #356602 - Flags: review?(roc)
OS: Mac OS X → All
Hardware: x86 → All
I think we should just rely on 472135 to fix this. I don't think we should add code to speed up incorrect markup.
Depends on: 472135
I tried to rewrite this along the marker/filter approach and it just doesn't hang together. Unlike markers and filters, text locations are cached and we update them only when necessary via NotifyGlyphMetrics change.

If I move to async painting then there is nothing to trigger UpdateEffects in the first place as the property is not referenced during painting, only when the text metrics i.e. the glyph locations change.

So after much thought I still think the original patch is the right thing to do. You can't get reference loops in text unless you fail to check the referenced frame type as referenced frame must point to a path and a path can't contain text (or anything else for that matter).
Comment on attachment 356602 [details] [diff] [review]
patch

OK
Attachment #356602 - Flags: superreview?(roc)
Attachment #356602 - Flags: superreview+
Attachment #356602 - Flags: review?(roc)
Attachment #356602 - Flags: review+
Flags: blocking1.9.1+
Priority: -- → P3
Pushed http://hg.mozilla.org/mozilla-central/rev/42b8e2c66c5b.

The test should be checked in as a crashtest, but I forgot to do that.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Flags: in-testsuite? → in-testsuite+
verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090415 Minefield/3.6a1pre ID:20090415030845

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090416 Shiretoko/3.5b4pre ID:20090416030924
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.