1401825 - stylo: Assertion failure: elem->GetPseudoElementType() == aFrame->StyleContext()->GetPseudoType(), at /builds/worker/workspace/build/src/layout/base/nsPresContext.cpp:2242

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev f8dd3f21e434.

Comment 1

[Jet Villegas (inactive)]

Louis: As you work on appearance, can you fire this up in a debugger and report back? We may need to relax this assert or reconcile the style vs. element discrepancy. Thx!

Comment 2

[Louis Chang [:louis]]

Ok, I will look into it.

Comment 3

[Louis Chang [:louis]]

Hi Jason,
I could not reproduce the assertion on my Mac, is it only happen on Linux? Could you be more specific, thanks.

Comment 4

[Jason Kratzer [:jkratzer]]

(In reply to Louis Chang [:louis] from comment #3)
> Hi Jason,
> I could not reproduce the assertion on my Mac, is it only happen on Linux?
> Could you be more specific, thanks.

Louis, this was tested on a linux64 system.  Unfortunately, I do not have access to a Mac at this time to verify if it reproduces there.

Comment 5

[Jason Kratzer [:jkratzer]]

Attachment: Minidump stack trace

Attached minidump stack trace.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

I can reproduce on both Win10 and Ubuntu 17.04 with Stylo enabled. Bisection goes all the back to when we first started building Stylo by default, though :(

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

I can reproduce this assertion on Windows, and looking at the code path, I think this is somehow platform-dependent. Different platform may have different rules for whether a widget should be considered themed.

The situation here is that, aFrame is a bullet frame, which corresponds to the ::-moz-list-bullet, while elem is the <li> element the bullet frame belongs to.

The assertion there assumes that if we are querying a pseudo element, the pseudo element must have its own content node. However, that is basically not true for many of pseudo-elements.

Looking at code after the assertion, I think the assertion is put there basically because Servo_HasAuthorSpecifiedRules doesn't support pseudo element which don't have their own content node.

So there are two solutions to this bug:
* avoid calling into nsPresContext::HasAuthorSpecifiedRules from nsNativeTheme::IsWidgetStyled when the frame is for a pseudo element;
* fix Servo_HasAuthorSpecifiedRules to have it work for all cases.

Digging a bit into the code, I quite believe the second approach is doable, because we already have the rule node pointer in computed style.

Taking.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1401825 - Support pseudo-element properly in nsPresContext::HasAuthorSpecifiedRules.

Review commit: https://reviewboard.mozilla.org/r/187940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187940/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8916898 [details]
Bug 1401825 - Support pseudo-element properly in nsPresContext::HasAuthorSpecifiedRules.

https://reviewboard.mozilla.org/r/187940/#review193038

r=me

::: layout/base/nsPresContext.cpp:2256
(Diff revision 1)
>                                            aRuleTypeMask,
>                                            UseDocumentColors());
>    }
>    Element* elem = aFrame->GetContent()->AsElement();
>  
> -  MOZ_ASSERT(elem->GetPseudoElementType() ==
> +  if (elem->GetPseudoElementType() != CSSPseudoElementType::NotPseudo) {

A comment here would be nice, to note that we need to handle non-generated content pseudos too.

::: layout/base/nsPresContext.cpp:2258
(Diff revision 1)
>    }
>    Element* elem = aFrame->GetContent()->AsElement();
>  
> -  MOZ_ASSERT(elem->GetPseudoElementType() ==
> -             aFrame->StyleContext()->GetPseudoType());
> -  if (elem->HasServoData()) {
> +  if (elem->GetPseudoElementType() != CSSPseudoElementType::NotPseudo) {
> +    elem = elem->GetParent()->AsElement();
> +    MOZ_ASSERT(elem, "Pseudo element has no parent element?");

At this point you have already dereferenced it in debug builds, so probably this assertion is not that worth (you'd have crashed if it would fail).

Or, maybe move it before the assignment to look like:

```
MOZ_ASSERT(elem->GetParent() && elem->GetParent()->IsElement());
```

::: layout/base/nsPresContext.cpp:2259
(Diff revision 1)
>    Element* elem = aFrame->GetContent()->AsElement();
>  
> -  MOZ_ASSERT(elem->GetPseudoElementType() ==
> -             aFrame->StyleContext()->GetPseudoType());
> -  if (elem->HasServoData()) {
> -    return Servo_HasAuthorSpecifiedRules(elem,
> +  if (elem->GetPseudoElementType() != CSSPseudoElementType::NotPseudo) {
> +    elem = elem->GetParent()->AsElement();
> +    MOZ_ASSERT(elem, "Pseudo element has no parent element?");
> +  }

Also, you should probably assert that the style context is not an anon-box pseudo style, otherwise this would be kind of a mess (or, we should just return false in that case).

::: servo/ports/geckolib/glue.rs:1873
(Diff revision 1)
> -    let primary_style = data.styles.primary();
> -
>      let guard = (*GLOBAL_STYLE_DATA).shared_lock.read();
>      let guards = StylesheetGuards::same(&guard);
>  
> -    primary_style.rules().has_author_specified_rules(element,
> +    style.rules().has_author_specified_rules(element, pseudo,

nit: Move `pseudo` to its own line (feel free to reformat the signatures while touching this while at it if you want to follow the RFC style).

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8916898 [details]
Bug 1401825 - Support pseudo-element properly in nsPresContext::HasAuthorSpecifiedRules.

https://reviewboard.mozilla.org/r/187940/#review193038

> At this point you have already dereferenced it in debug builds, so probably this assertion is not that worth (you'd have crashed if it would fail).
> 
> Or, maybe move it before the assignment to look like:
> 
> ```
> MOZ_ASSERT(elem->GetParent() && elem->GetParent()->IsElement());
> ```

Oh, yeah, good catch. I added `->AsElement()` after writing the assertion as a fixup...

Checking of `IsElement()` is not necessary because `AsElement()` would assert that.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8916898 [details]
Bug 1401825 - Support pseudo-element properly in nsPresContext::HasAuthorSpecifiedRules.

https://reviewboard.mozilla.org/r/187940/#review193038

> Also, you should probably assert that the style context is not an anon-box pseudo style, otherwise this would be kind of a mess (or, we should just return false in that case).

I guess it is currently not possible to have anonymous box reach here... but let's return false for now to be safe.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8916898 [details]
Bug 1401825 - Support pseudo-element properly in nsPresContext::HasAuthorSpecifiedRules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187940/diff/1-2/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Servo PR

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ad5d399ad84
Support pseudo-element properly in nsPresContext::HasAuthorSpecifiedRules. r=emilio

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8ad5d399ad84

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

I don't think this is something important to uplift. I guess we can have it ride the 58 train.