Closed Bug 606101 Opened 14 years ago Closed 14 years ago

"ABORT: observers should have unregistered" with <svg:animate>


(Core :: SVG, defect)

Not set



Tracking Status
blocking2.0 --- final+


(Reporter: jruderman, Assigned: dholbert)



(Keywords: assertion, testcase)


(2 files, 2 obsolete files)

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
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #486515 - Flags: review?(birtles)
Flags: in-testsuite?
Attached patch fix with crashtest (obsolete) — Splinter Review
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 #486515 - Attachment is obsolete: true
Attachment #486637 - Flags: review?
Attachment #486515 - Flags: review?(birtles)
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+
Right you are -- fixed here.  Thanks!
Attachment #486637 - Attachment is obsolete: true
Attachment #486785 - Flags: review+
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: --- → ?
Whiteboard: [needs landing]
Closed: 14 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.