| Summary: | Nullptr crash in ReadableStream::create | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
| Component: | WebRTC | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, product-security, rbuis, svillar, webkit-bug-importer, youennf | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Ryosuke Niwa
2021-01-06 20:27:32 PST
Created attachment 417867 [details]
Patch
Created attachment 417868 [details]
Patch
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. Created attachment 417872 [details]
Patch
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.
> >> 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?
(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). (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> Comment on attachment 417872 [details]
Patch
r=me with a simpler test case.
Created attachment 418030 [details]
Patch
(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. Committed r271690: <https://trac.webkit.org/changeset/271690> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418030 [details]. |