1469076 - thread '<unnamed>' panicked at 'Resolving style on unstyled element', /checkout/src/libcore/option.rs:891:5

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20180614094457
SourceStamp=91db0c695f0272f00bf92c81c471a85101056d96

thread '<unnamed>' panicked at 'Resolving style on unstyled element', /checkout/src/libcore/option.rs:891:5
stack backtrace:
   0:     0x7fcf86567bf3 - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::hb98fbe643b37b8bb
                               at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:     0x7fcf865629ec - std::panicking::default_hook::{{closure}}::h83c090f00cd2917d
                               at /checkout/src/libstd/sys_common/backtrace.rs:68
   2:     0x7fcf8656237d - std::panicking::default_hook::hf9722061a353cd29
                               at /checkout/src/libstd/panicking.rs:397
   3:     0x7fcf86561f02 - std::panicking::rust_panic_with_hook::h574be4fada9826dc
                               at /checkout/src/libstd/panicking.rs:577
   4:     0x7fcf865640f5 - std::panicking::begin_panic::h87949d2c83be3718
                               at /checkout/src/libstd/panicking.rs:538
   5:     0x7fcf86564089 - std::panicking::begin_panic_fmt::h351d9e48d73bd160
                               at /checkout/src/libstd/panicking.rs:522
   6:     0x7fcf8657d302 - core::panicking::panic_fmt::hd4212c5d5ed3b640
                               at /checkout/src/libstd/panicking.rs:498
   7:     0x7fcf8657d668 - core::option::expect_failed::hd528a7bd2cab115d
                               at /checkout/src/libcore/option.rs:891
   8:     0x7fcf86089095 - Servo_ResolveStyle
                               at /checkout/src/libcore/option.rs:302
                               at servo/ports/geckolib/glue.rs:4327
   9:     0x7fcf804b41a7 - _ZN21nsCSSFrameConstructor20ResolveComputedStyleEP10nsIContent
                               at src/obj-firefox/dist/include/mozilla/ServoStyleSetInlines.h:25
                               at src/layout/base/nsCSSFrameConstructor.cpp:4967
  10:     0x7fcf80496fc2 - _ZN21nsCSSFrameConstructor33AddFrameConstructionItemsInternalER23nsFrameConstructorStateP10nsIContentP16nsContainerFramebPN7mozilla13ComputedStyleEjP8nsTArrayIN26nsIAnonymousContentCreator11ContentInfoEERNS_25FrameConstructionItemListE
                               at src/layout/base/nsCSSFrameConstructor.cpp:5722
  11:     0x7fcf804b755a - _ZN21nsCSSFrameConstructor25AddFrameConstructionItemsER23nsFrameConstructorStateP10nsIContentbRKNS_14InsertionPointERNS_25FrameConstructionItemListE
                               at src/layout/base/nsCSSFrameConstructor.cpp:5531
                               at src/layout/base/nsCSSFrameConstructor.cpp:5550
  12:     0x7fcf804c0f65 - _ZN21nsCSSFrameConstructor20ContentRangeInsertedEP10nsIContentS1_P21nsILayoutHistoryStateNS_13InsertionKindE
                               at src/layout/base/nsCSSFrameConstructor.cpp:7700
  13:     0x7fcf804564c4 - _ZN7mozilla14RestyleManager21ProcessRestyledFramesER17nsStyleChangeList
                               at src/layout/base/RestyleManager.cpp:1512
  14:     0x7fcf804630a5 - _ZN7mozilla14RestyleManager24DoProcessPendingRestylesENS_19ServoTraversalFlagsE
                               at src/layout/base/RestyleManager.cpp:2997
  15:     0x7fcf8043b652 - _ZN7mozilla9PresShell11HandleEventEP8nsIFramePNS_14WidgetGUIEventEbP13nsEventStatus
                               at src/layout/base/RestyleManager.cpp:3107
                               at src/layout/base/PresShell.cpp:6632
                               at src/layout/base/PresShell.cpp:6896
  16:     0x7fcf7fd96fa1 - _ZN13nsViewManager13DispatchEventEPN7mozilla14WidgetGUIEventEP6nsViewP13nsEventStatus
                               at src/view/nsViewManager.cpp:812
  17:     0x7fcf7fd96776 - _ZN6nsView11HandleEventEPN7mozilla14WidgetGUIEventEb
                               at src/view/nsView.cpp:1141
  18:     0x7fcf7fdffe35 - _ZN7mozilla6widget12PuppetWidget13DispatchEventEPNS_14WidgetGUIEventER13nsEventStatus
                               at src/widget/PuppetWidget.cpp:410
  19:     0x7fcf7aa72a70 - _ZN7mozilla6layers18APZCCallbackHelper19DispatchWidgetEventERNS_14WidgetGUIEventE
                               at src/gfx/layers/apz/util/APZCCallbackHelper.cpp:500
  20:     0x7fcf7f6c8232 - _ZN7mozilla3dom8TabChild26HandleRealMouseButtonEventERKNS_16WidgetMouseEventERKNS_6layers19ScrollableLayerGuidERKm
                               at src/dom/ipc/TabChild.cpp:1799
                               at src/dom/ipc/TabChild.cpp:1739
  21:     0x7fcf7f6c922e - _ZN7mozilla3dom8TabChild24RecvRealMouseButtonEventERKNS_16WidgetMouseEventERKNS_6layers19ScrollableLayerGuidERKm
                               at src/dom/ipc/TabChild.cpp:1706
  22:     0x7fcf7f6c9404 - _ZThn96_N7mozilla3dom8TabChild23RecvSynthMouseMoveEventERKNS_16WidgetMouseEventERKNS_6layers19ScrollableLayerGuidERKm
                               at src/dom/ipc/TabChild.cpp:1667
  23:     0x7fcf79a5bb5b - _ZN7mozilla3dom13PBrowserChild17OnMessageReceivedERKN3IPC7MessageE
                               at src/obj-firefox/ipc/ipdl/PBrowserChild.cpp:3558
  24:     0x7fcf794a6c11 - _ZN7mozilla3dom13PContentChild17OnMessageReceivedERKN3IPC7MessageE
                               at src/obj-firefox/ipc/ipdl/PContentChild.cpp:5316
  25:     0x7fcf79345eee - _ZN7mozilla3ipc14MessageChannel20DispatchAsyncMessageERKN3IPC7MessageE
                               at src/ipc/glue/MessageChannel.cpp:2134
  26:     0x7fcf79342e04 - _ZN7mozilla3ipc14MessageChannel15DispatchMessageEON3IPC7MessageE
                               at src/ipc/glue/MessageChannel.cpp:2064
  27:     0x7fcf7934465c - _ZN7mozilla3ipc14MessageChannel10RunMessageERNS1_11MessageTaskE
                               at src/ipc/glue/MessageChannel.cpp:1910
  28:     0x7fcf79344cb8 - _ZN7mozilla3ipc14MessageChannel11MessageTask3RunEv
                               at src/ipc/glue/MessageChannel.cpp:1943
  29:     0x7fcf78439435 - _ZN7mozilla14SchedulerGroup8Runnable3RunEv
                               at src/xpcom/threads/SchedulerGroup.cpp:337
  30:     0x7fcf7845863a - _ZN8nsThread16ProcessNextEventEbPb
                               at src/xpcom/threads/nsThread.cpp:1088
  31:     0x7fcf78474894 - _Z19NS_ProcessNextEventP9nsIThreadb
                               at src/xpcom/threads/nsThreadUtils.cpp:519
  32:     0x7fcf7934dbaa - _ZN7mozilla3ipc11MessagePump3RunEPN4base11MessagePump8DelegateE
                               at src/ipc/glue/MessagePump.cpp:97
  33:     0x7fcf792a2b0c - _ZN11MessageLoop3RunEv
                               at src/ipc/chromium/src/base/message_loop.cc:325
                               at src/ipc/chromium/src/base/message_loop.cc:318
                               at src/ipc/chromium/src/base/message_loop.cc:298
  34:     0x7fcf7fe2aa2a - _ZN14nsBaseAppShell3RunEv
                               at src/widget/nsBaseAppShell.cpp:157
  35:     0x7fcf8410469f - _Z15XRE_RunAppShellv
                               at src/toolkit/xre/nsEmbedFunctions.cpp:896
  36:     0x7fcf792a2b0c - _ZN11MessageLoop3RunEv
                               at src/ipc/chromium/src/base/message_loop.cc:325
                               at src/ipc/chromium/src/base/message_loop.cc:318
                               at src/ipc/chromium/src/base/message_loop.cc:298
  37:     0x7fcf84104056 - _Z20XRE_InitChildProcessiPPcPK12XREChildData
                               at src/toolkit/xre/nsEmbedFunctions.cpp:722
  38:           0x4f1ca4 - main
                               at src/browser/app/../../ipc/contentproc/plugin-container.cpp:50
                               at src/browser/app/nsBrowserApp.cpp:287
  39:     0x7fcf97e3e82f - __libc_start_main
  40:           0x4210e8 - <unknown>

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Animation... bug 1468534 might be related.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

That is a surprisingly minimal test-case. Thanks for finding this Tyson!

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> Animation... bug 1468534 might be related.

Yup, that's why I asked Tyson to reduce it :)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

It's also a regression, since I can't repro it on stable.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Oh though I guess it's because WebAnimations are not enabled there... Let me check

Comment 6

[Alice0775 White]

regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=55c90c91e3f3844e6db5b99c86c1ec6a0c24758d&tochange=43862b2ee72eec858f509ce3b9cb5c6fa96194ba

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Alice0775 White from comment #6)
> regression window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=55c90c91e3f3844e6db5b99c86c1ec6a0c24758d&tochange=4386
> 2b2ee72eec858f509ce3b9cb5c6fa96194ba

Thanks a lot Alice!

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1469076: Fix the broken invariants of the rule node cache.

We were spuriously reframing the <shadow> because it initially shared style with
the <br>, which ended up being display: none, while the <shadow> should've been
display: contents from the beginning.

lookup_by_rules seems pretty prone to obscure bugs, and also it's pretty
complex... Probably we should try to get rid of it, I'm unconvinced that it's
worth it.

Even with that, in a normal restyle the <details> wouldn't have ended up with a
style. It of course never had it before the reframe because the <shadow> was
display: none, but that doesn't mean it shouldn't have gotten one, since we
detected we needed to go through kids in:

  https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/servo/components/style/matching.rs#500

That code did happen, but since it's an animation-only restyle, we don't look at
unstyled stuff.

That looks somewhat fishy, but I guess for now it's fine as long as display
isn't animatable.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I added a WPT which highlights the underlying correctness issue.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8985768 [details]
Bug 1469076: Fix the broken invariants of the rule node cache.

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Hard to make a tracking decision here without any indication of what the impact to the release is.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Crashing, basically, under some particular conditions (not really very likely, but patch is just caching less so it's trivial to prevent).

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Bug 1468534 is showing up in the Beta topcrash list, so yeah, it would be good to get this into 61 still.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Cam, Ryan asked me to try to get this reviewed ASAP in case we uplift this for bug 1469123 (since it may be the same cause). 

I'm not ultra-hopeful about it being the same cause since I don't think our chrome uses display: contents, but it's trivial I think so we may as well give it a shot.

The tl;dr is that bug 1453702 added a tagname-dependent style adjustment, which broke lookup_by_rules (that is, two nodes with same parent and same node, yet different tagname, can end up with different style). This patch fixes it to avoid using the optimization in case there's a mismatch in the tag name.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8985768 [details]
Bug 1469076: Fix the broken invariants of the rule node cache.

https://reviewboard.mozilla.org/r/251306/#review258076

::: commit-message-a1868:7
(Diff revision 2)
> +lookup_by_rules seems pretty prone to obscure bugs, and also it's pretty
> +complex... Probably we should try to get rid of it, I'm unconvinced that it's
> +worth it.

lookup_by_rules itself isn't complex.  (Although I agree that the coupling between the style adjustment and lookup_by_rules is.)

::: layout/style/crashtests/1469076.html:6
(Diff revision 2)
> +<style>
> +* { display: contents }
> +</style>
> +<script>
> +function go() {
> +  a.parentElement.animate({"ping": [0, 1]}, 0.281207776578);

I wonder if it's expected that we do animation restyles when the animation doesn't have any valid properties in it like this.

::: servo/components/style/sharing/mod.rs:848
(Diff revision 2)
> -
> +            if target.namespace() != candidate.element.namespace() {
> +                return None;
> +            }
> +            if target.local_name() != candidate.element.local_name() {
> +                return None;
> +            }

Please add a comment here to say that the only reason we need to check on the element name is that we do name-dependent style adjustments.

::: testing/web-platform/tests/css/css-display/display-contents-sharing-001.html:7
(Diff revision 2)
> +<meta charset="utf-8">
> +<title style="display: none">CSS Test: display:contents style sharing.</title>
> +<link rel="author" title="Emilio Cobos Álvarez" href="mailto:[e-mail]">
> +<link rel="match" href="display-contents-sharing-001-ref.html">
> +<link rel="help" href="https://drafts.csswg.org/css-display/#unbox-html">
> +<link rel="help" href="https://bugzil.la/1469076">

https://web-platform-tests.org/writing-tests/css-metadata.html says that `<link rel="help">` should be used for specification links, so I think a Bugzilla link isn't appropriate here.  Move it to a comment if it's useful to keep here?

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Thanks for taking the review of this. I felt that I need to learn more about the sharing stuff and thus delayed the review.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8985768 [details]
Bug 1469076: Fix the broken invariants of the rule node cache.

https://reviewboard.mozilla.org/r/251306/#review258076

> https://web-platform-tests.org/writing-tests/css-metadata.html says that `<link rel="help">` should be used for specification links, so I think a Bugzilla link isn't appropriate here.  Move it to a comment if it's useful to keep here?

I read it as "the spec link should be in `<link rel="help">`, but I don't think there should be an issue adding more, indeed I've used this pattern in quite a few places already.

I can ask for clarification and change all of them to comments if needed.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37d2fca3693a
Fix the broken invariants of the rule node cache. r=heycam

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8985768 [details]
Bug 1469076: Fix the broken invariants of the rule node cache.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1468534
[User impact if declined]: potential crashes or correctness issues
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet, but crashtest is checked-in.
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: we just cache a little less.
[String changes made/needed]: none

Comment 20

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

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8985768 [details]
Bug 1469076: Fix the broken invariants of the rule node cache.

CI is looking good on inbound. Thanks for adding all the new test coverage. This is expected to fix a Stylo topcrash in bug 1468534, so approved for Fx61 RC2.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/f5ea361ceb75

Comment 23

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 24

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/37d2fca3693a

Comment 25

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

Upstream PR merged