Closed Bug 590425 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: SVG, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b7

People

(Reporter: jruderman, Assigned: birtles)

References

Details

(Keywords: assertion, testcase)

Attachments

(4 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: Starting sampling but controller is paused: 'mPauseState == 0', file /builds/slave/mozilla-central-macosx-debug/build/content/smil/nsSMILAnimationController.cpp, line 248
Attached file stack
Not sure what this is yet so I don't know if it's worth nominating as a blocker.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Attached patch Patch v1a (obsolete) — Splinter Review
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.
Attachment #481729 - Flags: review?(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.
(... and then when we Resume() from our Pause(), we'll turn on mDeferredStartSampling if appropriate -- which the mechanics are already there to do)
Attached patch Patch v1bSplinter Review
(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.
Attachment #481729 - Attachment is obsolete: true
Attachment #482429 - Flags: review?(dholbert)
Attachment #481729 - Flags: review?(dholbert)
Comment on attachment 482429 [details] [diff] [review]
Patch v1b

Whoah, that simplifies the patch a lot! Nice. :)
Attachment #482429 - Flags: review?(dholbert) → review+
Comment on attachment 482429 [details] [diff] [review]
Patch v1b

Requesting approval to land. Fixes a failing assertion and includes a test.
Attachment #482429 - Flags: approval2.0?
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.
Attachment #482752 - Flags: review?(dholbert)
Attachment #482752 - Flags: review?(dholbert) → review+
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.
Attachment #482752 - Flags: approval2.0?
Pushed:
http://hg.mozilla.org/mozilla-central/rev/4665d2b0ea39
http://hg.mozilla.org/mozilla-central/rev/2ea2a3b3df0d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Depends on: 641388
Issue verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.