Bug 236454 – Keep promise in scope when calling DeferredPromise::reject

• Component: New Bugs
• Status: RESOLVED FIXED

[Gabriel Nava Marino]

<rdar://problem/88327814>

[Gabriel Nava Marino]

Created attachment 451567 [details]
Patch

[youenn fablet]

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.

[Gabriel Nava Marino]

(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.")

[Gabriel Nava Marino]

Created attachment 451714 [details]
Patch

[EWS]

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].

[Darin Adler]

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.

[Gabriel Nava Marino]

(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.

[Gabriel Nava Marino]

Created attachment 451924 [details]
Patch

[Darin Adler]

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.

[Gabriel Nava Marino]

(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.

[Darin Adler]

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.

[Gabriel Nava Marino]

(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.

[Gabriel Nava Marino]

Created attachment 451943 [details]
Patch

[Gabriel Nava Marino]

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

[Brady Eidson]

(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()

[Gabriel Nava Marino]

Created attachment 452425 [details]
Patch

[Gabriel Nava Marino]

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.

[EWS]

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].