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

RESOLVED FIXED

Status

()

Core
SVG
--
critical
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: birtles)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
x86
All
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 434011 [details]
testcase (causes crash on shutdown or earlier)

This might be the same as orange bug 549715.
(Reporter)

Comment 1

8 years ago
Created attachment 434014 [details]
testcase (causes crash on shutdown or earlier)
Attachment #434011 - Attachment is obsolete: true
(Reporter)

Comment 2

8 years ago
Created attachment 434015 [details]
stack trace
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)
OS: Mac OS X → All
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)
(Reporter)

Updated

8 years ago
Group: core-security
Whiteboard: [sg:critical?]
Looks like dholbert should take this
Assignee: nobody → dholbert
Assignee: dholbert → birtles
Actually, Brian is probably better.
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).

Updated

8 years ago
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:investigating]
(Assignee)

Comment 8

8 years ago
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.
(Assignee)

Comment 9

8 years ago
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.
(Assignee)

Comment 10

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

Comment 11

8 years ago
Created attachment 446428 [details]
stack trace for simplified testcase
(Reporter)

Comment 12

8 years ago
Hmm, I had nsSMILTimedElement::GetTimeContainer down as being bug 551620.  Is it a dup?
(Assignee)

Comment 13

8 years ago
Created attachment 446434 [details] [diff] [review]
Proposed patch v1a

With this patch applied I can no longer reproduce the problem.
Attachment #446434 - Flags: review?(roc)
(Assignee)

Comment 14

8 years ago
(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 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'?
Attachment #446434 - Flags: review?(roc) → review+
(Assignee)

Comment 16

8 years ago
Created attachment 446621 [details] [diff] [review]
Patch v1b

Documentation updates following review
Attachment #446434 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Updated

8 years ago
Blocks: 551620
(Assignee)

Updated

8 years ago
Duplicate of this bug: 557187
(Assignee)

Updated

8 years ago
Duplicate of this bug: 549715
(Assignee)

Updated

8 years ago
Duplicate of this bug: 549709
(Assignee)

Updated

8 years ago
Duplicate of this bug: 552853
(Assignee)

Updated

8 years ago
Duplicate of this bug: 551549
Crash Signature: [@ nsTArray_base::Length]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.