1048838 - PDF loading indicator is jumping back and forth (flickering) while loading the file

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This can also be seen on OS X.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

[Tracking Requested - why for this release]:

Comment 3

[Sylvestre Ledru [:Sylvestre]]

PDF.js is an important component. Tracking.

Yuri, does it ring a bell?

Comment 4

[Petruta Horea [:phorea], Desktop QA]

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.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

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

Comment 6

[Yury Delendik (:yury)]

Attachment: Minimal test case to replicate the issue

Comment 7

[Yury Delendik (:yury)]

Attachment: Minimal test case to replicate the issue (save on the file system)

Comment 8

[Yury Delendik (:yury)]

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

Comment 9

[Alice0775 White]

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

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 13

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

(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.

Comment 14

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

Attachment: Proposed patch

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.)

Comment 15

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

Attachment: Proposed patch

(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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 17

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

Attachment: Proposed patch

Comment 18

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

https://tbpl.mozilla.org/?tree=Try&rev=4266a283cc93

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.)

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 24

[Ed Morley [:emorley]]

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

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Petruta Horea [:phorea], Desktop QA]

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.

Comment 29

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

(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.