Bug 220396 – Nullptr crash in ReadableStream::create

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Ryosuke Niwa]

Created attachment 417150 [details]
Test

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000122dd8e7c JSC::JSGlobalObject::vm() const + 12 (JSGlobalObject.h:1043)
1   com.apple.WebCore             	0x00000001256158d7 WebCore::ReadableStream::create(JSC::JSGlobalObject&, WTF::RefPtr<WebCore::ReadableStreamSource, WTF::RawPtrTraits<WebCore::ReadableStreamSource>, WTF::DefaultRefDerefTraits<WebCore::ReadableStreamSource> >&&) + 55 (ReadableStream.cpp:42)
2   com.apple.WebCore             	0x0000000124ec9f18 WebCore::RTCRtpSFrameTransform::createStreams() + 168 (RTCRtpSFrameTransform.cpp:167)
3   com.apple.WebCore             	0x0000000124eca5e1 WebCore::RTCRtpSFrameTransform::readable() + 193 (RTCRtpSFrameTransform.cpp:207)
4   com.apple.WebCore             	0x0000000123fe5179 WebCore::jsRTCRtpSFrameTransform_readableGetter(JSC::JSGlobalObject&, WebCore::JSRTCRtpSFrameTransform&) + 201 (JSRTCRtpSFrameTransform.cpp:339)
5   com.apple.WebCore             	0x0000000123f261ab long long WebCore::IDLAttribute<WebCore::JSRTCRtpSFrameTransform>::get<&(WebCore::jsRTCRtpSFrameTransform_readableGetter(JSC::JSGlobalObject&, WebCore::JSRTCRtpSFrameTransform&)), (WebCore::CastedThisErrorBehavior)3>(JSC::JSGlobalObject&, long long, char const*) + 283 (JSDOMAttribute.h:67)
6   com.apple.WebCore             	0x0000000123f26088 WebCore::jsRTCRtpSFrameTransform_readable(JSC::JSGlobalObject*, long long, JSC::PropertyName) + 40 (JSRTCRtpSFrameTransform.cpp:344)
7   com.apple.JavaScriptCore      	0x00000001447137af JSC::PropertySlot::customGetter(JSC::JSGlobalObject*, JSC::PropertyName) const + 479 (PropertySlot.cpp:46)
8   com.apple.JavaScriptCore      	0x00000001443b7331 JSC::PropertySlot::getValue(JSC::JSGlobalObject*, JSC::PropertyName) const + 177 (PropertySlot.h:421)
9   com.apple.JavaScriptCore      	0x0000000144650025 JSC::JSValue::get(JSC::JSGlobalObject*, JSC::PropertyName, JSC::PropertySlot&) const + 389 (JSCJSValueInlines.h:952)
10  com.apple.JavaScriptCore      	0x00000001441ceeed JSC::LLInt::performLLIntGetByID(JSC::Instruction const*, JSC::CodeBlock*, JSC::JSGlobalObject*, JSC::JSValue, JSC::Identifier const&, JSC::GetByIdModeMetadata&) + 269 (LLIntSlowPaths.cpp:760)
11  com.apple.JavaScriptCore      	0x00000001441cec8a llint_slow_path_get_by_id + 394 (LLIntSlowPaths.cpp:834)
12  com.apple.JavaScriptCore      	0x000000014323c84c llint_entry + 43706 (LowLevelInterpreter64.asm:97)
13  com.apple.JavaScriptCore      	0x000000014325316a llint_entry + 136152 (LowLevelInterpreter.asm:1092)
14  com.apple.JavaScriptCore      	0x0000000143253212 llint_entry + 136320 (LowLevelInterpreter.asm:1092)
15  com.apple.JavaScriptCore      	0x0000000143253212 llint_entry + 136320 (LowLevelInterpreter.asm:1092)
16  com.apple.JavaScriptCore      	0x0000000143253212 llint_entry + 136320 (LowLevelInterpreter.asm:1092)
17  com.apple.JavaScriptCore      	0x0000000143231aa0 vmEntryToJavaScript + 289 (LowLevelInterpreter64.asm:316)
18  com.apple.JavaScriptCore      	0x000000014409f4cb JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 235 (JITCodeInlines.h:42)
19  com.apple.JavaScriptCore      	0x000000014409fc87 JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1815 (Interpreter.cpp:905)
20  com.apple.JavaScriptCore      	0x00000001443f299d JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 221 (CallData.cpp:57)
21  com.apple.JavaScriptCore      	0x00000001443f2c73 JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 115 (CallData.cpp:78)
22  com.apple.JavaScriptCore      	0x0000000144605eb1 JSC::JSMicrotask::run(JSC::JSGlobalObject*) + 657 (JSMicrotask.cpp:92)
23  com.apple.WebCore             	0x00000001255cecee WebCore::JSExecState::runTask(JSC::JSGlobalObject*, JSC::Microtask&) + 46 (JSExecState.h:91)
24  com.apple.WebCore             	0x00000001255d5a9b WebCore::JSMicrotaskCallback::call() + 235 (JSMicrotaskCallback.h:46)
25  com.apple.WebCore             	0x00000001255d58d4 WebCore::JSDOMWindowBase::queueMicrotaskToEventLoop(JSC::JSGlobalObject&, WTF::Ref<JSC::Microtask, WTF::RawPtrTraits<JSC::Microtask> >&&)::$_36::operator()() + 68 (JSDOMWindowBase.cpp:228)
26  com.apple.WebCore             	0x00000001255d57de WTF::Detail::CallableWrapper<WebCore::JSDOMWindowBase::queueMicrotaskToEventLoop(JSC::JSGlobalObject&, WTF::Ref<JSC::Microtask, WTF::RawPtrTraits<JSC::Microtask> >&&)::$_36, void>::call() + 30 (Function.h:52)
27  com.apple.WebCore             	0x0000000122d1f5f2 WTF::Function<void ()>::operator()() const + 130 (Function.h:83)
28  com.apple.WebCore             	0x0000000125c72e6e WebCore::EventLoopFunctionDispatchTask::execute() + 30 (EventLoop.cpp:159)
29  com.apple.WebCore             	0x0000000125cb4e3c WebCore::MicrotaskQueue::performMicrotaskCheckpoint() + 332 (Microtasks.cpp:64)
30  com.apple.WebCore             	0x0000000125c69441 WebCore::EventLoop::run() + 401 (EventLoop.cpp:125)
31  com.apple.WebCore             	0x0000000125dcfa9c WebCore::WindowEventLoop::didReachTimeToRun() + 44 (WindowEventLoop.cpp:120)

<rdar://problem/71933447>

[Rob Buis]

Created attachment 417867 [details]
Patch

[Rob Buis]

Created attachment 417868 [details]
Patch

[youenn fablet]

Comment on attachment 417868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417868&action=review

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.cpp:166
> +        return;

I think it would be best for createStreams caller to check for globalObject.
I would suggest we pass a GlobalObject& and at call site we throw an InvalidStateError error.
DOMCache::put may suffer from the same issue as well.

> LayoutTests/http/wpt/webrtc/sframe-transform-readable-crash.html:7
> +new SFrameTransform().readable;

It is unclear why we need the requestAdapter call.

[Rob Buis]

Created attachment 417872 [details]
Patch

[Rob Buis]

Comment on attachment 417868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417868&action=review

>> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.cpp:166
>> +        return;
> 
> I think it would be best for createStreams caller to check for globalObject.
> I would suggest we pass a GlobalObject& and at call site we throw an InvalidStateError error.
> DOMCache::put may suffer from the same issue as well.

Done.

>> LayoutTests/http/wpt/webrtc/sframe-transform-readable-crash.html:7
>> +new SFrameTransform().readable;
> 
> It is unclear why we need the requestAdapter call.

We need the whole thing, also the call to requestDevice. Also notice the test will not crash standalone but only on second or further iterations.

[youenn fablet]

> >> LayoutTests/http/wpt/webrtc/sframe-transform-readable-crash.html:7
> >> +new SFrameTransform().readable;
> > 
> > It is unclear why we need the requestAdapter call.
> 
> We need the whole thing, also the call to requestDevice. Also notice the
> test will not crash standalone but only on second or further iterations.

Why does this trigger the global object to be null?

[Rob Buis]

(In reply to youenn fablet from comment #6)
> > >> LayoutTests/http/wpt/webrtc/sframe-transform-readable-crash.html:7
> > >> +new SFrameTransform().readable;
> > > 
> > > It is unclear why we need the requestAdapter call.
> > 
> > We need the whole thing, also the call to requestDevice. Also notice the
> > test will not crash standalone but only on second or further iterations.
> 
> Why does this trigger the global object to be null?

A frameless document is handed over as context for RTCRtpSFrameTransform.

It looks like that is because this line:
await navigator.gpu.requestAdapter().then(a=>a.requestDevice());

requestDevice returns a promise and this code does not wait on it. This code does and avoids the problem:
const adapter = await navigator.gpu.requestAdapter;
const device = await adapter.requestDevice();

Do I need to debug further? I am not sure what is happening to the posted task in  WebGPUAdapter::requestDevice if the promise is not waited for, and why it happens every second iteration (these two things are probably related).

[youenn fablet]

(In reply to Rob Buis from comment #7)
> (In reply to youenn fablet from comment #6)
> > > >> LayoutTests/http/wpt/webrtc/sframe-transform-readable-crash.html:7
> > > >> +new SFrameTransform().readable;
> > > > 
> > > > It is unclear why we need the requestAdapter call.
> > > 
> > > We need the whole thing, also the call to requestDevice. Also notice the
> > > test will not crash standalone but only on second or further iterations.
> > 
> > Why does this trigger the global object to be null?
> 
> A frameless document is handed over as context for RTCRtpSFrameTransform.
> 
> It looks like that is because this line:
> await navigator.gpu.requestAdapter().then(a=>a.requestDevice());
> 
> requestDevice returns a promise and this code does not wait on it. This code
> does and avoids the problem:
> const adapter = await navigator.gpu.requestAdapter;
> const device = await adapter.requestDevice();
> 
> Do I need to debug further? I am not sure what is happening to the posted
> task in  WebGPUAdapter::requestDevice if the promise is not waited for, and
> why it happens every second iteration (these two things are probably
> related).

I think we should be able to write a simpler test case, not related to gpu, something like:

<html>
<body>
<iframe src="." id="frame"></iframe>
<script>
const transform = new frame.contentWindow.SFrameTransform;
frame.remove();
transform.readable
</script>
</body>
</html>

[youenn fablet]

Comment on attachment 417872 [details]
Patch

r=me with a simpler test case.

[Rob Buis]

Created attachment 418030 [details]
Patch

[Rob Buis]

(In reply to youenn fablet from comment #8)
> I think we should be able to write a simpler test case, not related to gpu,
> something like:
> 
> <html>
> <body>
> <iframe src="." id="frame"></iframe>
> <script>
> const transform = new frame.contentWindow.SFrameTransform;
> frame.remove();
> transform.readable
> </script>
> </body>
> </html>

Thanks! That indeed triggers the crash.

[EWS]

Committed r271690: <https://trac.webkit.org/changeset/271690>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418030 [details].

[youenn fablet]

<rdar://problem/74859450>