590425 - "ASSERTION: Starting sampling but controller is paused" with <svg:animate> in removed iframe

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Starting sampling but controller is paused: 'mPauseState == 0', file /builds/slave/mozilla-central-macosx-debug/build/content/smil/nsSMILAnimationController.cpp, line 248

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Not sure what this is yet so I don't know if it's worth nominating as a blocker.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1a

This patch tightens up some assumptions about deferred sampling since it's possible for us to set the deferred sampling flag and then get paused in the meantime.

Comment 4

[Daniel Holbert [:dholbert]]

> it's
> possible for us to set the deferred sampling flag and then get paused in the
> meantime.

I think perhaps Pause() should just clear mDeferredStartSampling, because while we're paused, we're not actually deferring sampling anymore, right?

I think that might simplify this patch a bit.  In particular:
 - (your first modified chunk) Resume() wouldn't need to clear mDeferredStartSampling, because it could already be assumed to be clear whenever we're paused.
 - (your second modified chunk) !mPauseState would be implied, whenever mDeferredStartSampling is true.

Comment 5

[Daniel Holbert [:dholbert]]

(... and then when we Resume() from our Pause(), we'll turn on mDeferredStartSampling if appropriate -- which the mechanics are already there to do)

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1b

(In reply to comment #4)
> I think perhaps Pause() should just clear mDeferredStartSampling, because while
> we're paused, we're not actually deferring sampling anymore, right?

Yep, good idea. That's a lot simpler.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 482429 [details] [diff] [review]
Patch v1b

Whoah, that simplifies the patch a lot! Nice. :)

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 482429 [details] [diff] [review]
Patch v1b

Requesting approval to land. Fixes a failing assertion and includes a test.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2: Make nsPresShell's interaction with the animation controller account for deferred sampling, v1a

As discussed with dholbert on IRC, the assertion added in the first patch was failing on OSX and Linux on tryserver so I've added an extra patch to rework the interaction between nsPresShell and nsSMILAnimationController to hopefully fix these issues. Currently running this two patches through TryServer.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 482752 [details] [diff] [review]
Part 2: Make nsPresShell's interaction with the animation controller account for deferred sampling, v1a

Requesting approval to land. As per previous comment, the original patch had failing assertions on OSX/Linux so this additional patch is required to rework the interaction between nsPresShell and nsSMILAnimationController.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/4665d2b0ea39
http://hg.mozilla.org/mozilla-central/rev/2ea2a3b3df0d

Comment 12

[George Carstoiu]

Issue verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre