Bug 180499

<rdar://problem/35890680>

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.

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.

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

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

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

Created attachment 330228 [details] Patch

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.

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.

Created attachment 330267 [details] Patch for landing

Comment on attachment 330267 [details] Patch for landing Clearing flags on attachment: 330267 Committed r226314: <https://trac.webkit.org/changeset/226314>

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

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