1588356 - Crash in [@ nsINode::WrapObject]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Julian Seward [:jseward]]

This bug is for crash report bp-c0a23330-e324-471e-8d1f-1bc5a0191012.

This crash happened 9 times in 4 different instances in the Windows nightly 20191010214019.

Top 10 frames of crashing thread:

0 xul.dll nsINode::WrapObject dom/base/nsINode.cpp:2669
1 xul.dll static bool mozilla::dom::Element_Binding::get_openOrClosedShadowRoot dom/bindings/ElementBinding.cpp:4257
2 xul.dll mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3132
3 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:549
4 xul.dll JS::Call js/src/jsapi.cpp:2722
5 xul.dll xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::get js/xpconnect/wrappers/XrayWrapper.cpp:2089
6 xul.dll js::Proxy::get js/src/proxy/Proxy.cpp:353
7 xul.dll js::Proxy::get js/src/proxy/Proxy.cpp:352
8 xul.dll js::Proxy::get js/src/proxy/Proxy.cpp:352
9 xul.dll js::Proxy::get js/src/proxy/Proxy.cpp:352

Comment 1

[Anne (:annevk)]

Shadow trees is Edgar territory.

Comment 2

[Marcia Knous [:marcia]]

Looks as if the crashes started in 71 in Build ID 20191009213914. There are some macOS crashes as well. Possible regression range based on build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be9a6289486a6f366e431782b84a0c0633f8fec2&tochange=e9783a644016aa9b317887076618425586730d73

Bug 1585823 has some ShadowRoot in the checkin description. adding a ni on emilio as well, in case it is related to that checkin

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So they are all from this assertion:

• https://searchfox.org/mozilla-central/rev/17756e2a5c180d980a4b08d99f8cc0c97290ae8d/dom/base/nsINode.cpp#2669

The spike is related to https://hg.mozilla.org/mozilla-central/rev/68e29d2cd18276860baa297af5b0621013595800, since that makes it go through WrapNode rather than WrapObject and thus run that assertion.

But that assertion should hold anyway, so it's only uncovering a pre-existing bug.

Jason, have you or any other of your fuzzers seen something like this?

Comment 4

[Jason Kratzer [:jkratzer]]

Emilio, no unfortunately not. I'm going to spin up some targeted shadow root fuzz runs and see if I can't shake it out.

Comment 5

[Edgar Chen [:edgar]]

Look over the crash reports, almost all of the crashes happen on extension, and https://crash-stats.mozilla.org/report/index/a76a44cc-4d33-4962-9b70-527900191021 indicates that the crash happens on Live Start Page extension. And I could also reproduce the crash with Live Start Page extension installed as well, here is the crash report from my side.

STR:

• install Live Start Page extension
• Press 'Open a new tab' icon to open a new tab.
• Crash randomly happens on new tab.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Oh, lovely, thanks for figuring out STR! Do you plan to look further into it? Otherwise I can take a look on rr to see what's going on.

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

I am going to track it for 71 as this is going up since mid-october and the volume is already noticeable on dev edition builds, so this is likely to be be significant in beta/release.

Comment 8

[Edgar Chen [:edgar]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Oh, lovely, thanks for figuring out STR! Do you plan to look further into it? Otherwise I can take a look on rr to see what's going on.

Yes, I plan to take a look into it.

Comment 9

[Edgar Chen [:edgar]]

In debug build, we hit the assertion https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/dom/bindings/BindingUtils.h#1616 first before https://searchfox.org/mozilla-central/rev/17756e2a5c180d980a4b08d99f8cc0c97290ae8d/dom/base/nsINode.cpp#2669.

Comment 10

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #9)

In debug build, we hit the assertion https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/dom/bindings/BindingUtils.h#1616 first before https://searchfox.org/mozilla-central/rev/17756e2a5c180d980a4b08d99f8cc0c97290ae8d/dom/base/nsINode.cpp#2669.

Update what I have found for now,

• We hit the assertion in https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/dom/bindings/BindingUtils.h#1616 is because the UAWidgetScope got from https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/js/xpconnect/src/XPCJSRuntime.cpp#3276 is a DeadWrapper.
• And it used to be a CrossCompartmentWrapper, but be nuked in https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/dom/base/WindowDestroyedEvent.cpp#120.

Comment 11

[Edgar Chen [:edgar]]

What exactly happened is,

• The extension page contains elements have UAWidget setup, for example, <audio> and <video> element.
• We setup UAWidget for the element in https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/toolkit/actors/UAWidgetsChild.jsm#103 in JS UAWidgetScope.
• And UAWidgetScope is created and cached in https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/js/xpconnect/src/XPCJSRuntime.cpp#3302 after creation, but the JSObject we cached in hashtable is a content-to-UAWidget wrapper.
• After closing the extension window, xpc::NukeAllWrappersForRealm is triggered in https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/dom/base/WindowDestroyedEvent.cpp#120. It may also nuke all outgoing wrappers as well, and then the cached wrapper is nuked.
• Open the same extension page again, we get UAWidgetScope cached in the hashtable for UAWidget setup, then we get a DeadWrapper.
• In debug build, we could not unwrap the wrapper and hit the assertion in https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/dom/bindings/BindingUtils.h#1616
• In non-debug build, we end up exposing UAWiget to the wrong scope and hit https://searchfox.org/mozilla-central/rev/17756e2a5c180d980a4b08d99f8cc0c97290ae8d/dom/base/nsINode.cpp#2669.

Comment 12

[Edgar Chen [:edgar]]

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

Comment 13

[Edgar Chen [:edgar]]

Attachment: Bug 1588356 - Store the underlying sandbox object instead of a wrapper to UAWidgetScopeMap;

If the wrapper is for extension, it could possibly be nuked after extension's
window is destroyed. Then we will end up getting a DeadWrapper for same
extension next time (if the UAWidgetScopeMap is not yet swept).

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e130f2d4809
Store the underlying sandbox object instead of a wrapper to UAWidgetScopeMap; r=bholley

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/8e130f2d4809

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 9104168 [details]
Bug 1588356 - Store the underlying sandbox object instead of a wrapper to UAWidgetScopeMap;

Beta/Release Uplift Approval Request

• User impact if declined: Webextension page might crash if it uses ShadowDOM
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change here is straightforward and makes more sense. And it has been verified in Nightly.
• String changes made/needed: None

Comment 18

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9104168 [details]
Bug 1588356 - Store the underlying sandbox object instead of a wrapper to UAWidgetScopeMap;

Crash fix, noticeable in beta, uplift approved for 71 beta 6, thanks.

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5614af3ece59