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

RESOLVED FIXED

Status

()

Core
DOM: Animation
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: KWierso, Assigned: hiro)

Tracking

({intermittent-failure})

46 Branch
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox46 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Comment 1

3 years ago
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
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
Another restyling which is caused by div.remove() in each test can be observed.  We should wait for the paint caused by div.remove().

Comment 4

3 years ago
15 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 10
* b2g-inbound: 3
* mozilla-central: 1
* fx-team: 1

Platform breakdown:
* b2g-emu-ics: 15

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1241692&startday=2016-01-18&endday=2016-01-24&tree=all
(Assignee)

Comment 5

3 years ago
(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
(Assignee)

Comment 6

3 years ago
Created 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

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

Comment 7

3 years ago
Created attachment 8716038 [details]
MozReview Request: Bug 1241692 - Part 2: We should observe stylings only for animations. r?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)
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 11

3 years ago
(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
(Assignee)

Comment 12

3 years ago
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/
(Assignee)

Comment 13

3 years ago
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/
(Assignee)

Comment 14

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecbac888e7b3

Thank you, Daniel!
Keywords: checkin-needed, leave-open
(Assignee)

Comment 17

2 years ago
Closing since this has not occurred any more.
Status: NEW → RESOLVED
Last Resolved: 2 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.