Bug 288918 – REGRESSION: [Win] Crash in JSC::MarkedBlock::vm under WebCore::Element::attachShadow

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Fujii Hironori]

By loading some web site, Windows MiniBrowser is crashing soon.
For example, <https://www.reddit.com/>.

291246@main good
291260@main bad

Callstack:

WebCore.dll!JSC::MarkedBlock::vm() Line 513	C++
WebCore.dll!JSC::HeapCell::vm() Line 66	C++
WebCore.dll!JSC::JSCell::classInfo() Line 384	C++
WebCore.dll!JSC::JSCell::inherits(const JSC::ClassInfo * info) Line 353	C++
WebCore.dll!JSC::JSCastingHelpers::FinalTypeDispatcher<0>::inheritsGeneric<WebCore::JSCustomElementRegistry,JSC::JSCell>(JSC::JSCell * from) Line 194	C++
WebCore.dll!JSC::JSCastingHelpers::InheritsTraits<WebCore::JSCustomElementRegistry>::inherits<JSC::JSCell>(JSC::JSCell * from) Line 229	C++
WebCore.dll!JSC::jsDynamicCast<WebCore::JSCustomElementRegistry *,JSC::JSCell>(JSC::JSCell * from) Line 258	C++
WebCore.dll!JSC::jsDynamicCast<WebCore::JSCustomElementRegistry *>(JSC::JSValue from) Line 268	C++
WebCore.dll!WebCore::Element::attachShadow(const WebCore::ShadowRootInit & init, WebCore::Element::CustomElementRegistryKind registryKind) Line 3273	C++
WebCore.dll!WebCore::jsElementPrototypeFunction_attachShadowBody(JSC::JSGlobalObject * lexicalGlobalObject, JSC::CallFrame * callFrame, WebCore::JSElement * castedThis) Line 3936	C++
WebCore.dll!WebCore::IDLOperation<WebCore::JSElement>::call<&WebCore::jsElementPrototypeFunction_attachShadowBody,0>(JSC::JSGlobalObject & lexicalGlobalObject, JSC::CallFrame & callFrame, const char * operationName) Line 63	C++
WebCore.dll!WebCore::jsElementPrototypeFunction_attachShadow(JSC::JSGlobalObject * lexicalGlobalObject, JSC::CallFrame * callFrame) Line 3941	C++
0000025d80001158()	Unknown
000000b3dbffbea0()	Unknown
JavaScriptCore.dll!llint_entry()	C++

[Ryosuke Niwa]

I can't reproduce this issue locally on macOS but maybe the following change will fix it?

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
index ed3728a8f57d..1031035ddc4b 100644
--- a/Source/WebCore/dom/Element.cpp
+++ b/Source/WebCore/dom/Element.cpp
@@ -3270,8 +3270,10 @@ ExceptionOr<ShadowRoot&> Element::attachShadow(const ShadowRootInit& init, Custo
         return Exception { ExceptionCode::NotSupportedError };
     }
     RefPtr<CustomElementRegistry> registry;
-    if (auto* wrapper = jsDynamicCast<JSCustomElementRegistry*>(init.customElements))
-        registry = &wrapper->wrapped();
+    if (init.customElements.isObject()) {
+        if (auto* wrapper = jsDynamicCast<JSCustomElementRegistry*>(init.customElements))
+            registry = &wrapper->wrapped();
+    }
     auto scopedRegistry = ShadowRoot::ScopedCustomElementRegistry::No;
     if (registryKind == CustomElementRegistryKind::Null) {
         ASSERT(!registry);

[Fujii Hironori]

Then, a crash happens in JSValue::isObject().
init.customElements is empty.

[Fujii Hironori]

Surprisingly, this is not reproducible with Windows WebKitTestRunner.
fast/shadow-dom/activate-over-slotted-content.html layout test passes,
but Windows MiniBrowser crashes by loading LayoutTests/fast/shadow-dom/activate-over-slotted-content.html.

[Fujii Hironori]

customElements is enabled only if ScopedCustomElementRegistryEnabled.
https://github.com/WebKit/WebKit/blob/ecfa4421d0b3408a1a90842cc8d0a76bbc67bd56/Source/WebCore/dom/ShadowRootInit.idl#L34

Enabling ScopedCustomElementRegistryEnabled works around the problem.

diff --git a/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp b/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp
index 9011c13090a0..4fb7ec8d223a 100644
--- a/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp
+++ b/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp
@@ -148,6 +148,8 @@ Ref<BrowserWindow> WebKitBrowserWindow::create(BrowserWindowClient& client, HWND
     WKPreferencesSetMediaCapabilitiesEnabled(preferences.get(), false);
     WKPreferencesSetDeveloperExtrasEnabled(preferences.get(), true);
 
+    WKPreferencesSetBoolValueForKeyForTesting(preferences.get(), true, createWKString("ScopedCustomElementRegistryEnabled").get());
+
     auto pageConf = adoptWK(WKPageConfigurationCreate());
     WKPageConfigurationSetWebsiteDataStore(pageConf.get(), websiteDataStore.get());
     WKPageConfigurationSetContext(pageConf.get(), context.get());

[Fujii Hironori]

Which is better condition here?

> if (document().settings().scopedCustomElementRegistryEnabled())

or

>    if (!init.customElements.isEmpty()) {

[Ryosuke Niwa]

We probably want to check both:

document().settings().scopedCustomElementRegistryEnabled() && !init.customElements.isEmpty()

[Fujii Hironori]

Created attachment 474393 [details]
crash log of Mac MiniBrowser

This is reproducible with Mac MiniBrowser.

1. Start Mac MiniBrowser
2. Turn off ScopedCustomElementRegistryEnabled
   Menu →  Settings → Experimental Features → Uncheked "Scoped custom element registry"
3. Load https://www.reddit.com/

[Fujii Hironori]

Thank you. I'm going to create a PR.

[Fujii Hironori]

Pull request: https://github.com/WebKit/WebKit/pull/41746

[EWS]

Committed 291477@main (2733dd0b5570): <https://commits.webkit.org/291477@main>

Reviewed commits have been landed. Closing PR #41746 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/145979974>