Closed Bug 1388031 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug, )

Details

(Keywords: crash)

Crash Data

Attachments

(4 files, 3 obsolete files)

+++ 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)
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.
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: nobody → hikezoe
Status: NEW → ASSIGNED
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.
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.
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 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)
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.
Attached 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)
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)
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+
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
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)
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.
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?
(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*.
(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.