428228 - Removing element from <svg:svg> crashes [@ nsSVGUtils::NotifyAncestorsOfFilterRegionChange]

Status: VERIFIED FIXED

(Core :: SVG, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase crashes Firefox.

The immediate cause of the crash seems to be:

* nsSVGDisplayContainerFrame::RemoveFrame calls nsSVGUtils::NotifyAncestorsOfFilterRegionChange(this), where |this| is an nsSVGOuterSVGFrame.

* nsSVGUtils::NotifyAncestorsOfFilterRegionChange skips the passed-in frame before walking up the ancestor chain, and expects to hit an nsSVGOuterSVGFrame before it hits any non-SVG frames.

* It encounters an nsBlockFrame or nsAreaFrame that happens to have the NS_STATE_SVG_FILTERED bit (which has a different meaning for non-SVG frames), but of course no actual filter property.

This bug appeared within the last few days.  I think it's a regression from bug 423998 -- the first hunk of that bug's patch fixed a typo causing the nsSVGUtils::NotifyAncestorsOfFilterRegionChange(this) call to be skipped.

Comment 1

[Jonathan Watt [:jwatt]]

I'll try and fix this, since I'm possibly not going to be available for reviews for the next four days.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Okay, this is probably the best I can do in the time I have available, and it's probably the safest way to play this anyway.

Comment 3

[Jonathan Watt [:jwatt]]

Marking blocking1.9+. This is a recent regression that will cause us to crash whenever anyone removed a direct child of an <svg> element. Clearly unacceptable.

The patch is very low risk.

Comment 4

[Robert Longson [:longsonr]]

Does checking (aFrame->GetStateBits() & NS_STATE_IS_OUTER_SVG) not work? If so that's what you want, rather than this.

Comment 5

[Jonathan Watt [:jwatt]]

Any particular reason that it matters?

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch using NS_STATE_IS_OUTER_SVG

Comment 7

[Robert Longson [:longsonr]]

Comment on attachment 314794 [details] [diff] [review]
patch using NS_STATE_IS_OUTER_SVG

faster and consistent with rest of codebase

Comment 8

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

Comment on attachment 314794 [details] [diff] [review]
patch using NS_STATE_IS_OUTER_SVG

Make sure a crashtest is landed for this.

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 314794 [details] [diff] [review]
patch using NS_STATE_IS_OUTER_SVG

Requesting approval1.9. This is a very safe, obviously correct fix. It's important we get this fix in, since it prevents us from iterating over unknown memory.

Comment 10

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

Comment on attachment 314794 [details] [diff] [review]
patch using NS_STATE_IS_OUTER_SVG

a1.9=beltzner

Comment 11

[Jonathan Watt [:jwatt]]

Checked in.

Comment 12

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050614 Minefield/3.0pre - no crash on testcase

--> Verified fixed