554141 - Crash [@ nsTArray_base::Length] during GC involving SMIL

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (causes crash on shutdown or earlier)

This might be the same as orange bug 549715.

Comment 1

[Jesse Ruderman]

Attachment: testcase (causes crash on shutdown or earlier)

Comment 2

[Jesse Ruderman]

Attachment: stack trace

Comment 3

[Daniel Holbert [:dholbert]]

Confirmed on my Linux box, in a debug build, and I think also in an opt build (opt crash, a few minutes after loading testcase, was 8adea4c1-589b-4dc5-a78d-b72942100322 which is missing most of its symbols for some reason).

I didn't hit it as a shutdown crash, though -- I was able to shut down Firefox just fine after loading testcase.  I hit the crash when I loaded testcase, waited a while, navigated to another site, and then opened a new tab and loaded testcase there.  (Not sure how reliable those steps are, just documenting 'em so I don't forget)

Comment 4

[Julian Seward [:jseward]]

Testcase in comment 2 reliably generates the following trace for me at
shutdown.  Seems like js_GC is finalizing something that has already
been deleted "by hand".

Invalid read of size 1
   at 0x5D39BA0: nsSMILTimeValueSpec::HandleDeletedInstanceTime(nsSMILInstanceTime&) (nsSMILTimeValueSpec.cpp:222)
   by 0x5D32454: nsSMILInstanceTime::HandleDeletedInterval() (nsSMILInstanceTime.cpp:161)
   by 0x5D32968: nsSMILInterval::NotifyDeleting() (nsSMILInterval.cpp:82)
   by 0x5D3774E: nsSMILTimedElement::~nsSMILTimedElement() (nsSMILTimedElement.cpp:152)
   by 0x5D28ABF: nsSVGAnimateElement::~nsSVGAnimateElement() (nsSVGAnimationElement.h:57)
   by 0x596EEDB: nsNodeUtils::LastRelease(nsINode*) (nsNodeUtils.cpp:274)
   by 0x5961B7A: nsGenericElement::Release() (nsGenericElement.cpp:4178)
   by 0x5592D3D: XPC_WN_NoHelper_Finalize(JSContext*, JSObject*) (xpcwrappednativejsops.cpp:701)
   by 0x6B9AB7F: js_GC (jsgc.cpp:2546)
   by 0x55443F4: nsXREDirProvider::DoShutdown() (nsXREDirProvider.cpp:872)
   by 0x553D9CF: ScopedXPCOMStartup::~ScopedXPCOMStartup() (nsAppRunner.cpp:1051)
   by 0x55420CC: XRE_main (nsAppRunner.cpp:3589)
 Address 0x1bd4f7b8 is 8 bytes inside a block of size 112 free'd
   at 0x4C2513D: free (vg_replace_malloc.c:366)
   by 0x5D371B8: nsSMILTimedElement::ClearBeginOrEndSpecs(int) (mozalloc.h:246)
   by 0x5D386FD: nsSMILTimedElement::UnsetBeginSpec() (nsSMILTimedElement.cpp:672)
   by 0x5D38784: nsSMILTimedElement::UnsetAttr(nsIAtom*) (nsSMILTimedElement.cpp:634)
   by 0x5D29D7F: nsSVGAnimationElement::UnsetAttr(int, nsIAtom*, int) (nsSVGAnimationElement.cpp:398)
   by 0x5958F59: nsGenericElement::RemoveAttribute(nsAString_internal const&) (nsGenericElement.cpp:2093)
   by 0x55E783E: nsIDOMElement_RemoveAttribute(JSContext*, unsigned int, long*) (dom_quickstubs.cpp:4214)
   by 0x6BA49D1: js_Interpret (jsops.cpp:2236)
   by 0x6BAF607: js_Invoke (jsinterp.cpp:843)
   by 0x6BAFB77: js_InternalInvoke (jsinterp.cpp:900)
   by 0x6B62679: JS_CallFunctionValue (jsapi.cpp:4966)
   by 0x5AEF96A: nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) (nsJSEnvironment.cpp:2157)

Comment 5

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

Looks like dholbert should take this

Comment 6

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

Actually, Brian is probably better.

Comment 7

[Daniel Holbert [:dholbert]]

Agreed, since this is in timing-model code from the syncbase timing patch.  

/me reassigns the related bug 549715 to Brian, too (also in syncbase timing code, & likely the same underlying issue as this bug here).

Comment 8

[Brian Birtles (:birtles)]

I'm more than happy to look at this but at this stage I'm not expecting to be up and running until 17 May at the earliest.

Comment 9

[Brian Birtles (:birtles)]

I've only just started looking into this but I hope it should be fixable within the week.

Currently, based on the stack trace in comment 4, my suspicion is that we might be removing nsSMILTimeValueSpec objects (in nsSMILTimedElement::ClearBeginOrEndSpecs) without disassociating all the instance times that are pointing to them (via raw pointer mCreator) and hence when we clean up the instance times we dereference the dangling pointer. Just a guess though. I'll look into it again tomorrow but that will be after the next critsmash meeting.

Comment 10

[Brian Birtles (:birtles)]

Attachment: Simplified test case (crashes browser immediately)

Here's a simplified test case that crashes the browser immediately for me.
The problem appears to be a case where unresolved begin times float around after the spec that created them is destroyed.

Comment 11

[Jesse Ruderman]

Attachment: stack trace for simplified testcase

Comment 12

[Jesse Ruderman]

Hmm, I had nsSMILTimedElement::GetTimeContainer down as being bug 551620.  Is it a dup?

Comment 13

[Brian Birtles (:birtles)]

Attachment: Proposed patch v1a

With this patch applied I can no longer reproduce the problem.

Comment 14

[Brian Birtles (:birtles)]

(In reply to comment #12)
> Hmm, I had nsSMILTimedElement::GetTimeContainer down as being bug 551620.  Is
> it a dup?

I haven't looked into bug 551620 yet but the proposed patch seems to fix that as well.

Looking at the stack trace, I think what's happening there is that pointer to the nsSMILTimeValueSpec object from the nsSMILInstanceTime dangles and from there it's just a matter of time before everything comes crashing down. The proposed patch makes sure the nsSMILInstanceTime gets cleaned up when the attribute is removed (because now we track all nsSMILInstanceTimes including unresolved begin times).

Comment 15

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

Comment on attachment 446434 [details] [diff] [review]
Proposed patch v1a

+// a) belong to the same nsSMILTimedElement but reference the instance times
+//    from which they are derived; and

Should be 'or' instead of 'and'?

Comment 16

[Brian Birtles (:birtles)]

Attachment: Patch v1b

Documentation updates following review

Comment 17

[Brian Birtles (:birtles)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/39b66ca816a3