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)
Core
CSS Parsing and Computation
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)
24.29 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
15.18 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
13.79 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
9.60 KB,
patch
|
hiro
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ 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•7 years ago
|
||
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•7 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.
Comment 3•7 years ago
|
||
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
Comment 4•7 years ago
|
||
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•7 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Updated•7 years ago
|
Blocks: stylo-crash-reports
Comment 5•7 years ago
|
||
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.
Blocks: stylo-pref-study
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Assignee | ||
Comment 6•7 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•7 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•7 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)
Assignee | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f30e5b3b75b206bfb20e2e979faae6163607a9c
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
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•7 years ago
|
||
Rebase: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1192082b37c3df74d238a76ad26c2f4857f8e59
Comment 15•7 years ago
|
||
Waiting for bug 1389385 to land, then will land this on top.
Comment 16•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8896510 -
Flags: review?(hikezoe)
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=521ddaa45e19ff0e2fb9247cea4464bf5bdbcff9
Assignee | ||
Comment 18•7 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
Comment 19•7 years ago
|
||
MozReview-Commit-ID: BirD8BDMifp
Attachment #8896525 -
Flags: review?(hikezoe)
Updated•7 years ago
|
Attachment #8896041 -
Attachment is obsolete: true
Attachment #8896041 -
Flags: review?(bobbyholley)
Updated•7 years ago
|
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•7 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 22•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52779d5b7df051ed0e05729b2886dabf3142f00a
Comment 23•7 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
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ad2ece50081 https://hg.mozilla.org/mozilla-central/rev/cbacd472fc22
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 25•7 years ago
|
||
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•7 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 27•7 years ago
|
||
Flags: needinfo?(hikezoe)
Attachment #8896800 -
Flags: review+
Assignee | ||
Comment 28•7 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•7 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•7 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 31•7 years ago
|
||
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+
Comment 32•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f3a8d9d0ccc8 https://hg.mozilla.org/releases/mozilla-beta/rev/4acb479ef69d
Comment 33•7 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•