386475 - Crash [@ nsRect::nsRect] [@ nsSVGFilterProperty::GetRect]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase crashes Firefox (Mac trunk).

Thread 0 Crashed:
0   nsRect::nsRect[in-charge](nsRect const&) + 47 (nsRect.h:61)
1   nsSVGFilterProperty::GetRect() + 30 (nsSVGUtils.cpp:174)
2   nsSVGUtils::FindFilterInvalidation(nsIFrame*) + 132 (nsSVGUtils.cpp:789)
3   nsSVGForeignObjectFrame::UpdateGraphic() + 263 (nsSVGForeignObjectFrame.cpp:544)
4   nsSVGForeignObjectFrame::NotifyRedrawUnsuspended() + 64 (nsSVGForeignObjectFrame.cpp:406)
5   nsSVGDisplayContainerFrame::NotifyRedrawUnsuspended() + 75 (nsSVGContainerFrame.cpp:275)
6   nsSVGOuterSVGFrame::UnsuspendRedraw() + 136 (nsSVGOuterSVGFrame.cpp:522)

Comment 1

[Robert Longson [:longsonr]]

When we resolve em units the side effect is to flush layout, in this case that destroys the frame we are processing because there is an unflushed change to layout.

Possible solutions I see would be one of:

a) Flush before we try to do the outer frame NotifyViewPortChange
b) Rewrite the em units code not to flush.

Comment 2

[Robert Longson [:longsonr]]

Attachment: option a) flush layout changes

Comment 3

[tor]

Comment on attachment 270728 [details] [diff] [review]
option a) flush layout changes

I don't think I know enough about the possible side effects of pushing flushes to usefully review this change.

Comment 4

[Boris Zbarsky [:bzbarsky]]

That patch looks wrong, in that after that flush call |this| could be dead, so accessing mContent is bad...

Comment 5

[Robert Longson [:longsonr]]

Attachment: plan b

Avoid calling the function that flushes altogether then.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Assuming the code in question will handle a future reresolve or reframe correctly, and that you don't need this style for display:none things, that seems like absolutely the way to go.

Comment 7

[Robert Longson [:longsonr]]

Attachment: updated patch (remove unnecessary includes)

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #6)
> Assuming the code in question will handle a future reresolve or reframe
> correctly, and that you don't need this style for display:none things, that
> seems like absolutely the way to go.
> 

The when the reframe occurs we just go again with the new frame layout.

We have various problems with things in defs elements being set to display:none and then referenced. I don't think this will make things any worse.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 270863 [details] [diff] [review]
updated patch (remove unnecessary includes)

Looks great!

Comment 10

[Robert Longson [:longsonr]]

checked in.

Comment 11

[Jesse Ruderman]

Crashtest checked in.