Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140804004002 CSet: 352420f664bf When loading PDF files like the one listed in the URL field, the loading indicator is jumping back and forth. It's no longer a smooth animation. This is a regression and is first visible with Aurora 33.0a2. The latest beta isn't affected.

This can also be seen on OS X.

[Tracking Requested - why for this release]:

PDF.js is an important component. Tracking. Yuri, does it ring a bell?

Regression range: Last good revision: bdac18bd6c74 (2014-06-20) First bad revision: 776f6d341b3f (2014-06-21) Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bdac18bd6c74&tochange=776f6d341b3f Mozilla inbound: Last good revision: e9118a2be9b3 First bad revision: 2c21b94a7775 Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e9118a2be9b3&tochange=2c21b94a7775 Seems to be regressed by bug 1025709.

Brian, could you have a look once you come back from PTO? Removing Yury's ni since it does not seems related to pdf.js

Hmm, it's weird that I can able to replicate that when attachment 8467828 [details] is saved locally and opened from the file system.

In local build: Last Good: 2d45653b217d First Bad: 31695984cfe2

So a regression from https://hg.mozilla.org/mozilla-central/rev/31695984cfe2 , then.

I suspect the problem is that nsTransitionManager::StyleContextChanged sets collection->mStyleRule to null at the end, but with the new way that EnsureStyleRuleFor works after the above patch, it needs to set collection->mStyleRuleRefreshTime to null instead. Fixing this will likely allow removing the added null-check in WalkTransitionRule. My tree is in an odd state right now so I can't test this until later.

I think that's not it, although I'm not 100% sure I'm looking at the testcase correctly.

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #11) > I suspect the problem is that nsTransitionManager::StyleContextChanged sets > collection->mStyleRule to null at the end, but with the new way that > EnsureStyleRuleFor works after the above patch, it needs to set > collection->mStyleRuleRefreshTime to null instead. That works for me. It also fixes bug 1045906. WIP patch forthcoming. > Fixing this will likely allow removing the added null-check in > WalkTransitionRule. That does seem to be the case for the spot-testing I did just now but I haven't run all the tests yet. > My tree is in an odd state right now so I can't test this until later.

This patch fixes a regression from https://hg.mozilla.org/mozilla-central/rev/31695984cfe2 (bug 1025709). That patch replaced the EnsureStyleRuleFor method on ElementTransitions and ElementAnimations with a common method in CommonElementAnimationData. ElementTransitions::EnsureStyleRuleFor would create a new style rule if there was no style rule (mStyleRule == nullptr) or if the refresh time was old (mStyleRuleRefreshTime != aRefreshTime). ElementAnimations::EnsureStyleRuleFor, however, would create a new style rule only if mStyleRuleRefreshTime was null or old since a null style rule may still be valid for animations (unlike transitions). If we bail as soon as we a null style rule we would never update mNeedsRefreshes when the animation finishes. The unified version of EnsureStyleRuleFor in ElementAnimations adopted the behavior from ElementAnimations checking for a null or old mStyleRuleRefreshTime. However, nsTransitionManager::StyleContextChanged sets mStyleRule to nullptr to indicate that we need to generate a new style rule. This means that we will fail to create a style rule for the transition in some cases and the cover rule will be applied instead??? (Is this right??) This patch addresses this by making nsTransitionManager::StyleContextChanged set mStyleRuleRefreshTime to a null timestamp. Setting mStyleRule to nullptr is no longer necessary since EnsureStyleRuleFor will do this if necessary and nsTransitionManager::mStyleRule is only used after calling EnsureStyleRuleFor. This patch also removes a null-check from nsTransitionManager::WalkTransitionRule since we no longer set mStyleRule to nullptr. (TODO: Check this is actually correct.)

(In reply to Brian Birtles (:birtles) from comment #13) > (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from > comment #11) ... > > Fixing this will likely allow removing the added null-check in > > WalkTransitionRule. > > That does seem to be the case for the spot-testing I did just now but I > haven't run all the tests yet. Running all the tests reveals we can't remove that null-check or else we fail this assertion in the constructor of nsRuleNode: http://hg.mozilla.org/mozilla-central/file/391f42c733fc/layout/style/nsRuleNode.cpp#l1459

(In reply to Brian Birtles (:birtles) from comment #15) > Running all the tests reveals we can't remove that null-check or else we fail > this assertion in the constructor of nsRuleNode: > > > http://hg.mozilla.org/mozilla-central/file/391f42c733fc/layout/style/ > nsRuleNode.cpp#l1459 Yeah, I think the interesting question is why it was ok without the null-check before, and thus what rebuilding we did before that we're failing to do now.

Comment on attachment 8471420 [details] [diff] [review] Proposed patch r=dbaron However, I'm still not ok with the addition of collection->mNeedsRefreshes = true; to nsTransitionManager::WalkTransitionRule which was done in the same patch. It seems like it's just adding unnecessary work rebuilding transition data if we have multiple style flushes. I'd also like to understand why WalkTransitionRule needs a null-check now when it didn't before, but I'm a bit less worried about that. I guess those can be fixed in separate patches, though.

(Sorry for the delay; I was waiting for a chance to look into those issues more closely, but that hasn't happened, so I'm just asking you to do it, which is probably what I should have done in the first place.)

I also had a mochitest in my progress on writing a patch for this, which I believe I'd tested to be a valid test for the bug although it's probably worth double-checking: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/5aa5f56bf8a1/transition-force-rebuild

Er, no, I guess I hadn't gotten it to show the bug.

I fixed up the commit message and pushedyour patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8dd1441e53 and I also fixed my test and pushed it: https://hg.mozilla.org/integration/mozilla-inbound/rev/ac59c5f851dc

https://hg.mozilla.org/mozilla-central/rev/3d8dd1441e53 https://hg.mozilla.org/mozilla-central/rev/ac59c5f851dc

Comment on attachment 8471420 [details] [diff] [review] Proposed patch Approval Request Comment [Feature/regressing bug #]: bug 1025709 [User impact if declined]: jumpy CSS transitions when they interleave with other changes [Describe test coverage new/current, TBPL]: mochitest landed along with patch [Risks and why]: low risk [String/UUID change made/needed]: no

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8bfb14bab0c https://hg.mozilla.org/releases/mozilla-aurora/rev/e85f5894f075

Verified as fixed using Aurora 33.0a2 (20140825004004) and Nightly 34.0a1 (20140824030210) under Win 7 64-bit, Ubuntu 13.04 64-bit and Mac OSX 10.9.4.

(In reply to David Baron [:dbaron] (UTC+2) (needinfo? for questions) (away/busy until Sep 11) from comment #19) > However, I'm still not ok with the addition of > collection->mNeedsRefreshes = true; > to nsTransitionManager::WalkTransitionRule which was done in the same patch. > It seems like it's just adding unnecessary work rebuilding transition data > if we have multiple style flushes. > > I'd also like to understand why WalkTransitionRule needs a null-check now > when it didn't before, but I'm a bit less worried about that. > > I guess those can be fixed in separate patches, though. I filed bug 1061364 for both of these issues.