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

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks 1 bug, {crash})

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 fixed, firefox57 fixed)

Details

(crash signature, URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
+++ 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
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"?
Flags: needinfo?(hikezoe)
(Assignee)

Comment 2

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

Updated

2 years ago
No longer depends on: 1382568
Flags: needinfo?(hikezoe)
Clearing state that was inherited from the original bug and that may not be valid anymore, feel free to reset any valid state.
No longer blocks: stylo-pref-study, 1384162
Keywords: topcrash
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.
No longer blocks: stylo-site-issues
(Assignee)

Updated

2 years ago
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
See Also: → 1388692
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.
(Assignee)

Comment 6

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

Comment 7

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
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.
Attachment #8896041 - Flags: review?(emilio+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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. :-)
Blocks: 1389681
Depends on: 1389385
Waiting for bug 1389385 to land, then will land this on top.
Posted patch Adendum to Part 1. (obsolete) — Splinter Review
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
Attachment #8896510 - Flags: review?(hikezoe)
(Assignee)

Comment 18

2 years ago
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
MozReview-Commit-ID: BirD8BDMifp
Attachment #8896525 - Flags: review?(hikezoe)
Attachment #8896041 - Attachment is obsolete: true
Attachment #8896041 - Flags: review?(bobbyholley)
Attachment #8896251 - Attachment is obsolete: true
Attachment #8896510 - Attachment is obsolete: true
Attachment #8896251 - Flags: review?(bobbyholley)
Attachment #8896510 - Flags: review?(hikezoe)
(Assignee)

Comment 21

2 years ago
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!
Attachment #8896525 - Flags: review?(hikezoe) → review+

Comment 23

2 years ago
Pushed by bholley@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/6ad2ece50081
https://hg.mozilla.org/mozilla-central/rev/cbacd472fc22
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
Flags: needinfo?(hikezoe)
(Assignee)

Comment 26

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

Comment 28

2 years ago
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
Attachment #8896801 - Flags: review+
Attachment #8896801 - Flags: approval-mozilla-beta?
(Assignee)

Comment 29

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

Comment 30

2 years ago
(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 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.
Attachment #8896801 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
See Also: → 1390179
You need to log in before you can comment on or make changes to this bug.