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

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: whimboo, Assigned: birtles)

Tracking

({regression})

33 Branch
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 unaffected, firefox33+ verified, firefox34+ verified)

Details

(Whiteboard: [pdfjs-c-ux], )

Attachments

(2 attachments, 3 obsolete attachments)

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.
OS: Linux → All
Hardware: x86_64 → All
Summary: PDF loading indicator is jumping back and forth → PDF loading indicator is jumping back and forth (flickering) while loading the file
[Tracking Requested - why for this release]:
PDF.js is an important component. Tracking.

Yuri, does it ring a bell?
Flags: needinfo?(ydelendik)
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
Flags: needinfo?(ydelendik) → needinfo?(birtles)
Component: PDF Viewer → CSS Parsing and Computation
Product: Firefox → Core
Whiteboard: [pdfjs-c-ux]
Hmm, it's weird that I can able to replicate that when attachment 8467828 [details] is saved locally and opened from the file system.
Attachment #8467828 - Attachment description: Minimal test case to replicate the issue → Minimal test case to replicate the issue (save on the file system)
In local build:
Last Good: 2d45653b217d
First Bad: 31695984cfe2
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.
Flags: needinfo?(birtles)
Posted patch Proposed patch (obsolete) — Splinter Review
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.)
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Posted patch Proposed patch (obsolete) — Splinter Review
(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
Attachment #8471338 - Attachment is obsolete: true
(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.
Attachment #8471420 - Flags: review?(dbaron)
Attachment #8471351 - Attachment is obsolete: true
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.
Attachment #8471420 - Flags: review?(dbaron) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/3d8dd1441e53
https://hg.mozilla.org/mozilla-central/rev/ac59c5f851dc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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
Attachment #8471420 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
Keywords: verifyme
Attachment #8471420 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
(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.
You need to log in before you can comment on or make changes to this bug.