Closed Bug 1418867 Opened 5 years ago Closed 5 years ago

stylo: Crash in core::option::expect_failed | geckoservo::glue::Servo_StyleSet_GetBaseComputedValuesForElement

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
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)

A crash test
749 bytes, text/html
Details
Bug 1418867 - Drop pseudo type argument from KeyframeEffectReadOnly::EnsureBaseStyle().
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1418867 - getUnanimatedComputedStyle throws an exception for non-existent pseudo element.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1418867 - Pass element or pseudo element to Servo_StyleSet_GetBaseComputedValuesForElement().
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1418867 - Crash test for the case where the parent element has no style data for pseudo.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Servo PR
41 bytes, text/x-github-pull-request
Details | Review
Attached file A crash test — 
+++ 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: nobody → hikezoe
No longer depends on: 1418059
See Also: → 1418059
Status: NEW → ASSIGNED
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'. :)
(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.
tracking-firefox59: ? → ---
Keywords: topcrash
Priority: P1 → P3
Depends on: 1419221
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 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 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+
(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.
(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 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 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+
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.
Attached file Servo PR — 

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8930401 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  
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

          You need to log in
          before you can comment on or make changes to this bug.