Bug 242345

Summary:

nullptr crash in EventPath::buildPath via FullscreenManager::dispatchFullscreenChangeOrErrorEvent

Product:

WebKit

Reporter:

Michael Saboff <msaboff>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

beidson, bfulgham, brandonstewart, cgarcia, csaavedra, fred.wang, gpoo, mmaxfield, msaboff, rbuis, rniwa, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
repro_972.html	none

Description Michael Saboff 2022-07-05 10:03:57 PDT

Created attachment 460685 [details]
repro_972.html

Summary:

this crashes:

<script>
  onload = async () => {
    let shadowRoot = document.createElement('div').attachShadow({mode: 'open'});
    window.r = new WeakRef(shadowRoot.children);
    let div2 = document.createElement('div');
    div2.webkitRequestFullscreen();
    shadowRoot.replaceChildren(div2);
    await undefined;
    GCController.collect();
  };
</script>


Steps To Reproduce:

Does not reproduce in DumpRenderTree.

DYLD_FRAMEWORK_PATH=<WEBKIT_PATH>/WebKitBuild/Release <WEBKIT_PATH>/WebKitBuild/Release/WebKitTestRunner --no-enable-all-experimental-features repro_972.html

Results:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x000000000000001c
Exception Codes:       0x0000000000000001, 0x000000000000001c

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   WebCore       0x10d9ec6a0 WTF::OptionSet<WebCore::Node::NodeFlag>::containsAny(WTF::OptionSet<WebCore::Node::NodeFlag>) const + 0 (OptionSet.h:179) [inlined]
1   WebCore       0x10d9ec6a0 WTF::OptionSet<WebCore::Node::NodeFlag>::contains(WebCore::Node::NodeFlag) const + 0 (OptionSet.h:174) [inlined]
2   WebCore       0x10d9ec6a0 WebCore::Node::hasNodeFlag(WebCore::Node::NodeFlag) const + 0 (Node.h:611) [inlined]
3   WebCore       0x10d9ec6a0 WebCore::Node::isElementNode() const + 0 (Node.h:198) [inlined]
4   WebCore       0x10d9ec6a0 WebCore::Node::pseudoId() const + 0 (Node.h:213) [inlined]
5   WebCore       0x10d9ec6a0 WebCore::Node::isPseudoElement() const + 0 (Node.h:210) [inlined]
6   WebCore       0x10d9ec6a0 WTF::TypeCastTraits<WebCore::PseudoElement const, WebCore::Node const, false>::isType(WebCore::Node const&) + 0 (PseudoElement.h:62) [inlined]
7   WebCore       0x10d9ec6a0 WTF::TypeCastTraits<WebCore::PseudoElement const, WebCore::Node const, false>::isOfType(WebCore::Node const&) + 0 (PseudoElement.h:61) [inlined]
8   WebCore       0x10d9ec6a0 bool WTF::is<WebCore::PseudoElement, WebCore::Node>(WebCore::Node&) + 0 (TypeCasts.h:58) [inlined]
9   WebCore       0x10d9ec6a0 WebCore::EventPath::eventTargetRespectingTargetRules(WebCore::Node&) + 0 (EventPath.h:63) [inlined]
10  WebCore       0x10d9ec6a0 WebCore::EventPath::buildPath(WebCore::Node&, WebCore::Event&) + 1756 (EventPath.cpp:152)
11  WebCore       0x10d9ec62c WTF::TypeCastTraits<WebCore::Node const, WebCore::EventTarget const, false>::isType(WebCore::EventTarget const&) + 32 (Node.h:950) [inlined]
12  WebCore       0x10d9ec62c WTF::TypeCastTraits<WebCore::Node const, WebCore::EventTarget const, false>::isOfType(WebCore::EventTarget const&) + 32 (Node.h:949) [inlined]
13  WebCore       0x10d9ec62c bool WTF::is<WebCore::Node, WebCore::EventTarget>(WebCore::EventTarget&) + 32 (TypeCasts.h:58) [inlined]
14  WebCore       0x10d9ec62c std::__1::conditional<std::is_const_v<WebCore::EventTarget>, std::__1::add_const<WebCore::Node>::type, std::__1::remove_const<WebCore::Node>::type>::type& WTF::downcast<WebCore::Node, WebCore::EventTarget>(WebCore::EventTarget&) + 32 (TypeCasts.h:79) [inlined]
15  WebCore       0x10d9ec62c WebCore::shouldEventCrossShadowBoundary(WebCore::Event&, WebCore::ShadowRoot&, WebCore::EventTarget&) + 252 (EventPath.cpp:54) [inlined]
16  WebCore       0x10d9ec62c WebCore::EventPath::buildPath(WebCore::Node&, WebCore::Event&) + 1640 (EventPath.cpp:146)
17  WebCore       0x10d9ebeec WebCore::EventPath::EventPath(WebCore::Node&, WebCore::Event&) + 56 (EventPath.cpp:85)
18  WebCore       0x10d9e3dc8 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) + 124 (EventDispatcher.cpp:155)
19  WebCore       0x10d9f8380 WebCore::FullscreenManager::dispatchFullscreenChangeOrErrorEvent(WTF::Deque<WTF::RefPtr<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>, WTF::DefaultRefDerefTraits<WebCore::Node> >, 0ul>&, WTF::AtomString const&, bool) + 676 (FullscreenManager.cpp:606)

Regression:

I am seeing this on WebKit canonical revision 252021@main
Both asan and non-asan builds crash

Comment 1 Michael Saboff 2022-07-05 10:05:46 PDT

<rdar://96292712>

Comment 2 Ryosuke Niwa 2022-08-06 01:50:03 PDT

The issue is that FullscreenManager doesn't keep JS wrapper of change/error event nodes alive. As a result, GC will happily collect shadow root as well as its host and ancestors.

Comment 3 Ryosuke Niwa 2022-08-06 02:14:08 PDT

Not a security issue.

Comment 4 Ryosuke Niwa 2022-08-06 02:24:53 PDT

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

Comment 5 EWS 2022-08-08 12:30:31 PDT

Committed 253227@main (29c4919ba55f): <https://commits.webkit.org/253227@main>

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