Closed
Bug 1418867
Opened 6 years ago
Closed 6 years ago
stylo: Crash in core::option::expect_failed | geckoservo::glue::Servo_StyleSet_GetBaseComputedValuesForElement
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
(Blocks 2 open bugs)
Details
(Keywords: crash, regression, reproducible)
Crash Data
Attachments
(6 files, 1 obsolete file)
749 bytes,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
41 bytes,
text/x-github-pull-request
|
Details | Review |
+++ This bug was initially created as a clone of Bug #1418059 +++ This bug was filed from the Socorro interface and is report bp-d5ef4bab-f8c7-411b-b8be-7b5960171116. ============================================================= I am going to fix CSS animation cases in bug 1418059. Most crashes in the wild should be fixed by bug 1418059. Apart form that I am going to fix script animations cases in this bug.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89f7570b8e15c8c519f0faef6ad06790a9580794
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Comment hidden (obsolete) |
Assignee | ||
Comment 3•6 years ago
|
||
I believe this is not a topcrash, the topcrash one is bug 1418059. After landing bug 1418059, there is only one crash report (bp-5302afea-6f3d-44ec-89b6-cd47d0171120). I believe the one was reported by Emilio with a crash test similar to the one in comment 0. Use comment in the crash report is 'lol, that crashtest works :O'. :)
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3) > I believe this is not a topcrash, the topcrash one is bug 1418059. After > landing bug 1418059, there is only one crash report > (bp-5302afea-6f3d-44ec-89b6-cd47d0171120). I believe the one was reported > by Emilio with a crash test similar to the one in comment 0. Use comment in > the crash report is 'lol, that crashtest works :O'. :) Oops, the test case should look like the one in bug 1418902 comment 1.
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
A new try based on bug 1419221. https://treeherder.mozilla.org/#/jobs?repo=try&revision=95abb36b2c654abdd6b9ffe3bd94492062eb0be2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8930400 [details] Bug 1418867 - Pass element or pseudo element to Servo_StyleSet_GetBaseComputedValuesForElement(). https://reviewboard.mozilla.org/r/201554/#review206808 ::: dom/animation/KeyframeEffectReadOnly.cpp:563 (Diff revision 1) > if (!hasAdditiveValues) { > return; > } > > if (!aBaseStyleContext) { > + Element* animatingElement = Just curious, is there a bug on file to remove the `mPseudoElement` from the animation target and just use the properties on them when the old style system is gone? We should just avoid recreating them each time we reframe, I'd think. ::: servo/ports/geckolib/glue.rs:809 (Diff revision 1) > return computed_values.clone_arc().into(); > } > > let element = GeckoElement(element); > > - let element_data = match element.borrow_data() { > + if let Some(pseudo) = element.implemented_pseudo_element() { Let's just remove this whole block, I think, to make everything more consistent.
Attachment #8930400 -
Flags: review?(emilio) → review+
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8930401 [details] Bug 1418867 - Fall back to re-resolve style if the parent element has no style data for the given pseudo element. https://reviewboard.mozilla.org/r/201556/#review206812 r=me even though this patch is not needed with the comments addressed of the first. If you disagree re. being worth optimising (accounting that this makes slower the non-pseudo case), you can land this with r=me, using `pseudo_element_originating_element` instead of `traversal_parent`. But again, I really think it's not worth the churn :) ::: servo/ports/geckolib/glue.rs:809 (Diff revision 1) > return computed_values.clone_arc().into(); > } > > let element = GeckoElement(element); > > if let Some(pseudo) = element.implemented_pseudo_element() { This is not needed if you actually remove the block :) I honestly don't think it's worth optimizing for this case, this function isn't even _that_ hot I'd think.
Attachment #8930401 -
Flags: review?(emilio) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8930402 [details] Bug 1418867 - Crash test for the case where the parent element has no style data for pseudo. https://reviewboard.mozilla.org/r/201558/#review206818 (I'm assuming the commit message was missing an `r` :)) ::: layout/style/crashtests/1418867.html:8 (Diff revision 1) > +<style> > +@keyframes anim { > + to { transform: rotate(360deg); } > +} > +.document-ready div::after { > + display: none; Can you also add a test for the `content: none` case from yesterday's test?
Attachment #8930402 -
Flags: review+
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #12) > Comment on attachment 8930401 [details] > Bug 1418867 - Fall back to re-resolve style if the parent element has no > style data for the given pseudo element. > > https://reviewboard.mozilla.org/r/201556/#review206812 > > r=me even though this patch is not needed with the comments addressed of the > first. > > If you disagree re. being worth optimising (accounting that this makes > slower the non-pseudo case), you can land this with r=me, using > `pseudo_element_originating_element` instead of `traversal_parent`. > > But again, I really think it's not worth the churn :) OK, if you are OK, I will drop the code, honestly I like this optimization though. (When I first saw the code I didn't understand what the purpose is, but after I realized the the optimization I was impressed with the gimmick.) (In reply to Emilio Cobos Álvarez [:emilio] from comment #13) > Comment on attachment 8930402 [details] > Bug 1418867 - Crash test for the case where the parent element has no style > data for pseudo. ?emilio > > https://reviewboard.mozilla.org/r/201558/#review206818 > > (I'm assuming the commit message was missing an `r` :)) > > ::: layout/style/crashtests/1418867.html:8 > (Diff revision 1) > > +<style> > > +@keyframes anim { > > + to { transform: rotate(360deg); } > > +} > > +.document-ready div::after { > > + display: none; > > Can you also add a test for the `content: none` case from yesterday's test? I will add the test case in bug 1418902.
Comment 15•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14) > OK, if you are OK, I will drop the code, honestly I like this optimization > though. (When I first saw the code I didn't understand what the purpose is, > but after I realized the the optimization I was impressed with the gimmick.) Just for the record, I just added it because the alternative was hard (since I didn't have the pseudo-element handy, so I'd have to add another special path to push_applicable_declarations or what not, etc...)
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8930398 [details] Bug 1418867 - Drop pseudo type argument from KeyframeEffectReadOnly::EnsureBaseStyle(). https://reviewboard.mozilla.org/r/201550/#review207150
Attachment #8930398 -
Flags: review?(bbirtles) → review+
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8930399 [details] Bug 1418867 - getUnanimatedComputedStyle throws an exception for non-existent pseudo element. https://reviewboard.mozilla.org/r/201552/#review207154 ::: dom/base/test/file_domwindowutils_animation.html:133 (Diff revision 1) > > if (utils.isStyledByServo) { > + SimpleTest.doesThrow( > + () => utils.getUnanimatedComputedStyle(div, "::before", "opacity", utils.FLUSH_NONE), > + "NS_ERROR_FAILURE", > + "Inexistent pseudo should throw"); Nit: Non-existent
Attachment #8930399 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 18•6 years ago
|
||
The test case sometimes causes an assertion on old style system. I am going to skip the test on old style system here. Filed bug 1419641 for the assertion.
Assignee | ||
Comment 19•6 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8930401 -
Attachment is obsolete: true
Assignee | ||
Comment 24•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8930400 [details] Bug 1418867 - Pass element or pseudo element to Servo_StyleSet_GetBaseComputedValuesForElement(). https://reviewboard.mozilla.org/r/201554/#review206808 > Just curious, is there a bug on file to remove the `mPseudoElement` from the animation target and just use the properties on them when the old style system is gone? > > We should just avoid recreating them each time we reframe, I'd think. Filed bug 1419651 now.
Comment 25•6 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4868b9f25718 Drop pseudo type argument from KeyframeEffectReadOnly::EnsureBaseStyle(). r=birtles https://hg.mozilla.org/integration/autoland/rev/51ab3c3c9935 getUnanimatedComputedStyle throws an exception for non-existent pseudo element. r=birtles https://hg.mozilla.org/integration/autoland/rev/0fe9ca39473a Pass element or pseudo element to Servo_StyleSet_GetBaseComputedValuesForElement(). r=emilio https://hg.mozilla.org/integration/autoland/rev/00a08154f505 Crash test for the case where the parent element has no style data for pseudo. r=emilio
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4868b9f25718 https://hg.mozilla.org/mozilla-central/rev/51ab3c3c9935 https://hg.mozilla.org/mozilla-central/rev/0fe9ca39473a https://hg.mozilla.org/mozilla-central/rev/00a08154f505
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•