[webcrypto] Make it safe to delete WebCryptoResult from any thread.

[webcrypto] Make it safe to delete WebCryptoResult from any thread.

It used to be that WebCrytoResult had to be deleted from the original Blink thread which created it. This posed a challenge for embedders that wanted to defer the work to a background thread. Since it was possible that by the time the work had completed, the original Blink thread was gone (if it was a WebWorker).

The approach taken in this changelist is to split out the state tied to an ExecutionContext into a separate WeakPtr which deletes itself on request completion, or when the ExecutionContext is destroyed.

BUG=366840, 245025
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172696

[eroman, 2014-04-25 02:33:36]

I am not sure if this is the best way to address my problem, please review the
approach.

Thanks!

[abarth-chromium, 2014-04-25 03:49:45]

This seems quite error prone...  I wonder, instead of using
RefCountedThreadSafe, can you use WeakPtr?  The idea is that the interface
object has a WeakPtr to its state on the worker thread.  If that state dies on
the worker thread, then the WeakPtr doesn't lead anywhere.

[eroman, 2014-04-25 18:27:28]

Thanks Adam. To make sure I understand the proposal, are you suggesting that:

 * blink::WebCryptoResult holds a WeakPtr to a CryptoResultImpl

 * On completion or context destruction, the CryptoResultImpl deletes itself,
and WebCryptoResult can no longer get at it.

[abarth-chromium, 2014-04-25 19:30:14]

On 2014/04/25 18:27:28, eroman wrote:
> Thanks Adam. To make sure I understand the proposal, are you suggesting that:
> 
>  * blink::WebCryptoResult holds a WeakPtr to a CryptoResultImpl
> 
>  * On completion or context destruction, the CryptoResultImpl deletes itself,
> and WebCryptoResult can no longer get at it.

Yeah, something like that. You know more of the details than I do. :)

[eroman, 2014-04-25 22:24:14]

PTAL, updated to use WeakPtr

[abarth-chromium, 2014-04-25 23:26:13]

LGTM

You might want to update the CL description to explain what you ended up with.

https://codereview.chromium.org/253563002/diff/20001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/253563002/diff/20001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:62: ExecutionContext* context =
callingExecutionContext(v8::Isolate::GetCurrent());
Can we pass in the isolate?

[eroman, 2014-04-26 00:23:43]

Updated the description.

https://codereview.chromium.org/253563002/diff/20001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/253563002/diff/20001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:62: ExecutionContext* context =
callingExecutionContext(v8::Isolate::GetCurrent());
On 2014/04/25 23:26:14, abarth wrote:
> Can we pass in the isolate?

Done (I think).

[abarth-chromium, 2014-04-26 01:25:03]

lgtm

[eroman, 2014-04-26 01:52:08]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-04-26 01:57:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/253563002/80001

[commit-bot: I haz the power, 2014-04-26 03:29:42]

Change committed as 172696