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

Status: VERIFIED FIXED

(Core :: SVG, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

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)
...

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

FWIW the patch in bug 472135 would fix this too, although I suppose this patch is neater and more efficient.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should just rely on 472135 to fix this. I don't think we should add code to speed up incorrect markup.

Comment 3

[Robert Longson [:longsonr]]

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 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 356602 [details] [diff] [review]
patch

OK

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/42b8e2c66c5b.

The test should be checked in as a crashtest, but I forgot to do that.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1c9757fe35c2

Comment 7

[Aakash Desai [:aakashd]]

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