Created attachment 484966 [details] testcase (causes abort during shutdown) During shutdown: ###!!! ABORT: observers should have unregistered: 'ObserverCount() == 0', file layout/base/nsRefreshDriver.cpp, line 96
I'll look into this. Taking...
Assignee: nobody → dholbert
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Created attachment 486515 [details] [diff] [review] fix So this testcase gets us this rare-but-valid sequence of events: - <svg> node gets detached from document --> our Animation Controller has no child time-containers registered - new <svg> node created (with a time controller, which isn't yet registered) - <animate> node created & attached to our new <svg> node --> animation gets registered with Animation Controller --> we call StartSampling since this is our first registered animation - <svg> node attached to document --> we call StartSampling since this is our first registered time-container and we have a nonzero number of registered animations. So we call StartSampling twice, which means we register *twice* with the nsRefreshDriver. But at tear-down, we only unregister once, so we still occupy one entry in its observer list. And it aborts. The implememtnations of nsSMILAnimationController::AddChild & nsSMILAnimationController::RemoveChild show that we intend to maintain the following invariant: "When there are no child containers, sampling should be disabled." However, the sequence of steps at the top of this post violates that invariant, in the chunk for <animate>-binding -- there, we start sampling when no child containers are registered. This patch avoids the StartSampling call in that case, thereby preserving this invariant. Note that this patch clears mDeferredStartSampling, without necessarily immediately starting sampling. That might seem counterintuitive, but it's actually fine, because we're really just transitioning into the "OK, we're now ready to start sampling anytime that we have a registered child container" state. (which is the same state that nsSMILAnimationController::RemoveChild puts us into) I've confirmed that this fixes the attached testcase. Haven't tested much beyond that yet, but I'm pretty sure this is right.
Created attachment 486637 [details] [diff] [review] fix with crashtest Here's the same fix, with crashtest attached. (In reply to comment #2) > I've confirmed that this fixes the attached testcase. Haven't tested much > beyond that yet, but I'm pretty sure this is right. Tested more thoroughly now -- this passed a TryServer run last night.
Attachment #486637 - Flags: review? → review?(birtles)
Comment on attachment 486637 [details] [diff] [review] fix with crashtest Looks great! One nit: > + // mAnimationElementTable was empty until we just inserted its first element I think this line may be more than 80 chars long. Thanks Daniel!
Attachment #486637 - Flags: review?(birtles) → review+
Created attachment 486785 [details] [diff] [review] fix with crashtest & nit fixed [r=birtles] Right you are -- fixed here. Thanks!
Requesting blocking. This is an abort in debug builds due to registering twice with the nsRefreshDriver (but only unregistering once). Patch makes us just register once.
blocking2.0: --- → ?
blocking2.0: ? → final+
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.