Closed Bug 1469076 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 + fixed
firefox62 + fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files)

Attached file 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>
Flags: in-testsuite?
Component: Layout: Web Painting → Layout
Component: Layout → CSS Parsing and Computation
Animation... bug 1468534 might be related.
That is a surprisingly minimal test-case. Thanks for finding this Tyson!
Flags: needinfo?(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 :)
Blocks: 1468534
It's also a regression, since I can't repro it on stable.
Oh though I guess it's because WebAnimations are not enabled there... Let me check
(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!
Assignee: nobody → emilio
Blocks: 1453702
Flags: needinfo?(emilio)
Blocks: 1469123
I added a WPT which highlights the underlying correctness issue.
Hard to make a tracking decision here without any indication of what the impact to the release is.
Crashing, basically, under some particular conditions (not really very likely, but patch is just caching less so it's trivial to prevent).
Flags: needinfo?(emilio)
Bug 1468534 is showing up in the Beta topcrash list, so yeah, it would be good to get this into 61 still.
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.
Flags: needinfo?(cam)
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:emilio@crisal.io">
> +<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?
Attachment #8985768 - Flags: review+
Flags: needinfo?(cam)
Attachment #8985768 - Flags: review?(xidorn+moz)
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 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.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37d2fca3693a
Fix the broken invariants of the rule node cache. r=heycam
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
Attachment #8985768 - Flags: approval-mozilla-release?
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11592 for changes under testing/web-platform/tests
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.
Attachment #8985768 - Flags: approval-mozilla-release? → approval-mozilla-release+
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/37d2fca3693a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.