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

RESOLVED FIXED in mozilla2.0b8

Status

()

--
critical
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jruderman, Assigned: dholbert)

Tracking

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

Trunk
mozilla2.0b8
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

8 years ago
I'll look into this.  Taking...
Assignee: nobody → dholbert
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 2

8 years ago
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.
Attachment #486515 - Flags: review?(birtles)
(Assignee)

Updated

8 years ago
Flags: in-testsuite?
(Assignee)

Comment 3

8 years ago
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 #486515 - Attachment is obsolete: true
Attachment #486637 - Flags: review?
Attachment #486515 - Flags: review?(birtles)
(Assignee)

Updated

8 years ago
Attachment #486637 - Flags: review? → review?(birtles)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
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+
(Assignee)

Comment 5

8 years ago
Created attachment 486785 [details] [diff] [review]
fix with crashtest & nit fixed [r=birtles]

Right you are -- fixed here.  Thanks!
Attachment #486637 - Attachment is obsolete: true
Attachment #486785 - Flags: review+
(Assignee)

Comment 6

8 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.
blocking2.0: --- → ?
(Assignee)

Updated

8 years ago
Whiteboard: [needs landing]
(Assignee)

Comment 7

8 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/ce4e85563024
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.