Bug 180499 – [WebCrypto] Avoid promises being destroyed in secondary threads

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Matt Lewis]

Created attachment 328616 [details]
Crash Log

This assertion is being seen with the test imported/w3c/web-platform-tests/WebIDL/current-realm.html however in the crash log it is blaming CRASHING TEST: imported/w3c/css/css-color-3/t425-hsla-values-b-expected.html
This is an expected file and the associated test is imported/w3c/css/css-color-3/t425-hsla-values-b.xht however this seems like the wrong test is being blamed in the crash.

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r225574%20(1134)/results.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2FWebIDL%2Fcurrent-realm.html


stderr:

ASSERTION FAILED: m_workerGlobalScope->hasOneRef()
/Volumes/Data/slave/highsierra-debug/build/Source/WebCore/workers/WorkerThread.cpp(211) : void WebCore::WorkerThread::workerThread()
1   0x71f7743ed WTFCrash
2   0x71319aab6 WebCore::WorkerThread::workerThread()
3   0x7131a6a48 WebCore::WorkerThread::start(WTF::Function<void (WTF::String const&)>&&)::$_12::operator()() const
4   0x7131a6a09 WTF::Function<void ()>::CallableWrapper<WebCore::WorkerThread::start(WTF::Function<void (WTF::String const&)>&&)::$_12>::call()
5   0x71f78d2fb WTF::Function<void ()>::operator()() const
6   0x71f7ffebf WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*)
7   0x71f805675 WTF::wtfThreadEntryPoint(void*)
8   0x7fff70e8b6c1 _pthread_body
9   0x7fff70e8b56d _pthread_body
10  0x7fff70e8ac5d thread_start
LEAK: 2 WebPageProxy

[Radar WebKit Bug Importer]

<rdar://problem/35890680>

[youenn fablet]

The test before the crashing one is imported/w3c/web-platform-tests/WebCryptoAPI/wrapKey_unwrapKey/wrapKey_unwrapKey.worker.html which does crypto functions in a worker.

[youenn fablet]

Looking at CryptoKeyRSA::generatePair, it does the following:
- ref the ScriptExecutionContext in a thread T1
- dispatch to a thread T2
- execute a task in T2
- go back to the original thread T1
- unref the ScriptExecutionContext.

I do not believe this works in practice since T1 may be stopped when executing T2 task.
Refing the context will not help going back to T1 in that case.
This is probably why we are hitting that assertion.

Also, since we might not be able to go back to T1, we should not ref anything in the callbacks otherwise, they might be destroyed in the wrong thread.

For instance, SubtleCrypto::generateKey callbacks are capturing promises.
It might be better to keep a ref of the promise in SubtleCrypto and keep a weak ref to it. Then you can probably get SubtleCrypto from the ScriptExecutionContext on the way back.

[Jiewen Tan]

(In reply to youenn fablet from comment #3)
> Looking at CryptoKeyRSA::generatePair, it does the following:
> - ref the ScriptExecutionContext in a thread T1
> - dispatch to a thread T2
> - execute a task in T2
> - go back to the original thread T1
> - unref the ScriptExecutionContext.
> 
> I do not believe this works in practice since T1 may be stopped when
> executing T2 task.
> Refing the context will not help going back to T1 in that case.
> This is probably why we are hitting that assertion.
> 
> Also, since we might not be able to go back to T1, we should not ref
> anything in the callbacks otherwise, they might be destroyed in the wrong
> thread.
> 
> For instance, SubtleCrypto::generateKey callbacks are capturing promises.
> It might be better to keep a ref of the promise in SubtleCrypto and keep a
> weak ref to it. Then you can probably get SubtleCrypto from the
> ScriptExecutionContext on the way back.

I agreed that this might be the problem. However, I am not sure if it is a good idea to keep a strong ref of the promise within SubtleCrypto. SubtleCrypto is a global object that ties with DOMWindow and WokerGlobalScope. For that solution, we might need a map in SubtleCrypto that holds a bunch of Promises and then somehow can deallocate each when an identifier is passed.

[youenn fablet]

> For that solution, we might need a map in SubtleCrypto
> that holds a bunch of Promises and then somehow can deallocate each when an
> identifier is passed.

Sure, see ServiceWorkerClients and ServiceWorkerContainers as examples of this pattern.

[Jiewen Tan]

(In reply to youenn fablet from comment #5)
> > For that solution, we might need a map in SubtleCrypto
> > that holds a bunch of Promises and then somehow can deallocate each when an
> > identifier is passed.
> 
> Sure, see ServiceWorkerClients and ServiceWorkerContainers as examples of
> this pattern.

That's cool!!!

[Jiewen Tan]

Created attachment 330228 [details]
Patch

[youenn fablet]

Comment on attachment 330228 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        promises are destroyed in secondary threads.

Change looks good to me.
There might be more changes needed to fix the assertion failure.
Maybe this patch should have a more specific name?

> Source/WebCore/crypto/SubtleCrypto.cpp:511
> +std::optional<Ref<DeferredPromise>> getPromise(DeferredPromise* index, WeakPtr<SubtleCrypto> subtleCryptoWeakPointer)

How about using RefPtr<DeferredPromise>?
We would then use releaseNonNull() after a null check to get back a Ref<DeferredPromise>.

> Source/WebCore/crypto/SubtleCrypto.cpp:553
>      };

In the future, it might be good to unify these failure/success callbacks in a unique lambda, for instance by taking a ExceptionOr<SomeResult>.

> Source/WebCore/crypto/SubtleCrypto.cpp:798
> +    auto callback = [index, subtleCryptoWeakPointer, importAlgorithm, importParams = WTFMove(importParams), extractable, keyUsagesBitmap](const Vector<uint8_t>& derivedKey) mutable {

I wonder whether importParams (or some other parameters of this callback and also other callbacks) should be isolatedCopy() or not.
If so, might be good to do that in a future refactoring.

> Source/WebCore/crypto/SubtleCrypto.h:79
>      SubtleCrypto(ScriptExecutionContext&);

Use explicit.

> Source/WebCore/crypto/SubtleCrypto.h:84
> +    HashMap<DeferredPromise*, Ref<DeferredPromise>> m_pendingPromises;

We might want to clear m_pendingPromises whenever it is no longer allowed to execute JavaScript.
One way of doing so is to make SubtleCryto an ActiveDOMObject and clear m_pendingPromises in the stop method.
We might also want to implement hasPendingActivity by checking whether m_pendingPromises is empty.

> Source/WebCore/crypto/SubtleCrypto.h:85
> +    WeakPtrFactory<SubtleCrypto> m_weakPtrFactory;

The alternative to the weak map would have been to get the SubtleCrypto from Crypto which we would get from ScriptExecutionContext.
Current approach looks good given callbacks do not have an easy way to get a ScriptExecutionContext as parameter without further refactoring.

[Jiewen Tan]

Comment on attachment 330228 [details]
Patch

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

Thanks Youenn for r+ my patch.

>> Source/WebCore/ChangeLog:11
>> +        promises are destroyed in secondary threads.
> 
> Change looks good to me.
> There might be more changes needed to fix the assertion failure.
> Maybe this patch should have a more specific name?

Certainly, how about "[WebCrypto] Avoid promises being destroyed in secondary threads"?

>> Source/WebCore/crypto/SubtleCrypto.cpp:511
>> +std::optional<Ref<DeferredPromise>> getPromise(DeferredPromise* index, WeakPtr<SubtleCrypto> subtleCryptoWeakPointer)
> 
> How about using RefPtr<DeferredPromise>?
> We would then use releaseNonNull() after a null check to get back a Ref<DeferredPromise>.

Sounds good.

>> Source/WebCore/crypto/SubtleCrypto.cpp:553
>>      };
> 
> In the future, it might be good to unify these failure/success callbacks in a unique lambda, for instance by taking a ExceptionOr<SomeResult>.

I kind of thinking of doing this when making this patch. However, it was not quite obvious for me at a very first glance. Sure, it will be a nice improvement.

>> Source/WebCore/crypto/SubtleCrypto.cpp:798
>> +    auto callback = [index, subtleCryptoWeakPointer, importAlgorithm, importParams = WTFMove(importParams), extractable, keyUsagesBitmap](const Vector<uint8_t>& derivedKey) mutable {
> 
> I wonder whether importParams (or some other parameters of this callback and also other callbacks) should be isolatedCopy() or not.
> If so, might be good to do that in a future refactoring.

What's the benefits of making them isolated copies? They are certainly not at this moment.

>> Source/WebCore/crypto/SubtleCrypto.h:79
>>      SubtleCrypto(ScriptExecutionContext&);
> 
> Use explicit.

Fixed.

>> Source/WebCore/crypto/SubtleCrypto.h:84
>> +    HashMap<DeferredPromise*, Ref<DeferredPromise>> m_pendingPromises;
> 
> We might want to clear m_pendingPromises whenever it is no longer allowed to execute JavaScript.
> One way of doing so is to make SubtleCryto an ActiveDOMObject and clear m_pendingPromises in the stop method.
> We might also want to implement hasPendingActivity by checking whether m_pendingPromises is empty.

I am not quite sure what ActiveDOMObject is. Could you illustrate a bit for me? I might need that for the CredentialManagement API as well.

>> Source/WebCore/crypto/SubtleCrypto.h:85
>> +    WeakPtrFactory<SubtleCrypto> m_weakPtrFactory;
> 
> The alternative to the weak map would have been to get the SubtleCrypto from Crypto which we would get from ScriptExecutionContext.
> Current approach looks good given callbacks do not have an easy way to get a ScriptExecutionContext as parameter without further refactoring.

Which approach you think would be better? I am currently making CredentialManagement API which will pretty much employ the very same technique.

[Jiewen Tan]

Created attachment 330267 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 330267 [details]
Patch for landing

Clearing flags on attachment: 330267

Committed r226314: <https://trac.webkit.org/changeset/226314>

[youenn fablet]

> What's the benefits of making them isolated copies? They are certainly not
> at this moment.

If you lambda-capture parameters in a thread and execute the lambda in another thread, one needs to ensure thread safety of the parameters.
Strings for instance are refcounted but not threadsafe refcounted.

> >> Source/WebCore/crypto/SubtleCrypto.h:84
> >> +    HashMap<DeferredPromise*, Ref<DeferredPromise>> m_pendingPromises;
> > 
> > We might want to clear m_pendingPromises whenever it is no longer allowed to execute JavaScript.
> > One way of doing so is to make SubtleCryto an ActiveDOMObject and clear m_pendingPromises in the stop method.
> > We might also want to implement hasPendingActivity by checking whether m_pendingPromises is empty.
> 
> I am not quite sure what ActiveDOMObject is. Could you illustrate a bit for
> me? I might need that for the CredentialManagement API as well.

ActiveDOMObjects do several things.
They are typically objects that create asynchronous tasks, which has impact in case document goes to page cache or is destroyed for instance.
ActiveDOMObjects register themselves to their context and are stopped when their context is about to be destroyed (see Document::prepareForDestruction and WorkerThread::stop for instance).

> >> Source/WebCore/crypto/SubtleCrypto.h:85
> >> +    WeakPtrFactory<SubtleCrypto> m_weakPtrFactory;
> > 
> > The alternative to the weak map would have been to get the SubtleCrypto from Crypto which we would get from ScriptExecutionContext.
> > Current approach looks good given callbacks do not have an easy way to get a ScriptExecutionContext as parameter without further refactoring.
> 
> Which approach you think would be better? I am currently making
> CredentialManagement API which will pretty much employ the very same
> technique.

The weak map is a more general approach.
If your implementation is using routines like postTask/postTaskToLoader, it would seem natural to get Crypto from the ScriptExecutionContext.

[Jiewen Tan]

*** Bug 170907 has been marked as a duplicate of this bug. ***