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.

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.

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

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

(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

My money's on Bug 459666

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

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

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.

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?

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.

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

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.

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

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.

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

Fixed by backing out 459666.

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