Closed Bug 1324691 Opened 7 years ago Closed 7 years ago

stylo: several tests non-fatally assert with "stylo: Servo-backed style system should not be using EffectCompositor" and "stylo: ServoRestyleManager does not support animations yet"

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla54

Tracking Flags:

Tracking

Status

firefox54

---

fixed

People

(Reporter: heycam, Assigned: boris)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1324691 - Part 1: Use warnings instead of assertions for unsupported OMTA. 7 years ago Boris Chiou [:boris] 59 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1324691 - Part 2: Move AnimationsWithDestroyedFrame into RestyleManagerBase. 7 years ago Boris Chiou [:boris] 59 bytes, text/x-review-board-request	hiro : review+	Details
Bug 1324691 - Part 3: Support AnimationsWithDestroyedFrame for ServoRestyleManager. 7 years ago Boris Chiou [:boris] 59 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1324691 - Part 4: Remove conditions on some crashtests. 7 years ago Boris Chiou [:boris] 59 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1324691 followup - Remove the last condition on crashtest of this bug. 7 years ago Xidorn Quan [:xidorn] UTC+11 59 bytes, text/x-review-board-request	xidorn : review+	Details

Cameron McCormack (:heycam)

Reporter

Description

•

7 years ago

###!!! ASSERTION: stylo: Servo-backed style system should not be using EffectCompositor: 'Error', file /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp, line 279
###!!! ASSERTION: stylo: ServoRestyleManager does not support animations yet: 'Error', file /home/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 710

  dom/animation/test/crashtests/1216842-3.html
  dom/animation/test/crashtests/1216842-4.html
  dom/animation/test/crashtests/1278485-1.html

Boris Chiou [:boris]

Assignee

Updated

•

7 years ago

Assignee: nobody → boris.chiou

Boris Chiou [:boris]

Assignee

Comment 1

•

7 years ago

OK, I can remove these NS_ERROR after Bug 1317209 is merged.

Boris Chiou [:boris]

Assignee

Comment 2

•

7 years ago

1. ASSERTION: stylo: Servo-backed style system should not be using EffectCompositor
 * We need to support animation on compositor. However, I think that needs much effort, so we can create a bug for that
   and use NS_WARNING, instead of NS_ERROR.

2. ASSERTION: stylo: ServoRestyleManager does not support animations yet
 * This is to destroy frames because no frame for the element (i.e. display: none). I am trying to do the same thing
   for ServoRestyleManager.

Boris Chiou [:boris]

Assignee

Updated

•

7 years ago

Status: NEW → ASSIGNED

Boris Chiou [:boris]

Assignee

Updated

•

7 years ago

Comment 7

•

7 years ago

mozreview-review

Comment on attachment 8831052 [details]
Bug 1324691 - Part 1: Use warnings instead of assertions for unsupported OMTA.

https://reviewboard.mozilla.org/r/107706/#review108846

Attachment #8831052 - Flags: review?(cam) → review+

Cameron McCormack (:heycam)

Reporter

Comment 8

•

7 years ago

mozreview-review

Comment on attachment 8831054 [details]
Bug 1324691 - Part 3: Support AnimationsWithDestroyedFrame for ServoRestyleManager.

https://reviewboard.mozilla.org/r/107710/#review108848

::: layout/base/ServoRestyleManager.cpp:325
(Diff revision 1)
>    }
>  
> +  // Create a AnimationsWithDestroyedFrame during restyling process to
> +  // stop animations and transitions on elements that have no frame at the end
> +  // of the restyling process.
> +  RestyleManagerBase::AnimationsWithDestroyedFrame

You should be able to remove the "RestyleManagerBase::", since AnimationsWithDestroyedFrame should be in scope here.

Attachment #8831054 - Flags: review?(cam) → review+

Cameron McCormack (:heycam)

Reporter

Comment 9

•

7 years ago

mozreview-review

Comment on attachment 8831055 [details]
Bug 1324691 - Part 4: Remove conditions on some crashtests.

https://reviewboard.mozilla.org/r/107712/#review108852

Attachment #8831055 - Flags: review?(cam) → review+

Hiroyuki Ikezoe (:hiro)

Comment 10

•

7 years ago

mozreview-review

Comment on attachment 8831053 [details]
Bug 1324691 - Part 2: Move AnimationsWithDestroyedFrame into RestyleManagerBase.

https://reviewboard.mozilla.org/r/107708/#review108854

Looks good to me!
I am really hoping MozReview can track moving code in different files.

Attachment #8831053 - Flags: review?(hikezoe) → review+

Boris Chiou [:boris]

Assignee

Comment 11

•

7 years ago

Note: We only stop CSS animations/transitions and compositor-driven animations that have no frames [1]. (So I cannot test this feature by script animations now.)

[1] http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/layout/base/RestyleManager.cpp#229-230

Comment hidden (mozreview-request)

Comment on attachment 8831054 [details]
Bug 1324691 - Part 3: Support AnimationsWithDestroyedFrame for ServoRestyleManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107710/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8831055 [details]
Bug 1324691 - Part 4: Remove conditions on some crashtests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107712/diff/1-2/

Boris Chiou [:boris]

Assignee

Comment 14

•

7 years ago

Oops, got these failed. I should fix the assertion numbers in part 4. (Reducing the number is so happy. :))

REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1244595-1.html | assertion count 2 is less than expected 3 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1216842-1.html | assertion count 0 is less than expected 3 to 10 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1216842-2.html | assertion count 0 is less than expected 3 to 10 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1216842-5.html | assertion count 0 is less than expected 4 to 10 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1216842-6.html | assertion count 0 is less than expected 4 to 10 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1277272-1.html | assertion count 10 is less than expected 31 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1290535-1.html | assertion count 0 is less than expected 2 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/layout/style/crashtests/1282076-2.html | assertion count 2 is less than expected 4 to 64 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/layout/style/crashtests/1290994-1.html | assertion count 0 is less than expected 2 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/layout/style/crashtests/1290994-2.html | assertion count 0 is less than expected 2 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/layout/style/crashtests/1290994-3.html | assertion count 0 is less than expected 1 assertions

Cameron McCormack (:heycam)

Reporter

Comment 15

•

7 years ago

That's OK, I'll fix them up when merging.

Boris Chiou [:boris]

Assignee

Comment 16

•

7 years ago

(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #15)
> That's OK, I'll fix them up when merging.

OK, Thanks.

Comment hidden (obsolete)

try stylo: https://treeherder.mozilla.org/#/jobs?repo=try&revision=364b57c710c3&selectedJob=72494786

Comment hidden (mozreview-request)

Comment on attachment 8831052 [details]
Bug 1324691 - Part 1: Use warnings instead of assertions for unsupported OMTA.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107706/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8831053 [details]
Bug 1324691 - Part 2: Move AnimationsWithDestroyedFrame into RestyleManagerBase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107708/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8831054 [details]
Bug 1324691 - Part 3: Support AnimationsWithDestroyedFrame for ServoRestyleManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107710/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8831055 [details]
Bug 1324691 - Part 4: Remove conditions on some crashtests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107712/diff/2-3/

Boris Chiou [:boris]

Assignee

Updated

•

7 years ago

Blocks: 1324690

Comment hidden (mozreview-request)

Comment on attachment 8831055 [details]
Bug 1324691 - Part 4: Remove conditions on some crashtests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107712/diff/3-4/

Boris Chiou [:boris]

Assignee

Comment 23

•

7 years ago

try stylo: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cdfdb0c5e33c3d852b09bee8217745418319946

Pulsebot

Comment 24

•

7 years ago

Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9734ac70df3f
Part 1: Use warnings instead of assertions for unsupported OMTA. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e2d79699222d
Part 2: Move AnimationsWithDestroyedFrame into RestyleManagerBase. r=hiro
https://hg.mozilla.org/integration/autoland/rev/b401d8f83eb1
Part 3: Support AnimationsWithDestroyedFrame for ServoRestyleManager. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e6f1e92c7afb
Part 4: Remove conditions on some crashtests. r=heycam

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 25

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/9734ac70df3f
https://hg.mozilla.org/mozilla-central/rev/e2d79699222d
https://hg.mozilla.org/mozilla-central/rev/b401d8f83eb1
https://hg.mozilla.org/mozilla-central/rev/e6f1e92c7afb

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox54: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla54

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/178222/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178222/

Xidorn Quan [:xidorn] UTC+11

Comment 29

•

7 years ago

mozreview-review

Comment on attachment 8906469 [details]
Bug 1324691 followup - Remove the last condition on crashtest of this bug.

https://reviewboard.mozilla.org/r/178222/#review183118

Attachment #8906469 - Flags: review+

Pulsebot

Comment 30

•

7 years ago

Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e429e3f2b3f
followup - Remove the last condition on crashtest of this bug. r=xidorn

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 31

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/2e429e3f2b3f

You need to log in before you can comment on or make changes to this bug.