1291891 - stylo: Fix various reftest crashes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

I've worked my way through many, but not all, of the crashes in bug 1289964. I have some miscellaneous fixes in both Gecko and Servo that fix most of the crashes I've encountered so far.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't segfault on null parent context in ServoStyleSet::ResolveStyleForOtherNonElement. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement ServoStyleSet::ReplaceSheets. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Switch to NS_WARNING for unimplemented <style scoped>. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Switch to NS_WARNING for various unimplemented incremental restyle methods. v1

No need to bring the browser down.

Comment 5

[Bobby Holley (:bholley)]

Part 1 requires the patch in https://github.com/servo/servo/pull/12725 to be fully effective, but does not require it per se.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8777551 [details] [diff] [review]
Part 1 - Don't segfault on null parent context in ServoStyleSet::ResolveStyleForOtherNonElement. v1

Review of attachment 8777551 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ +141,5 @@
>  ServoStyleSet::ResolveStyleForOtherNonElement(nsStyleContext* aParentContext)
>  {
> +  ServoComputedValues* parent =
> +    aParentContext ? aParentContext->StyleSource().AsServoComputedValues() : nullptr;
> +  RefPtr<ServoComputedValues> computedValues = Servo_InheritComputedValues(parent);

Just curious, where have you hit this? LGTM.

Also, dont_AddRef() it, though it might be done in the other bug.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8777552 [details] [diff] [review]
Part 2 - Implement ServoStyleSet::ReplaceSheets. v1

Review of attachment 8777552 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ +254,5 @@
>  {
> +  // Gecko uses a two-dimensional array keyed by sheet type, whereas Servo
> +  // stores a flattened list. This makes ReplaceSheets a pretty clunky thing
> +  // to express. If the need ever arises, we can easily make this more efficent,
> +  // probably by aligning the representations better between engines.

heh, I was just going to suggest a comment like this (I read the patch bottom up).

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8777554 [details] [diff] [review]
Part 4 - Switch to NS_WARNING for various unimplemented incremental restyle methods. v1

Review of attachment 8777554 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresShell.cpp
@@ +2901,5 @@
>    RestyleManagerHandle restyleManager = mPresContext->RestyleManager();
>    if (restyleManager->IsServo()) {
> +    NS_WARNING("stylo: PresShell::RecreateFramesFor not implemented for Servo-"
> +               "backed style system");
> +    return NS_OK;

This is gone with bug 1290335.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Comment on attachment 8777551 [details] [diff] [review]
> Part 1 - Don't segfault on null parent context in
> ServoStyleSet::ResolveStyleForOtherNonElement. v1
> 
> Review of attachment 8777551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ServoStyleSet.cpp
> @@ +141,5 @@
> >  ServoStyleSet::ResolveStyleForOtherNonElement(nsStyleContext* aParentContext)
> >  {
> > +  ServoComputedValues* parent =
> > +    aParentContext ? aParentContext->StyleSource().AsServoComputedValues() : nullptr;
> > +  RefPtr<ServoComputedValues> computedValues = Servo_InheritComputedValues(parent);
> 
> Just curious, where have you hit this? LGTM.
> 
> Also, dont_AddRef() it, though it might be done in the other bug.

Yeah, that fix is in bug 1291885. I'll land them together.

Comment 10

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a8f0dbcb786

Comment 11

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Comment on attachment 8777551 [details] [diff] [review]
> Part 1 - Don't segfault on null parent context in
> ServoStyleSet::ResolveStyleForOtherNonElement. v1
> 
> Review of attachment 8777551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ServoStyleSet.cpp
> @@ +141,5 @@
> >  ServoStyleSet::ResolveStyleForOtherNonElement(nsStyleContext* aParentContext)
> >  {
> > +  ServoComputedValues* parent =
> > +    aParentContext ? aParentContext->StyleSource().AsServoComputedValues() : nullptr;
> > +  RefPtr<ServoComputedValues> computedValues = Servo_InheritComputedValues(parent);
> 
> Just curious, where have you hit this? LGTM.

The issue was happened when the style context was shared with a <div> that was the root of an anonymous (probably XBL) subtree. I don't remember which precise kind of non-element we were dealing with unfortunately. I've added a comment.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed55ef70d491
Don't segfault on null parent context in ServoStyleSet::ResolveStyleForOtherNonElement. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/012ce5256391
Implement ServoStyleSet::ReplaceSheets. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/a22bd2009482
Switch to NS_WARNING for unimplemented <style scoped>. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a4af3c926b2
Switch to NS_WARNING for various unimplemented incremental restyle methods. r=emilio

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed55ef70d491
https://hg.mozilla.org/mozilla-central/rev/012ce5256391
https://hg.mozilla.org/mozilla-central/rev/a22bd2009482
https://hg.mozilla.org/mozilla-central/rev/6a4af3c926b2