1465572 - Crash in geckoservo::glue::Servo_Element_GetPrimaryComputedValues

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-d3160d40-b0ef-4ee0-bebd-dae9c0180529.
=============================================================

Primarily Mac and Fennec crash spotted on nightly: https://bit.ly/2kF1WP3. Mac crashes are 16 crashes/5 installs, although it appears the same person crashed several times on a localhost test. 

I can produce the crash on Mac nightly using https://www.vanmoof.com/en_us.

Top 10 frames of crashing thread:

0 XUL geckoservo::glue::Servo_Element_GetPrimaryComputedValues src/libcore/cell.rs:240
1 XUL nsCSSFrameConstructor::ResolveComputedStyle layout/base/nsCSSFrameConstructor.cpp:5012
2 XUL nsCSSFrameConstructor::AddFrameConstructionItems layout/base/nsCSSFrameConstructor.cpp:5573
3 XUL nsCSSFrameConstructor::ContentRangeInserted layout/base/nsCSSFrameConstructor.cpp:6753
4 XUL mozilla::RestyleManager::ProcessRestyledFrames layout/base/RestyleManager.cpp:1510
5 XUL mozilla::RestyleManager::DoProcessPendingRestyles layout/base/RestyleManager.cpp:2994
6 XUL mozilla::PresShell::DoFlushPendingNotifications layout/base/RestyleManager.cpp:3071
7 XUL nsIDocument::FlushPendingNotifications layout/base/nsIPresShell.h:576
8 XUL mozilla::dom::Element::GetScrollFrame dom/base/Element.cpp:2291
9 XUL mozilla::dom::Element::GetClientAreaRect dom/base/Element.cpp:1049

=============================================================

Comment 1

[Michael England]

FWIW, I submitted one of the crash reports (probably the localhost ones mentioned above).  We're getting ready to release our application which uses Web Components.  We were testing how our product works after ShadowDOM was enabled in the nightly: https://bugzilla.mozilla.org/show_bug.cgi?id=1460069.  It looks like Vanmoof's site also uses Web Components.  

In our application we have a `div` with `display:flex;` styling and a HTMLSlotElement child.  Changing that style from `flex` to `block` stops the crashing.  I've tried building out a simple reproduction of this setup but haven't been able to get it to crash.  Let me know if there is more information I can provide.  Thanks!

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Michael England from comment #1)
> FWIW, I submitted one of the crash reports (probably the localhost ones
> mentioned above).  We're getting ready to release our application which uses
> Web Components.  We were testing how our product works after ShadowDOM was
> enabled in the nightly:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1460069.  It looks like
> Vanmoof's site also uses Web Components.  
> 
> In our application we have a `div` with `display:flex;` styling and a
> HTMLSlotElement child.  Changing that style from `flex` to `block` stops the
> crashing.  I've tried building out a simple reproduction of this setup but
> haven't been able to get it to crash.  Let me know if there is more
> information I can provide.  Thanks!

That's really helpful! I managed to repro on Vanmoof, though no clear STR (switching tabs back and forth seems to help).

I'll take a look. Of course a more reduced test-case would be appreciated, but of course that's really helpful still!

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1465572: Changing the slot name should properly invalidate layout.

Just something I found while trying to construct a test-case for this.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1465572: Ensure <slot> disables whitespace optimizations.

The whitespace optimization code only knows about the light tree.

It's not a great idea to try to put flattened tree children of a slot through
there, since the children may not be assigned to the same slot, or to any slot
(in which case we crash).

We should probably rename XBLInvolved to ShadowDOMOrXBLInvolved too, I guess.
Note that the ShadowRoot case already sets the bit on Init().

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8982776 [details]
Bug 1465572: Changing the slot name should properly invalidate layout.

https://reviewboard.mozilla.org/r/248664/#review254886

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8982777 [details]
Bug 1465572: Ensure <slot> disables whitespace optimizations.

https://reviewboard.mozilla.org/r/248666/#review255606

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/297e6634c0a6
Changing the slot name should properly invalidate layout. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d81a759c7d95
Ensure <slot> disables whitespace optimizations. r=bz

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/297e6634c0a6
https://hg.mozilla.org/mozilla-central/rev/d81a759c7d95

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11377 for changes under testing/web-platform/tests

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is this something that should ride the trains or should we consider it for Beta backport?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

This is only a Shadow DOM issue which is not enabled in Beta.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Huh, we see a couple Beta reports, but maybe they're from people manually turning it on.

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged