Closed Bug 1401825 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- disabled
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: jkratzer, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file trigger.html
Testcase found while fuzzing mozilla-central rev f8dd3f21e434.
Flags: in-testsuite?
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!
Flags: needinfo?(lochang)
Priority: -- → P3
Ok, I will look into it.
Assignee: nobody → lochang
Flags: needinfo?(lochang)
Hi Jason,
I could not reproduce the assertion on my Mac, is it only happen on Linux? Could you be more specific, thanks.
Flags: needinfo?(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.
Flags: needinfo?(jkratzer)
Attached file Minidump stack trace
Attached minidump stack trace.
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 :(
Has Regression Range: --- → no
Priority: P3 → P2
Summary: Assertion failure: elem->GetPseudoElementType() == aFrame->StyleContext()->GetPseudoType(), at /builds/worker/workspace/build/src/layout/base/nsPresContext.cpp:2242 → stylo: Assertion failure: elem->GetPseudoElementType() == aFrame->StyleContext()->GetPseudoType(), at /builds/worker/workspace/build/src/layout/base/nsPresContext.cpp:2242
Flags: needinfo?(xidorn+moz)
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.
Assignee: lochang → xidorn+moz
Flags: needinfo?(xidorn+moz)
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).
Attachment #8916898 - Flags: review?(emilio) → review+
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 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.
Attached file Servo PR
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ad5d399ad84
Support pseudo-element properly in nsPresContext::HasAuthorSpecifiedRules. r=emilio
https://hg.mozilla.org/mozilla-central/rev/8ad5d399ad84
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I don't think this is something important to uplift. I guess we can have it ride the 58 train.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.