464374 - Crash [@ nsSVGUtils::MaxExpansion] with svg filter, mathml

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: SVG frames should not get here: '!aNonSVGFrame->IsFrameOfType(nsIFrame::eSVG)', file /Users/jruderman/central/layout/svg/base/src/nsSVGIntegrationUtils.cpp, line 399

followed by a null deref [@ nsSVGUtils::MaxExpansion].

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

precedence

Comment 3

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

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

But can you explain what caused the crash and why this fixes it?

Comment 4

[Robert Longson [:longsonr]]

Here, or as a comment in the code? Assuming you meant here...

All of the elements are in a <defs> section which makes them unrendered. <defs> children only exist for use by other rendered elements such as markers and don't have a covered region.

The testcase manages to cause a style change to a pathGeometryFrame which is a child of a defs frame. Presumably the pathGeometryFrame is that of the rect element as that would seem to be the only thing that would have one. 

When the invalidation code runs on something that doesn't have a covered region it goes boom.

You'll find the same thing in nsSVGDisplayContainerFrame::RemoveFrame.

Comment 5

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

Perhaps it would be better to move the NONDISPLAY_CHILD check to InvalidateCoveredRegion, instead of checking that at every call site?

Comment 6

[Robert Longson [:longsonr]]

It's more efficient this way as you can avoid the outer svg frame call. How about hoisting the whole thing to a new nsSVGUtils::InvalidateCoveredRegion method?

Comment 7

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

Sure.

Comment 8

[Robert Longson [:longsonr]]

Attachment: address review comment

I need to test this.

Comment 9

[Robert Longson [:longsonr]]

Attachment: address review comment v2 [Checkin: Comment 14 & 16]

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 348066 [details] [diff] [review]
address review comment v2
[Checkin: Comment 14 & 16]

Simple fix for crash. Just adds a test for whether the function will crash and avoids calling it.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348066 [details] [diff] [review]
address review comment v2
[Checkin: Comment 14 & 16]

a191=beltzner

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 348066 [details] [diff] [review]
address review comment v2
[Checkin: Comment 14 & 16]

http://hg.mozilla.org/mozilla-central/rev/c764915ebd2e

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 348066 [details] [diff] [review]
address review comment v2
[Checkin: Comment 14 & 16]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/598d498219ce

Comment 17

[Marcia Knous [:marcia]]

verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081230 Shiretoko/3.1b3pre. Adding keyword.

verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081230 Minefield/3.2a1pre.