734 bytes, image/svg+xml
3.69 KB, text/plain
961 bytes, image/svg+xml
7.90 KB, text/plain
19.41 KB, patch
|Details | Diff | Splinter Review|
Created attachment 434011 [details] testcase (causes crash on shutdown or earlier) This might be the same as orange bug 549715.
Created attachment 434014 [details] testcase (causes crash on shutdown or earlier)
Attachment #434011 - Attachment is obsolete: true
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)
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).
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:investigating]
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.
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.
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.
Hmm, I had nsSMILTimedElement::GetTimeContainer down as being bug 551620. Is it a dup?
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)
(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+
Created attachment 446621 [details] [diff] [review] Patch v1b Documentation updates following review
Attachment #446434 - Attachment is obsolete: true
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsTArray_base::Length]
You need to log in before you can comment on or make changes to this bug.