Last Comment Bug 650469 - [css3-animations] "ABORT: should already have refreshed style rule" with css-animations
: [css3-animations] "ABORT: should already have refreshed style rule" with css-...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P4 critical (vote)
: mozilla6
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
Depends on:
Blocks: randomstyles 594645 435442
  Show dependency treegraph
 
Reported: 2011-04-16 01:09 PDT by Jesse Ruderman
Modified: 2011-04-26 14:56 PDT (History)
7 users (show)
dbaron: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (751 bytes, application/xhtml+xml)
2011-04-16 01:09 PDT, Jesse Ruderman
no flags Details
stack trace (1.98 KB, text/plain)
2011-04-16 01:10 PDT, Jesse Ruderman
no flags Details
patch (6.68 KB, patch)
2011-04-21 12:35 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch (6.94 KB, patch)
2011-04-21 15:21 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review

Description Jesse Ruderman 2011-04-16 01:09:23 PDT
Created attachment 526465 [details]
testcase (crashes Firefox when loaded)

###!!! ABORT: should already have refreshed style rule: 'ea->mStyleRuleRefreshTime == mPresContext->RefreshDriver()->MostRecentRefresh()', file layout/style/nsAnimationManager.cpp, line 801

This assertion is part of the new code from bug 435442, and the testcase uses MozAnimationName, so I guess that makes sense.
Comment 1 Jesse Ruderman 2011-04-16 01:10:04 PDT
Created attachment 526466 [details]
stack trace
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-21 12:35:53 PDT
Created attachment 527620 [details] [diff] [review]
patch

The underlying problem here is that the refresh driver was lying about it's most recent refresh time.  This fixes it so that it only updates mMostRecentRefresh either (a) when it goes from not having observers (or callers of MostRecentRefresh()) to having some or (b) when it actually notifies its observers.

Note that this inserts a notification in Thaw().
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-21 12:36:20 PDT
I'll land Jesse's crashtest in a separate cset (queued up in my tree already).
Comment 4 Boris Zbarsky [:bz] 2011-04-21 13:23:15 PDT
Comment on attachment 527620 [details] [diff] [review]
patch

The Notify() call in Thaw() worries me.  Can consumers there deal with script running?  Why is that call needed?
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-21 13:53:57 PDT
Well, Thaw() advances the most recent refresh time up to the present... which I tend to think it should do.  Waiting until 15ms after Thaw() to update animation state seems a little silly.  But I suppose running things inside Thaw() probably isn't the best idea.  Maybe we should defer everything until after Thaw()?

I would like to maintain the invariant that any observer that's been registered can call MostRecentRefresh() to get the time their WillRefresh() method was called; the current Thaw() code violates that by updating the result of MostRecentRefresh() without notifying observers.  I also think just failing to update MostRecentRefresh() is also a bad idea, since if an animation starts between then and when the timer fires, it'll have its timing way off.
Comment 6 Boris Zbarsky [:bz] 2011-04-21 14:03:43 PDT
> Waiting until 15ms after Thaw() to update animation state seems a little silly.

We don't wait 15ms.  In Thaw() we immediately post an event to the event loop to call DoRefresh(), so we'll Notify(nsnull) as soon as that event is processed.

I agree that the problems you describe in the last part of comment 5 need to be fixed.  Maybe we should put a script blocker around the entire thawing process in docshell and run the Notify() call off a script runner?  If so, we can take out the DoRefresh event.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-21 15:21:32 PDT
Created attachment 527661 [details] [diff] [review]
patch

For some reason I didn't notice that DoRefresh call right there. :-)

In any case, I think the Thaw issue is mostly less serious -- how about fixing most of the problems here and worrying about that later, or at least in a distinct patch.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-21 16:14:07 PDT
Jesse's test doesn't seem to work in the crashtest harness.  (Is it the popup blocker?)
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-04-21 16:32:44 PDT
Sort of ... last I checked crash|reftests couldn't open windows at all.
Comment 10 Boris Zbarsky [:bz] 2011-04-21 17:23:14 PDT
Comment on attachment 527661 [details] [diff] [review]
patch

r=me; let's get a followup bug filed on Thaw().

I just looked at the callers, and PresShell:Thaw() can already trigger frame flushes (to ensure plugin instantiation), so we need to make sure it and its callers can handle arbitrary script running anyway.  But I agree that a separate bug for it is good.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-21 18:17:24 PDT
Filed bug 652030.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-21 20:25:53 PDT
https://hg.mozilla.org/mozilla-central/rev/156e72ac605a

Note You need to log in before you can comment on or make changes to this bug.