Closed Bug 1241692 Opened 8 years ago Closed 8 years ago

Intermittent test_restyles.html | CSS animations running on the compositor should not update style on the main thread - got 3, expected +0

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Version:
46 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox46 --- affected

People

(Reporter: KWierso, Assigned: hiro)

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

MozReview Request: Bug 1241692 - Part 1: Ensure that element which is added in each test is removed before subsequent test is processed. r?dholbert
58 bytes, text/x-review-board-request
dholbert
: review+
Details
MozReview Request: Bug 1241692 - Part 2: We should observe stylings only for animations. r?dholbert
58 bytes, text/x-review-board-request
dholbert
: review+
Details 
The behavior of animations running on compositor on B2G and Android is something different other platforms.
I am going to investigate this failure reason but I may disable this test once we have mozinfo in mochitest (bug 1153126).
Assignee: nobody → hiikezoe
OK. I could reproduce this failure locally on Android emulator.
Neither waitForAllPaints nor waitForAllPaintsFlushed waits for all paints on Android emulator sometimes.
Replacing waitForAllPaints with setTimeout(, 1000) does solve the failure locally.

Some styling process (3 styling in this bug title case) still remain after the callback of waitForAllPaints is called.  We need to make waitForAllPaints reliable or another way to ensure that styling process has been processed.
Another restyling which is caused by div.remove() in each test can be observed.  We should wait for the paint caused by div.remove().
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Some styling process (3 styling in this bug title case) still remain after
> the callback of waitForAllPaints is called.  We need to make
> waitForAllPaints reliable or another way to ensure that styling process has
> been processed.

Two of three stylings are for ScrollbarFrame.  I guess one is for horizontal, another is for vertical.  scrolling="no" is specified [1], but it seems not to suppress the styling on Android (and B2G?).  And one of three styling is really for the animation in the first test.  If the two stylings have not been processed while waitForAllPaints, the two styling cause a styling update for the animation on the main-thread.  I am not sure it's correct behavior on Android/B2G.

[1] https://dxr.mozilla.org/mozilla-central/rev/d6d81655dd9e146c300a64c0fcaeb04ca3300a19/testing/mochitest/harness.xul#87
Sometimes stylings for removal an element added in prior test have not finished when sebsequent test starts.
We should wait for the removal the element before subsequent test starts.

Review commit: https://reviewboard.mozilla.org/r/33685/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33685/
Attachment #8716037 - Flags: review?(dholbert)
We don't need observe restylings other than animations.  If those restylings
happen, it's just noise for this test.  We should drop them.

Review commit: https://reviewboard.mozilla.org/r/33687/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33687/
Attachment #8716038 - Flags: review?(dholbert)
Hello, Daniel. Could you please take time to review them? Now B2G became Tier-3 platform, then, this failure is less frequent, but, of cource, we should fix this.  I am not 100% sure these two patches fix this failure completely, but it definitely improve it.
Comment on attachment 8716037 [details]
MozReview Request: Bug 1241692 - Part 1: Ensure that element which is added in each test is removed before subsequent test is processed. r?dholbert

https://reviewboard.mozilla.org/r/33685/#review30437

I'll admit to not being particularly experienced with promises/generators/yield.  But this looks sane enough, so r=me, with nits below addressed.

Several nits on the extended commit message (the part after the first line):
> Sometimes stylings for removal an element added in prior test
> have not finished when sebsequent test starts.

(1) Typo: s/seb/sub/ ("sebsequent" --> "subsequent")

(2) This sentence as a whole doesn't quite make sense to me. I think you mean to say something like:
 "Before this fix, sometimes an element which was removed in a prior test would still be visible when the subsequent test starts" 

Continuing:
> We should wait for the removal the element before subsequent test starts.

(3) I think you're missing a word here -- "the removal *of* the element".  ("of" is missing)

(4) But more importantly: we were already waiting for the removal of the element. The key thing that this patch adds is that we're now waiting for *paints to complete* after the element has been removed, yes? Maybe better to emphasize that here, rather than "removal of the element" (which we were already performing before starting the next test IIUC)

(Probably worth a Try run before landing, too, if you haven't run this through Try yet.)
Attachment #8716037 - Flags: review?(dholbert) → review+
Attachment #8716038 - Flags: review?(dholbert) → review+
Comment on attachment 8716038 [details]
MozReview Request: Bug 1241692 - Part 2: We should observe stylings only for animations. r?dholbert

https://reviewboard.mozilla.org/r/33687/#review30439

r=me on part 2, with review note below addressed.

::: dom/animation/test/chrome/test_restyles.html:50
(Diff revision 1)
> +               (marker.restyleHint == 'eRestyle_CSSAnimations' |

I think you mean "||", not "|" here, right?

"|" is bitwise or, "||" is logical or.
(In reply to Daniel Holbert [:dholbert] from comment #9)
> Comment on attachment 8716037 [details]
> MozReview Request: Bug 1241692 - Part 1: Ensure that element which is added
> in each test is removed before subsequent test is processed. r?dholbert
> 
> https://reviewboard.mozilla.org/r/33685/#review30437
> 
> I'll admit to not being particularly experienced with
> promises/generators/yield.  But this looks sane enough, so r=me, with nits
> below addressed.
> 
> Several nits on the extended commit message (the part after the first line):
> > Sometimes stylings for removal an element added in prior test
> > have not finished when sebsequent test starts.
> 
> (1) Typo: s/seb/sub/ ("sebsequent" --> "subsequent")
> 
> (2) This sentence as a whole doesn't quite make sense to me. I think you
> mean to say something like:
>  "Before this fix, sometimes an element which was removed in a prior test
> would still be visible when the subsequent test starts" 
> 
> Continuing:
> > We should wait for the removal the element before subsequent test starts.
> 
> (3) I think you're missing a word here -- "the removal *of* the element". 
> ("of" is missing)
> 
> (4) But more importantly: we were already waiting for the removal of the
> element. The key thing that this patch adds is that we're now waiting for
> *paints to complete* after the element has been removed, yes? Maybe better
> to emphasize that here, rather than "removal of the element" (which we were
> already performing before starting the next test IIUC)

That's exactly what I wanted to say in the comment! Thank you for the collections.

> (Probably worth a Try run before landing, too, if you haven't run this
> through Try yet.)

A try is now running. Though part 2 patch in the try has still the mistake in if statement, there are no failures yet.  That means the nose what I am concerned does not appear in the try yet.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0553ba074c1a
Comment on attachment 8716037 [details]
MozReview Request: Bug 1241692 - Part 1: Ensure that element which is added in each test is removed before subsequent test is processed. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33685/diff/1-2/
Comment on attachment 8716038 [details]
MozReview Request: Bug 1241692 - Part 2: We should observe stylings only for animations. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33687/diff/1-2/
Closing since this has not occurred any more.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: