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

VERIFIED FIXED in mozilla2.0b7

Status

()

Core
SVG
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: birtles)

Tracking

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

Trunk
mozilla2.0b7
x86
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Created attachment 468956 [details]
testcase

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

Comment 1

7 years ago
Created attachment 468957 [details]
stack
(Assignee)

Comment 2

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

Updated

7 years ago
Blocks: 594645
(Assignee)

Comment 3

7 years ago
Created attachment 481729 [details] [diff] [review]
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.
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)
(Assignee)

Comment 6

7 years ago
Created attachment 482429 [details] [diff] [review]
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.
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+
(Assignee)

Comment 8

7 years ago
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?
Attachment #482429 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 9

7 years ago
Created attachment 482752 [details] [diff] [review]
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.
Attachment #482752 - Flags: review?(dholbert)
Attachment #482752 - Flags: review?(dholbert) → review+
(Assignee)

Comment 10

7 years ago
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?
Attachment #482752 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 11

7 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/4665d2b0ea39
http://hg.mozilla.org/mozilla-central/rev/2ea2a3b3df0d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Depends on: 641388

Comment 12

7 years ago
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.