[webkit-reviews] review denied: [Bug 202500] Only wrapping CryptoKeys for IDB during serialization : [Attachment 380091] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Jiewen Tan <jiewen_tan at apple.com>'s
request for review:
Bug 202500: Only wrapping CryptoKeys for IDB during serialization
https://bugs.webkit.org/show_bug.cgi?id=202500

Attachment 380091: Patch

https://bugs.webkit.org/attachment.cgi?id=380091&action=review




--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 380091
  --> https://bugs.webkit.org/attachment.cgi?id=380091
Patch

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

> Source/WebCore/workers/WorkerGlobalScope.cpp:417
> +    m_thread.workerLoaderProxy().postTaskToLoader([resultContainer =
resultContainer.copyRef(), key, wrappedKeyContainer =
wrappedKeyContainer.copyRef(), doneContainer = doneContainer.copyRef(),
workerMessagingProxy =
Ref<WorkerMessagingProxy>(downcast<WorkerMessagingProxy>(m_thread.workerLoaderP
roxy()))](ScriptExecutionContext& context) {

Ref'ing the WorkerMessagingProxy and passing it to another thread like this is
not correct. If you look at the constructor / destructor of
WorkerMessagingProxy, you'll see that this object needs to be constructed and
destroyed on the same thread. The fact that it subclasses
ThreadSafeRefCounted<> does not change that.

> Source/WebCore/workers/WorkerGlobalScope.cpp:420
> +	  
((WorkerLoaderProxy&)workerMessagingProxy.get()).postTaskForModeToWorkerGlobalS
cope([](ScriptExecutionContext& context) {

No C-casts please.

> Source/WebCore/workers/WorkerGlobalScope.cpp:427
>	   waitResult = m_thread.runLoop().runInMode(this,
WorkerRunLoop::defaultMode());

What makes sure that |this| stays alive while spinning the run loop? I do not
see you holding a Ref<> to the WorkerGlobalScope, which seems wrong.

> Tools/ChangeLog:9
> +	   Modifies IndexedDB.StructuredCloneBackwardCompatibility test to
include CryptoKeys.

Do these reproduce the crash?