[webkit-reviews] review denied: [Bug 183602] imported/w3c/web-platform-tests/WebCryptoAPI/wrapKey_unwrapKey/wrapKey_unwrapKey.worker.html is crashing : [Attachment 335725] Patch

[bugzilla-daemon at webkit.org]

Jiewen Tan <jiewen_tan at apple.com> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 183602:
imported/w3c/web-platform-tests/WebCryptoAPI/wrapKey_unwrapKey/wrapKey_unwrapKe
y.worker.html is crashing
https://bugs.webkit.org/show_bug.cgi?id=183602

Attachment 335725: Patch

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




--- Comment #6 from Jiewen Tan <jiewen_tan at apple.com> ---
Comment on attachment 335725
  --> https://bugs.webkit.org/attachment.cgi?id=335725
Patch

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

Hi Youenn, thanks for making this patch. However, I don't think a lock is
necessary. A context can be referred to Document or WorkerGlobalScope, as far
as I understood.

In Document, the postTask method invokes callOnMainThread() which has a lock
already.
In WorkerGlobalScope, the postTask method ends up with MessageQueue::append()
which has a lock as well.

Therefore, I am not sure if adding a lock is useful here. I propose we should
just get rid of the context.ref() and context.deref() to see if that works. r-
because of the locking mechanism. Will talk to Youenn offline.

> Source/WebCore/ChangeLog:10
> +	   Use that method in Crypto instead of refing/unrefing  the context.

Extra space.

> Source/WebCore/dom/Document.cpp:571
> +    allDocumentsMap().remove(m_identifier);

Is this relevant to this patch?