1388031 - stylo: crash in geckoservo::glue::Servo_ResolveStyle caused by FlushThrottledStyles

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Hiroyuki Ikezoe (:hiro)]

+++ This bug was initially created as a clone of Bug #1382568 +++
From bug 1382568 comment 50

(In reply to Calixte Denizet (:calixte) from comment #50)
> There still are 14 crashes in 57 with buildid >= 20170806100257 ([1]) for
> signature 'core::option::expect_failed |
> geckoservo::glue::Servo_ResolveStyle'.
> 
> [1]
> https://crash-stats.mozilla.com/search/
> ?signature=%3Dcore%3A%3Aoption%3A%3Aexpect_failed%20%7C%20geckoservo%3A%3Aglu
> e%3A%3AServo_ResolveStyle&build_id=%3E%3D20170806100257&product=Firefox&versi
> on=57.0a1&date=%3E%3D2017-07-31T11%3A16%3A46.000Z&date=%3C2017-08-
> 07T11%3A16%3A46.000Z&_sort=-
> date&_facets=url&_facets=install_time&_facets=version&_facets=build_id&_facet
> s=platform_pretty_version&_columns=date&_columns=signature&_columns=product&_
> columns=version&_columns=build_id&_columns=platform#crash-reports

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Hiro, looks like an animation is causing a reframe, and we're trying to resolve the styles of an unstyled element from there. Any idea other than "traverse unstyled children during animation traversals"?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I think a proper solution for throttled animations is to restyle only transform and opacity animations there. Those properties don't cause reframe at all I think. But yeah, it will need some amount of work. So I have no idea other than traverse unstyled children for now.  Keep ni? to me.

Comment 3

[Bobby Holley (:bholley)]

Clearing state that was inherited from the original bug and that may not be valid anymore, feel free to reset any valid state.

Comment 4

[Bobby Holley (:bholley)]

Removing crashes from crash-stats (i.e. where we don't have somebody filing with STR) from stylo-site-issues, we can track crash-stats separately.

Comment 5

[Chris Peterson [:cpeterson]]

This is the top Stylo crash in Nightly 57. We should uplift this fix to Beta 56 before we start the Stylo experiment for any Beta users.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I am currently suspecting that the reframe which causes this crash is not coming from animation because

1) Throttled animation (opacity and transform) does not cause nsChangeHint_ReconstructFrame
2) It's really really hard to make pending animations between restyling and PresShell::HandleEvent(). Moreover there several properties that cause nsChangeHint_ReconstructFrame, they are rare.

So, I now think making throttled animation flush only for opacity and transform will not solve this crash. I think the nsChangeHint_ReconstructFrame in question comes from ServoRestyleManager::AttributeChanged() (I've been trying to write a test case for it but never succeeded) *or* HTMLInputElement::EnablePreview(). 

Anyway, I will take the approach to traverse unstyled elements in animation-only restyle.  One unhappy thing about the approach is that we need to create SequentialTask for creating CSS animations because traversing unstyled elements means that we process initial styling in animation-only restyle, thus we need to create CSS animations.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Forgot to put a try link;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb5b742ddf49c11376152f8bd52344c54dc82d17

The failure of test_value_computation.html is bug 1389041 since the try was based on a revision of autoland.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1388031 - Process normal traversal for throttled animation flush as well.

Traversing unstyled elements in animation-only restyle means that initial
styling is processed in the animation-only restyle. That means that we need to
create SequentialTask for creating CSS animations (whereas we don't need to
create SequentialTask for creating CSS transitions since CSS transition is not
triggered by initial style). We don't need to create other SequentialTask (i.e.
EFFECT_PROPERTIES and CASCADE_RESULTS) for the initial style either since it's
for updating existing animations on the element.

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896041 [details]
Bug 1388031 - Process normal traversal for throttled animation flush as well.

As per discussion with Bobby and Brian on IRC, we'd take another approach to not skip normal traversal for throttled animation flush.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f30e5b3b75b206bfb20e2e979faae6163607a9c

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1388031 - Cleanup code that was used for verifying styling results for throttled animation flush in post traversal.

Now that we do process normal traversal even in the case of throttled animation
flush so that we don't need to do special handling for the case.

Note about the comment in has_current_styles():
the remaining animation hints is not caused by either this patch or the
previous patch in this patch series, it's been there in the first place, but
it should be fixed somehow later.
Below is the tests that leave animation hints when an animating element is
moved into subtree of a contenteditable element.

 layout/style/crashtests/1383319.html
 layout/style/crashtests/1383001.html

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896041 [details]
Bug 1388031 - Process normal traversal for throttled animation flush as well.

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

Comment 13

[Bobby Holley (:bholley)]

These patches look like great cleanup!

I just bitrotted them with the other stuff I'm landing. I feel bad about that so I'm going to rebase them for you. :-)

Comment 14

[Bobby Holley (:bholley)]

Rebase: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1192082b37c3df74d238a76ad26c2f4857f8e59

Comment 15

[Bobby Holley (:bholley)]

Waiting for bug 1389385 to land, then will land this on top.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Adendum to Part 1.

It seems like this should be added to Part 1, since otherwise we'll always
have the AnimationOnly flag set, and thus never do a non-animation traversal
on the servo side.

MozReview-Commit-ID: EVSp6UiHLoW

Comment 17

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=521ddaa45e19ff0e2fb9247cea4464bf5bdbcff9

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896510 [details] [diff] [review]
Adendum to Part 1.

Review of attachment 8896510 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ -862,5 @@
> -  if (!!(aBaseFlags & ServoTraversalFlags::AnimationOnly)) {
> -    PreTraverse(nullptr, EffectCompositor::AnimationRestyleType::Full);
> -  } else {
> -    PreTraverse();
> -  }

I think this is overkill. Without AnimationRestyleType::Full, we do never unthrottle *throttled* animation.  See how the flag works in EffectCompositor::PreTraverseInSubtree().

https://hg.mozilla.org/mozilla-central/file/80ff3f300e05/dom/animation/EffectCompositor.cpp#l986

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 1 - Process normal traversal for throttled animation flush as well.

MozReview-Commit-ID: BirD8BDMifp

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 2 - Cleanup code that was used for verifying styling results for throttled animation flush in post traversal. r=bholley

MozReview-Commit-ID: JojHufxNCiS

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896525 [details] [diff] [review]
Part 1 - Process normal traversal for throttled animation flush as well.

Review of attachment 8896525 [details] [diff] [review]:
-----------------------------------------------------------------

Great!  Thanks for quick update!

Comment 22

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52779d5b7df051ed0e05729b2886dabf3142f00a

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ad2ece50081
Process normal traversal for throttled animation flush as well. r=bholley
https://hg.mozilla.org/integration/autoland/rev/cbacd472fc22
Cleanup code that was used for verifying styling results for throttled animation flush in post traversal. r=bholley

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/6ad2ece50081
https://hg.mozilla.org/mozilla-central/rev/cbacd472fc22

Comment 25

[Chris Peterson [:cpeterson]]

Hiro, this is our top Stylo crash in Nightly 57. We should uplift your fix to the Beta 56 channel before we start our Stylo experiment for Beta users.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

A try with the patches rebased on beta branch.

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

I will request uplift after I confirmed the try result is good.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1 - Process normal traversal for throttled animation flush as well. for mozilla-beta

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2 - Cleanup code that was used for verifying styling results for throttled animation flush in post traversal for mozilla-beta

Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]: Crash happens if user enabled stylo (i.e. for stylo experiment for beta users)
[Is this code covered by automated tests?]: We have no test cases for this crash since we don't know yet how to reproduce this crash, but we have some test cases 
that cover this change (layout/style/crashtests/{1371450-1.html, 1378064-1.html, 1383589-1.html, 1381420-1.html}).
[Has the fix been verified in Nightly?]: Patches have been already landed in mozilla-central, but nightly hasn't built yet.
[Needs manual test from QE? If yes, steps to reproduce]: We have no reliable way to reproduce this crash unfortunately.
[List of other uplifts needed for the feature/fix]: Part 1 patch here (attachment 8896800 [details] [diff] [review])
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: These patches changed the styling behavior for event handling. Before these patches we did special handling for the events but after these patches we do the same thing what we do for normal styling.  So, in theory, the crash caused by event handling (i.e. FlushThrottledStyles) should not happen any more. (If it happened it's another cause)
[String changes made/needed]: None

Comment 29

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> Created attachment 8896801 [details] [diff] [review]
> Part 2 - Cleanup code that was used for verifying styling results for
> throttled animation flush in post traversal for mozilla-beta
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: None
> [User impact if declined]: Crash happens if user enabled stylo (i.e. for
> stylo experiment for beta users)
> [Is this code covered by automated tests?]: We have no test cases for this
> crash since we don't know yet how to reproduce this crash, but we have some
> test cases 
> that cover this change (layout/style/crashtests/{1371450-1.html,
> 1378064-1.html, 1383589-1.html, 1381420-1.html}).
> [Has the fix been verified in Nightly?]: Patches have been already landed in
> mozilla-central, but nightly hasn't built yet.
> [Needs manual test from QE? If yes, steps to reproduce]: We have no reliable
> way to reproduce this crash unfortunately.
> [List of other uplifts needed for the feature/fix]: Part 1 patch here
> (attachment 8896800 [details] [diff] [review])
> [Is the change risky?]: Low risk
> [Why is the change risky/not risky?]: These patches changed the styling
> behavior for event handling. Before these patches we did special handling
> for the events but after these patches we do the same thing what we do for
> normal styling.  So, in theory, the crash caused by event handling (i.e.
> FlushThrottledStyles) should not happen any more. (If it happened it's
> another cause)
> [String changes made/needed]: None

I did forget mentioning an important thing.  These patches affect only for *stylo*.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> Created attachment 8896801 [details] [diff] [review]
> Part 2 - Cleanup code that was used for verifying styling results for
> throttled animation flush in post traversal for mozilla-beta
> 
> Approval Request Comment
> [Has the fix been verified in Nightly?]: Patches have been already landed in
> mozilla-central, but nightly hasn't built yet.

Correction. The latest nightly (buildid: 20170813183258) has already included these patches.

Comment 31

[Gerry Chang [:gchang]]

Comment on attachment 8896801 [details] [diff] [review]
Part 2 - Cleanup code that was used for verifying styling results for throttled animation flush in post traversal for mozilla-beta

Fix a stylo crash. Beta56+. Should be in 56.0b3.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f3a8d9d0ccc8
https://hg.mozilla.org/releases/mozilla-beta/rev/4acb479ef69d

Comment 33

[Marcia Knous [:marcia]]

There appears to be a few crashes on Nightly after this landed (using 20170814100258), see Bug 1390179. That stack does have some "flush" references in it and is only reproducible with stylo enabled.