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

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: jruderman, Assigned: roc)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
crash, fixed1.9.1, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 361876 [details]
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.
(Reporter)

Updated

10 years ago
Whiteboard: [sg:critical?]
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.
OS: Mac OS X → All
Hardware: x86 → All
Created attachment 363694 [details]
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?)
(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.
Blocks: 459666
Created attachment 364160 [details] [diff] [review]
(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 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)
Attachment #364160 - Attachment description: proposed patch, from bent/smaug → (ignore -- intended for another bug)
Attachment #364160 - Attachment is obsolete: true
Created attachment 367031 [details] [diff] [review]
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.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #367031 - Flags: superreview?(roc)
Attachment #367031 - Flags: review?(roc)
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.
Assignee: Olli.Pettay → nobody
Status: ASSIGNED → NEW
Assignee: nobody → roc
Flags: blocking1.9.1?
Blocking, though I agree that comment 16 is likely the right way to go about the solution here.
Flags: blocking1.9.1? → blocking1.9.1+
Fixed by backing out 459666.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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+.
Keywords: fixed1.9.1
Crash Signature: [@ nsSVGRenderingObserver::InvalidateViaReferencedFrame]
Group: core-security
You need to log in before you can comment on or make changes to this bug.