Jesse Ruderman Reporter
	Description • 14 years ago

During shutdown:

###!!! ABORT: observers should have unregistered: 'ObserverCount() == 0', file layout/base/nsRefreshDriver.cpp, line 96

	Daniel Holbert [:dholbert] Assignee
	Comment 1 • 14 years ago

I'll look into this.  Taking...

	Daniel Holbert [:dholbert] Assignee
	Comment 2 • 14 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 3 • 14 years ago

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.

	Brian Birtles (:birtles)
	Comment 4 • 14 years ago

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!

	Daniel Holbert [:dholbert] Assignee
	Comment 5 • 14 years ago

Right you are -- fixed here.  Thanks!

	Daniel Holbert [:dholbert] Assignee
	Comment 6 • 14 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 7 • 14 years ago

Landed: http://hg.mozilla.org/mozilla-central/rev/ce4e85563024