478128 - Crash [@ nsSVGRenderingObserver::InvalidateViaReferencedFrame] with filter, foreignObject, -moz-column

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded if MallocScribble is enabled)

Steps to reproduce:
1. export MallocScribble=1
2. Run Firefox (I'm using debug on Mac)
3. Load the testcase.

Result: Crash [@ nsSVGRenderingObserver::InvalidateViaReferencedFrame] trying to touch 0x55555589.

I think this is a regression from within the last two days.

Comment 1

[Daniel Holbert [:dholbert]]

I see this crash in my Linux debug build.  However, I've got a few differences:
 - I don't crash on the initial load, but I crash after reloading a few times (sometimes it takes as many as 5 times)
 - I don't need to enable MallocScribble to trigger the crash.  But when I do enable it, it seems to take fewer reloads to crash.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: backtrace of Linux crash

Here's a backtrace of the crash on Linux.

We're crashing at nsSVGEffects.cpp:147 on a call to the virtual method nsSVGRenderingObserver::DoUpdate, with "this" being an already-deleted (or otherwise bogus) object. (Its member data is all 0x5a5a5a5a, which I think is some form of freed memory, right?)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
>  - I don't need to enable MallocScribble to trigger the crash.  But when I do
> enable it, it seems to take fewer reloads to crash.

Jesse tells me MallocScribble only has an effect on Mac.  So, the "seems to take fewer reloads to crash" effect that I observed was probably just due to chance.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> I think this is a regression from within the last two days.

I think you're right -- I'm narrowing down a regression range, but so far it looks like it regressed in this window of Feb 10th:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5a6def05ccbc&tochange=788bd259c115

Comment 5

[Robert Longson [:longsonr]]

My money's on Bug 459666

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> My money's on Bug 459666
That's the one.  A build from that bug's fix-revision (4167419a5f4a) crashes after a few reloads, but a build from the previous revision (5a6def05ccbc) does not.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: (ignore -- intended for another bug)

bent/smaug suggested this patch (which I think might be from a different bug...?)   This seems to fix the crash in my debug build.

To be sure, I've just fired off two tryserver builds -- one unpatched & one patched -- and I'll give those a try once their done (to verify the fix in optimized builds).

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 364160 [details] [diff] [review]
(ignore -- intended for another bug)

d'oh, sorry -- I posted that last comment/attachment on the wrong bug. (intended for bug 480158)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: backing out part of bug 459666 without regressing it

Roc, you probably had some reason to change this part of the code, though
I don't quite understand what.

Other option might be to remove observer in ::DoUpdate if
we're destroying the frame tree, but that might not capture all the cases.
So I prefer that observer removes itself from the list in destructor.

Comment 10

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

The idea of the change I made was that when mReferencedFramePresShell->FrameConstructor()->IsDestroyingFrameTree() returns true due to frame reconstruction for the root element, we shouldn't call nsSVGEffects::RemoveRenderingObserver since mReferencedFrame might be dead already and in any case will be definitely destroyed (and the observer property removed that way).

So what's the actual bug here? Is mReferencedFramePresShell->FrameConstructor()->IsDestroyingFrameTree() returning true because we're reconstructing frames for the root element, but for some reason, we don't actually destroy mReferencedFrame and it ends up with a dangling reference to 'this' via its property?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

We're reconstructing frame tree, so the old tree is destroyed first.
During destruction the properties for frames are removed.
In this case it is nsSVGFilterProperty. That one inherits nsSVGRenderingObserver.
But because of IsDestroyingFrameTree() we don't actually remove the
to be deleted observer from the list. So if referenced frame is destroyed later,
its observer list is iterated, and that list contains the dead object.

Based on the comment in nsSVGRenderingObserver::GetReferencedFrame()
mReferencedFrame would be null if it was destroyed already.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

And the situation is different when destroying the shell, because then
properties are deleted after frames. See PresShell::NotifyDestroyingFrame

Comment 13

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

Hmm. I guess you're right and that fix was wrong. When nsIPresShell::IsDestroying returns true, things are weird because PresShell::NotifyDestroyingFrame *doesn't* destroy the properties of destroyed frames, which is why we tested for it specially here. Maybe we should simplify things by changing NotifyDestroyingFrame so it always destroys frame properties. It's probably almost as efficient in practice.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

The optimization was done in bug 41119 and bug 253888.
I would guess that enumerating properties is quite a bit faster than
getting and removing them one by one.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 367031 [details] [diff] [review]
backing out part of bug 459666 without regressing it

So I'm not sure what else reasonable simple could be done here.

Comment 16

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

I think we probably should just back out the fix in bug 459666 and re-fix it differently. Assign this bug to me if you want me to do it.

Comment 17

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

Blocking, though I agree that comment 16 is likely the right way to go about the solution here.

Comment 18

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

Fixed by backing out 459666.

Comment 19

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

This is fixed on 1.9.1 because the patch that caused this regression never landed there... I suppose the other option is to remove blocking1.9.1+.