<rdar://problem/88327814>
Created attachment 451567 [details] Patch
Comment on attachment 451567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451567&action=review > Source/WebCore/ChangeLog:9 > + could go through a path that invokes GC on its owner and it. 'and it' is probably missing a word.
(In reply to youenn fablet from comment #2) > Comment on attachment 451567 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451567&action=review > > > Source/WebCore/ChangeLog:9 > > + could go through a path that invokes GC on its owner and it. > > 'and it' is probably missing a word. Here, I am referring to the promise (i.e. "Keep promise in scope as ... invokes GC on its owner and it.")
Created attachment 451714 [details] Patch
Committed r289640 (247145@main): <https://commits.webkit.org/247145@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451714 [details].
Comment on attachment 451714 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451714&action=review > Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:135 > + Ref protectedThis(*this); This is not the correct fix given our overall approach to reference counting. The burden of protecting "this" is on the caller, not the reject function. I suppose that consistently protecting the wrapped object in every DOM function might be prohibitive, but we chose that approach a while back, and I am worried about mixing approaches.
(In reply to Darin Adler from comment #6) > Comment on attachment 451714 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451714&action=review > > > Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:135 > > + Ref protectedThis(*this); > > This is not the correct fix given our overall approach to reference > counting. The burden of protecting "this" is on the caller, not the reject > function. I suppose that consistently protecting the wrapped object in every > DOM function might be prohibitive, but we chose that approach a while back, > and I am worried about mixing approaches. Thank you for the context on the convention. I have updated the patch so that the caller protects the deferred promise.
Created attachment 451924 [details] Patch
Comment on attachment 451924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451924&action=review > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:274 > + Ref protectedPromise { promise }; Seems this doesn’t make sense. The "promise" captured is already protected, so this wouldn’t have any effect.
(In reply to Darin Adler from comment #9) > Comment on attachment 451924 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451924&action=review > > > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:274 > > + Ref protectedPromise { promise }; > > Seems this doesn’t make sense. The "promise" captured is already protected, > so this wouldn’t have any effect. The lambda owns the promise, but during execution, the lambda and its owner hierarchy (i.e. FetchBodyConsumer, FormDataConsumer, ...) are deallocated during garbage collection while calling DeferredPromise::reject. This can be see in the backtrace attached to the radar.
Comment on attachment 451924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451924&action=review >>> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:274 >>> + Ref protectedPromise { promise }; >> >> Seems this doesn’t make sense. The "promise" captured is already protected, so this wouldn’t have any effect. > > The lambda owns the promise, but during execution, the lambda and its owner hierarchy (i.e. FetchBodyConsumer, FormDataConsumer, ...) are deallocated during garbage collection while calling DeferredPromise::reject. > > This can be see in the backtrace attached to the radar. Given this maybe we should have a local variable at the top of the entire function: [..., capturedPromise = WTFMove(promise), ...] auto promise = WTFMove(capturedPromise); This might be a good idiom for situations like this, not just this one case.
(In reply to Darin Adler from comment #11) > Comment on attachment 451924 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451924&action=review > > >>> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:274 > >>> + Ref protectedPromise { promise }; > >> > >> Seems this doesn’t make sense. The "promise" captured is already protected, so this wouldn’t have any effect. > > > > The lambda owns the promise, but during execution, the lambda and its owner hierarchy (i.e. FetchBodyConsumer, FormDataConsumer, ...) are deallocated during garbage collection while calling DeferredPromise::reject. > > > > This can be see in the backtrace attached to the radar. > > Given this maybe we should have a local variable at the top of the entire > function: > > [..., capturedPromise = WTFMove(promise), ...] > > auto promise = WTFMove(capturedPromise); > > This might be a good idiom for situations like this, not just this one case. Thank you for the recommendation, I will update the patch and add this idiom to my notes to add in similar situations.
Created attachment 451943 [details] Patch
The updated patch is resulting in a crash that I am now investigating: stderr: ASSERTION FAILED: m_ptr /Volumes/Data/worker/macOS-AppleSilicon-Big-Sur-Debug-Build-EWS/build/WebKitBuild/Debug/usr/local/include/wtf/Ref.h(129) : T &WTF::Ref<WebCore::DeferredPromise, WTF::RawPtrTraits<WebCore::DeferredPromise>>::leakRef() [T = WebCore::DeferredPromise, Traits = WTF::RawPtrTraits<WebCore::DeferredPromise>] 1 0x13db56258 WTFCrash 2 0x118b0a7b0 WebCore::JSDOMGlobalObject* JSC::jsCast<WebCore::JSDOMGlobalObject*, JSC::JSGlobalObject>(JSC::JSGlobalObject*) 3 0x11a3dfbe8 WTF::Ref<WebCore::DeferredPromise, WTF::RawPtrTraits<WebCore::DeferredPromise> >::leakRef() 4 0x11a3dfb18 WTF::Ref<WebCore::DeferredPromise, WTF::RawPtrTraits<WebCore::DeferredPromise> >::Ref(WTF::Ref<WebCore::DeferredPromise, WTF::RawPtrTraits<WebCore::DeferredPromise> >&&) 5 0x118e01438 WTF::Ref<WebCore::DeferredPromise, WTF::RawPtrTraits<WebCore::DeferredPromise> >::Ref(WTF::Ref<WebCore::DeferredPromise, WTF::RawPtrTraits<WebCore::DeferredPromise> >&&) 6 0x11a672844 auto WebCore::FetchBodyConsumer::resolveWithFormData(WTF::Ref<WebCore::DeferredPromise, WTF::RawPtrTraits<WebCore::DeferredPromise> >&&, WTF::String const&, WebCore::FormData const&, WebCore::ScriptExecutionContext*)::$_0::operator()<WebCore::ExceptionOr<WTF::Span<unsigned char const, 18446744073709551615ul> > >(WebCore::ExceptionOr<WTF::Span<unsigned char const, 18446744073709551615ul> >&&) 7 0x11a6726a8 WTF::Detail::CallableWrapper<WebCore::FetchBodyConsumer::resolveWithFormData(WTF::Ref<WebCore::DeferredPromise, WTF::RawPtrTraits<WebCore::DeferredPromise> >&&, WTF::String const&, WebCore::FormData const&, WebCore::ScriptExecutionContext*)::$_0, void, WebCore::ExceptionOr<WTF::Span<unsigned char const, 18446744073709551615ul> > >::call(WebCore::ExceptionOr<WTF::Span<unsigned char const, 18446744073709551615ul> >) 8 0x11a66bd6c WTF::Function<void (WebCore::ExceptionOr<WTF::Span<unsigned char const, 18446744073709551615ul> >)>::operator()(WebCore::ExceptionOr<WTF::Span<unsigned char const, 18446744073709551615ul> >) const 9 0x11a66bf9c WebCore::FormDataConsumer::consume(WTF::Span<unsigned char const, 18446744073709551615ul>) 10 0x11a6960e4 auto WebCore::FormDataConsumer::consumeFile(WTF::String const&)::$_24::operator()()::'lambda'(auto&)::operator()<WebCore::ScriptExecutionContext>(auto&) const 11 0x11a695b94 WTF::Detail::CallableWrapper<WebCore::FormDataConsumer::consumeFile(WTF::String const&)::$_24::operator()()::'lambda'(auto&), void, WebCore::ScriptExecutionContext&>::call(WebCore::ScriptExecutionContext&) 12 0x11aec45e0 WTF::Function<void (WebCore::ScriptExecutionContext&)>::operator()(WebCore::ScriptExecutionContext&) const 13 0x11aeae160 WebCore::ScriptExecutionContext::Task::performTask(WebCore::ScriptExecutionContext&) 14 0x11de8dac8 WebCore::WorkerDedicatedRunLoop::Task::performTask(WebCore::WorkerOrWorkletGlobalScope*) 15 0x11de8cfd8 WebCore::WorkerDedicatedRunLoop::runInMode(WebCore::WorkerOrWorkletGlobalScope*, WebCore::ModePredicate const&) 16 0x11de8cc44 WebCore::WorkerDedicatedRunLoop::run(WebCore::WorkerOrWorkletGlobalScope*) 17 0x11de569b0 WebCore::WorkerOrWorkletThread::runEventLoop() 18 0x11df289ac WebCore::ServiceWorkerThread::runEventLoop() 19 0x11de56d28 WebCore::WorkerOrWorkletThread::workerOrWorkletThread() 20 0x11deaca14 WebCore::WorkerThread::createThread()::$_4::operator()() const 21 0x11deac990 WTF::Detail::CallableWrapper<WebCore::WorkerThread::createThread()::$_4, void>::call() 22 0x13db81018 WTF::Function<void ()>::operator()() const 23 0x13dc5c1e8 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) 24 0x13dc6a860 WTF::wtfThreadEntryPoint(void*) 25 0x19a2db878 _pthread_start 26 0x19a2d65e0 thread_start LEAK: 1 WebPageProxy
(In reply to Gabriel Nava Marino from comment #14) > The updated patch is resulting in a crash that I am now investigating: I chatted with Garbiel about this on slack. This lambda is called more than once, so it's wrong to WTFMove the promise out unconditionally - It should only be moved out when it's needed for the call to reject()
Created attachment 452425 [details] Patch
Comment on attachment 452425 [details] Patch Moving to cq? as I noticed the iOS-wk2 test that failed is also having issues in at least one unrelated patch.
Committed r290152 (247491@main): <https://commits.webkit.org/247491@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452425 [details].