1382019 - stylo: Startup Crash in nsStyleContext::AsServo

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-167610b3-bdb9-4fa1-a9bc-d94050170718.
=============================================================

Today's respun nightly is crashing on startup with this stack with stylo enabled: http://bit.ly/2uxdYRb. Running Windows 10.

Without stylo there is no crash.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Will be fixed by bug 1382017, Manish?

Comment 2

[Manish Goregaokar [:manishearth]]

Yes.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

thanks!

Comment 4

[Bobby Holley (:bholley)]

This is a regression that happened to be picked up by the respin. :-(

Manish has a patch. We probably want to land it directly to central and then respin again.

Comment 5

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1382019 - stylo: Don't crash on null parent contexts in nsCSSFrameConstructor::AddFrameConstructionItemsInternal ;

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

Comment 6

[Manish Goregaokar [:manishearth]]

Patch moved to this bug. try @ https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=115335600

Comment 7

[Manish Goregaokar [:manishearth]]

Stylo try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=da13071104f4ba881580ae242dfbc36c0ba14d60 , forgot to include it in the previous stuff

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8887685 [details]
Bug 1382019 - stylo: Don't crash on null parent contexts in nsCSSFrameConstructor::AddFrameConstructionItemsInternal ;

https://reviewboard.mozilla.org/r/158584/#review163870

::: layout/base/nsCSSFrameConstructor.cpp:5892
(Diff revision 1)
>            // first time. Thus call StyleNewChildren() again.
>            styleSet->StyleNewChildren(element);
>  
> +          auto parent = styleContext->GetParentAllowServo();
> +          ServoStyleContext* parentServo =  parent ? parent->AsServo() : nullptr;
> +

This works, but I'd rather just pass `nullptr` there. There's no way that can create an style context given it's using `LazyComputeBehavior::Assert`.

That way you also get rid of a `GetParentAllowServo`, which is nicer.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8887685 [details]
Bug 1382019 - stylo: Don't crash on null parent contexts in nsCSSFrameConstructor::AddFrameConstructionItemsInternal ;

https://reviewboard.mozilla.org/r/158584/#review163870

Also, it'd be nice to have a test for this, btw.

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8887685 [details]
Bug 1382019 - stylo: Don't crash on null parent contexts in nsCSSFrameConstructor::AddFrameConstructionItemsInternal ;

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

Comment 11

[Manish Goregaokar [:manishearth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=818f0461e6e0b9dc39b0281a1ba0764aeee0f69f

Seems to be fine, might want to wait a bit more for tests to fill out.


How can we uplift this?

(AIUI getting this landed is more urgent, so i'm not including a test yet)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/1b065ffd8a53
stylo: Don't crash on null parent contexts in nsCSSFrameConstructor::AddFrameConstructionItemsInternal ; r=emilio a=crashfix CLOSED TREE

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Can you resolve this once you confirm it actually fixes things?

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you have a way to confirm it/reproduce the crash?

Comment 16

[Ekanan Ketunuti]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Do you have a way to confirm it/reproduce the crash?

just enable stylo (layout.css.servo.enabled=true) and start firefox.

Comment 17

[Gary [:streetwolf52]]

I'm running with this patch on inbound and no longer get the crashes as specified.

Comment 18

[Bobby Holley (:bholley)]

FWIW, I think the confusion here is that this is a platform-specific startup crash that doesn't happen on linux, which is the only platform we're running on CI right now for stylo. We're fixing this in bug 1380053.

Comment 19

[Gary [:streetwolf52]]

I'm running On Windows 10 btw...