WebCrypto: Add interfaces for importKey().

WebCrypto: Add interfaces for importKey().

Adds WebKit API interfaces importKey(). Also refactors CryptoOperation to match.

The key operations support both synchronous and asynchronous completions, as well as a separate synchronous initialization failure. The asynchronous operation is abortable (however nothing aborts it yet)

In order to test this, added a mock implementation of the interfaces which hardcodes some actions based on the bytes of the key. This allows for layout tests that verify the parsing and reflection of parameters.

BUG=245025
R=abarth@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154860

[eroman, 2013-07-20 06:31:06]

https://codereview.chromium.org/19885002/diff/2001/Source/modules/crypto/Key.idl
File Source/modules/crypto/Key.idl (left):

https://codereview.chromium.org/19885002/diff/2001/Source/modules/crypto/Key....
Source/modules/crypto/Key.idl:37: enum KeyUsage {
I removed these because it seemed to just duplicate the parsing I do for
SubtleCrypto. Seemed worse to have to update even more places when something was
changed/added.

[abarth-chromium, 2013-07-20 06:31:59]

https://codereview.chromium.org/19885002/diff/2001/Source/modules/crypto/Subt...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:108: class MockPlatformCrypto : public
WebKit::WebCrypto {
It would be better if these mock implementations where in the TestRunner
library.  Then we wouldn't have to include them in the production binary...

[eroman, 2013-07-22 17:02:06]

https://codereview.chromium.org/19885002/diff/2001/Source/modules/crypto/Subt...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:108: class MockPlatformCrypto : public
WebKit::WebCrypto {
On 2013/07/20 06:32:00, abarth wrote:
> It would be better if these mock implementations where in the TestRunner
> library.  Then we wouldn't have to include them in the production binary...

Thanks Adam.

I am trying to wrap my head around how the test-only stuff gets loaded for
content_shell. I did a search for TestRunner but it turned up only a file in
DumpRenderTree/*, which I assumed was no longer relevant.

Will keep digging, but any extra guidance appreciated :)

[abarth-chromium, 2013-07-22 18:12:53]

On 2013/07/22 17:02:06, eroman wrote:
> I am trying to wrap my head around how the test-only stuff gets loaded for
> content_shell. I did a search for TestRunner but it turned up only a file in
> DumpRenderTree/*, which I assumed was no longer relevant.

That's still relevant.  We just haven't moved the code to its new home yet.

> Will keep digging, but any extra guidance appreciated :)

If you pass --dump-render-tree to content_shell, we'll load in TestRunner.so,
which contains a bunch of Mock implementation for WebKit::Platform concepts. 
Take a look at how MockWebAudioDevice gets wired in:

https://code.google.com/p/chromium/codesearch#search/&q=MockWebAudioDevice&sq...

[eroman, 2013-07-23 01:37:25]

PTAL: I have moved the mocks into the TestRunner library.

The corresponding blink side changes are:
https://codereview.chromium.org/19594005

[eroman, 2013-07-23 01:38:22]

https://codereview.chromium.org/19885002/diff/10001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/19885002/diff/10001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1729: Bug(eroman) crypto/  [ Skip ]
This seems wrong. Perhaps it is better if after this is reviewed, I split it up
for a 3-sided patch which does not require disabling any tests.

[abarth-chromium, 2013-07-23 06:22:07]

https://codereview.chromium.org/19885002/diff/10001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/19885002/diff/10001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1729: Bug(eroman) crypto/  [ Skip ]
On 2013/07/23 01:38:22, eroman wrote:
> This seems wrong. Perhaps it is better if after this is reviewed, I split it
up
> for a 3-sided patch which does not require disabling any tests.

I think it's fine.  You're the only one working in this area.  We're unlikely to
get into too much trouble if these tests are disabled for a bit.

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Key...
File Source/modules/crypto/Key.cpp (right):

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Key...
Source/modules/crypto/Key.cpp:95: return WebKit::WebCryptoKeyUsageUnwrapKey;
Maybe use a single table to drive both?  The table is small, so the reverse
lookup should be fast.  That seems less fragile than having to keep these two
functions in sync.

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Nor...
File Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Nor...
Source/modules/crypto/NormalizeAlgorithm.cpp:191: bool getAlgorithmInfo(const
Dictionary& raw, const AlgorithmInfo*& info, ExceptionCode& ec)
We generally try to avoid using "get" as a verb in function names.  It's a weak
verb that doesn't really mean anything.  In this case, it might make sense
because you're extracting this information from a dictionary, and the main
operation on a dictionary is "get"...  I guess its fine.

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Nor...
Source/modules/crypto/NormalizeAlgorithm.cpp:220: const AlgorithmInfo* info;
Please initialize scalars.

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:49: PassRefPtr<CryptoOperation>
doDummyOperation(const Dictionary& rawAlgorithm, AlgorithmOperation
operationType, ExceptionCode& ec)
Like "get", "do" is a weak verb.  Perhaps "dummyOperation"?

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:93: ExceptionCode* m_exceptionCode;
I don't understand how KeyOperation can hold a pointer to m_exceptionCode.  The
ExceptionCode is a stack-allocated object, which means it dies after the stack
unwinds.  The KeyOperation appears to live in the heap and complete its work
asynchronously.  That seems to imply that KeyOperation outlives m_exceptionCode
and therefore cannot keep a raw pointer to m_exceptionCode.

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:123: delete this;
This line is suspicious and indicates a bad memory management pattern...  Is
there a better way to structure the ownership relationships between these
objects?

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:241: KeyOperation* op = new
KeyOperation(promiseResolver.get(), &ec);
This is a "naked new", which is where the bad memory pattern originates.  All
calls to "new" should be immediately wrapped in either adoptRef (for ref-counted
objects) or adoptPtr (for non-ref-counted objects).  We prefer to avoid these
sorts of complicated lifetime management patterns because they're fragile and
lead to memory corruption or leaks.  You might be smart enough to author this
code correctly, but, as a group, we're not smart enough to maintain them.

https://codereview.chromium.org/19885002/diff/10001/Tools/DumpRenderTree/chro...
File Tools/DumpRenderTree/chromium/TestRunner/src/MockWebCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/10001/Tools/DumpRenderTree/chro...
Tools/DumpRenderTree/chromium/TestRunner/src/MockWebCrypto.cpp:33: #include
"base/sha1.h"
You're not allowed to depend directly on base from Blink.  If you do, the folks
who maintain base will be sad because they'll be unable to change base without
writing complicated Blink CLs.

Maybe it's not worth trying to mock this functionality out.  It's not going to
scale to the whole API..  Why not complete the implementation in the Content
layer?  You're not going to be able to avoid that work for long anyway.

[eroman, 2013-07-23 06:53:41]

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:93: ExceptionCode* m_exceptionCode;
On 2013/07/23 06:22:07, abarth wrote:
> I don't understand how KeyOperation can hold a pointer to m_exceptionCode. 
The
> ExceptionCode is a stack-allocated object, which means it dies after the stack
> unwinds.  The KeyOperation appears to live in the heap and complete its work
> asynchronously.  That seems to imply that KeyOperation outlives
m_exceptionCode
> and therefore cannot keep a raw pointer to m_exceptionCode.

It is a little bit tricky, but the m_exceptionCode pointer is cleared before it
goes out of scope.

Upon calling into the Platform key operation, it either calls:
   initializationSucceeded(), initializationFailed(), completeWithKey() or
completeWithError(). These all drop the pointer.

I could instead have them buffer the exception code, and then call another
function to explicitly set it.

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:241: KeyOperation* op = new
KeyOperation(promiseResolver.get(), &ec);
On 2013/07/23 06:22:07, abarth wrote:
> This is a "naked new", which is where the bad memory pattern originates.  All
> calls to "new" should be immediately wrapped in either adoptRef (for
ref-counted
> objects) or adoptPtr (for non-ref-counted objects).  We prefer to avoid these
> sorts of complicated lifetime management patterns because they're fragile and
> lead to memory corruption or leaks.  You might be smart enough to author this
> code correctly, but, as a group, we're not smart enough to maintain them.

I'll think about this some more and see what else I can come up with.
I modeled this similarly to CryptoOperation.

I wanted to have a simple interface for the embedder which can allow them to
notify sync/async completion/error in a consistent manner, since there will be
many such operations implemented on top of this.

Because both the embedder and blink hold pointers to each other reference
counting would cause a cycle and I ended up with explicit deletions

https://codereview.chromium.org/19885002/diff/10001/Tools/DumpRenderTree/chro...
File Tools/DumpRenderTree/chromium/TestRunner/src/MockWebCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/10001/Tools/DumpRenderTree/chro...
Tools/DumpRenderTree/chromium/TestRunner/src/MockWebCrypto.cpp:33: #include
"base/sha1.h"
On 2013/07/23 06:22:07, abarth wrote:
> You're not allowed to depend directly on base from Blink.  If you do, the
folks
> who maintain base will be sad because they'll be unable to change base without
> writing complicated Blink CLs.
> 
> Maybe it's not worth trying to mock this functionality out.  It's not going to
> scale to the whole API..  Why not complete the implementation in the Content
> layer?  You're not going to be able to avoid that work for long anyway.

I saw that DumpRenderTree already depended on "base" so I thought it was fair
game:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

That said, I can just hardcode certain input/output pairs.

I agree that the implementation should just go in Content layer, and that work
is in progress (https://codereview.chromium.org/19757011/).
The hack I had was intended to be short lived so I could test the blink side I
had written. However moving it to TestRunner makes it feel more legitimate.

[abarth-chromium, 2013-07-23 07:06:14]

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:93: ExceptionCode* m_exceptionCode;
On 2013/07/23 06:53:41, eroman wrote:
> It is a little bit tricky, but the m_exceptionCode pointer is cleared before
it
> goes out of scope.

That's too tricky.  Is there a simpler design?

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:241: KeyOperation* op = new
KeyOperation(promiseResolver.get(), &ec);
On 2013/07/23 06:53:41, eroman wrote:
> I'll think about this some more and see what else I can come up with.
> I modeled this similarly to CryptoOperation.

We might want to change CryptoOperation to follow whatever pattern we come up
with here.

> I wanted to have a simple interface for the embedder which can allow them to
> notify sync/async completion/error in a consistent manner, since there will be
> many such operations implemented on top of this.

Rather than returning a raw pointer to the embedder, perhaps we should return a
value type that acts like a smart pointer.  We can implement this with RefPtr
internally to allow the value type to be copied on the embedder's side, but the
expectation is that the embedder will eventually call the success or failure
function on the API object, which will clear any references the WebCore side has
to any other objects (making it a leaf in the object graph and breaking any
cycles).

That approach doesn't add any extra complexity because we already need a similar
guarantee to make sure we resolve the Promise (and thereby breaking the strong
reference from the ScriptPromiseResolver into the JavaScript heap).

> Because both the embedder and blink hold pointers to each other reference
> counting would cause a cycle and I ended up with explicit deletions

Hopefully the approach above resolves any reference cycles.

[eroman, 2013-07-23 18:10:56]

Thanks Adam.

I have responded to your comment in this google doc (in order to include some
diagrams):

https://docs.google.com/a/google.com/document/d/1W7In-MzL5hPwwto5hPBGMEYe536R...

(Unfortunately I started it on @google.com domain, and haven't figured out how
to make it visible to all yet)

[abarth-chromium, 2013-07-23 18:18:50]

I don't seem to be able to comment on the doc, so I'm replying here:

"Would need to create an equivalent of RefPtr<WebCryptoKeyOperationResult> in
the WebKit API that the embedder can use."

We already have that.  Take a look at WebNode to see how the pattern works. 
It's a widely used pattern in the API.

"the embedder would have to additionally release its reference to the
WebCryptoKeyOperationResult"

That's not correct.  Instead of writing:

> result_->completeWithError(...);

The embedder would write:

> result_.completeWithError(...);

In the implementation of completeWithError, the result object would drop all
references to other objects, breaking any cycles.

"Having to manually release the cycle at each completion site adds more
boilerplate and room for error"

Agreed, but that's not what I meant.  :)

"I can adjust the current design so that KeyOperation does not hold a pointer to
the ExceptionCode."

What I'd recommend is having the code that creates the KeyOperation ask the
KeyOperation object whether it had an exception after giving it to the embedder.
 If so, then set the ec local variable normally.

"My design goal is to keep the interface on the embedder’s side as simple as
possible, and hard to mis-use, even if the tradeoff is more complexity on the
Blink side."

I believe we can get both.

We've managed to avoid manual memory management everywhere else in the Blink
codebase.  I don't think there's anything particularly special about WebCrypto
that requires us to result to raw pointers and manual calls to delete.

[abarth-chromium, 2013-07-23 18:19:23]

s/result/resort/

[eroman, 2013-07-23 21:31:33]

I have changed to giving the embedder a result object rather than pointer in
patchset 3 as discussed.

WDYT?

The good:
  - protects the embedder from calling completion methods more than once (before
this error would have been a use-after-free, now a null pointer crash)

The bad:
  - More wrapping layers
  - Copying of the result wrappers required making KeyOperation
RefCountedThreadSafe, in case embedder tries to copy it across threads

Unchanged:
  - Doesn't protect against using the result object after abortion (this error
will result in a use-after-free).

[abarth-chromium, 2013-07-23 21:45:48]

Thanks for iterating on the design.  This CL LGTM with the comments below
addressed.

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Cry...
File Source/modules/crypto/CryptoOperation.h (right):

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.h:87: CryptoOperation(const
WebKit::WebCryptoAlgorithm&);
Please mark one-argument constructors "explicit"

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Key...
File Source/modules/crypto/Key.cpp (right):

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Key...
Source/modules/crypto/Key.cpp:56: // NOTE: Keep this in sync with
keyUsageStringToMask() below.
IMHO, a table would be more maintainable here, but this is ok too.

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Sub...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:108: return op->returnValue(ec);
Thanks.  This looks much better.

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:142: RefPtr<KeyOperation> keyOp =
adoptRef(new KeyOperation);
Usually classes have a static "create" function that does the adoptRef.  That
way we can make the constructor private and force everyone to go through the
create function.

https://codereview.chromium.org/19885002/diff/32001/Tools/DumpRenderTree/chro...
File Tools/DumpRenderTree/chromium/TestRunner/src/MockWebCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/32001/Tools/DumpRenderTree/chro...
Tools/DumpRenderTree/chromium/TestRunner/src/MockWebCrypto.cpp:33: #include
"base/sha1.h"
You've still got the dependency on base here.  It's important to remove it so
that the folks who maintain base don't get mad at us when changes to base break
the Blink build.

https://codereview.chromium.org/19885002/diff/32001/public/platform/WebCrypto.h
File public/platform/WebCrypto.h (right):

https://codereview.chromium.org/19885002/diff/32001/public/platform/WebCrypto...
public/platform/WebCrypto.h:178: WebCore::CryptoOperation* m_impl;
Should this be a WebPrivatePtr<WebCore::CryptoOperation> ?

[abarth-chromium, 2013-07-23 21:47:23]

On 2013/07/23 06:53:41, eroman wrote:
> I saw that DumpRenderTree already depended on "base" so I thought it was fair
> game:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> That said, I can just hardcode certain input/output pairs.

There's a build-time dependency on base, but we're very careful about what
details of base we depend on.  The reason is that we don't want to constrain the
folks who work on base.  If we start including and using a bunch of headers from
base, then they won't be able to change those headers without painful
multi-sided Cls.

[eroman, 2013-07-23 23:29:02]

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Key...
File Source/modules/crypto/Key.cpp (right):

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Key...
Source/modules/crypto/Key.cpp:95: return WebKit::WebCryptoKeyUsageUnwrapKey;
On 2013/07/23 06:22:07, abarth wrote:
> Maybe use a single table to drive both?  The table is small, so the reverse
> lookup should be fast.  That seems less fragile than having to keep these two
> functions in sync.

Done.

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Nor...
File Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Nor...
Source/modules/crypto/NormalizeAlgorithm.cpp:191: bool getAlgorithmInfo(const
Dictionary& raw, const AlgorithmInfo*& info, ExceptionCode& ec)
On 2013/07/23 06:22:07, abarth wrote:
> We generally try to avoid using "get" as a verb in function names.  It's a
weak
> verb that doesn't really mean anything.  In this case, it might make sense
> because you're extracting this information from a dictionary, and the main
> operation on a dictionary is "get"...  I guess its fine.

Done.

https://codereview.chromium.org/19885002/diff/10001/Source/modules/crypto/Nor...
Source/modules/crypto/NormalizeAlgorithm.cpp:220: const AlgorithmInfo* info;
On 2013/07/23 06:22:07, abarth wrote:
> Please initialize scalars.

Done.

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Cry...
File Source/modules/crypto/CryptoOperation.h (right):

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.h:87: CryptoOperation(const
WebKit::WebCryptoAlgorithm&);
On 2013/07/23 21:45:49, abarth wrote:
> Please mark one-argument constructors "explicit"

Done.

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Key...
File Source/modules/crypto/Key.cpp (right):

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Key...
Source/modules/crypto/Key.cpp:56: // NOTE: Keep this in sync with
keyUsageStringToMask() below.
On 2013/07/23 21:45:49, abarth wrote:
> IMHO, a table would be more maintainable here, but this is ok too.

Done.

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Sub...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/32001/Source/modules/crypto/Sub...
Source/modules/crypto/SubtleCrypto.cpp:142: RefPtr<KeyOperation> keyOp =
adoptRef(new KeyOperation);
On 2013/07/23 21:45:49, abarth wrote:
> Usually classes have a static "create" function that does the adoptRef.  That
> way we can make the constructor private and force everyone to go through the
> create function.

Done.

https://codereview.chromium.org/19885002/diff/32001/public/platform/WebCrypto.h
File public/platform/WebCrypto.h (right):

https://codereview.chromium.org/19885002/diff/32001/public/platform/WebCrypto...
public/platform/WebCrypto.h:178: WebCore::CryptoOperation* m_impl;
On 2013/07/23 21:45:49, abarth wrote:
> Should this be a WebPrivatePtr<WebCore::CryptoOperation> ?

Done.

(I was initially concerned about refcounting happening from another thread; made
CryptoOperation RefCountedThreadSafe to be safe)

[eroman, 2013-07-23 23:49:16]

One issue I have found with using the wrapper result object, is it makes
unittests on the embedder side trickier -- you cannot simply provide a mock
implementation of the result. Instead need to expose additional methods for
testing.

[abarth-chromium, 2013-07-24 00:05:01]

On 2013/07/23 23:49:16, eroman wrote:
> One issue I have found with using the wrapper result object, is it makes
> unittests on the embedder side trickier -- you cannot simply provide a mock
> implementation of the result. Instead need to expose additional methods for
> testing.

Maybe we should do most of the testing from inside LayoutTests?

[eroman, 2013-07-24 01:06:04]

I have uploaded one more patchset, which includes a way to construct mockable
result objects for tests.

(Bryan is already depending on being able to write unit-tests for the blink side
in https://codereview.chromium.org/19757011/ so I would like to preserve that
ability)

Sorry for all the back and forth!

[abarth-chromium, 2013-07-24 01:19:10]

I'm not super excited about all this test-specific code, but I also don't want
to hold up this CL over that issue.  I'll leave it to your judgement.

https://codereview.chromium.org/19885002/diff/51002/Source/core/platform/chro...
File Source/core/platform/chromium/support/WebCrypto.cpp (right):

https://codereview.chromium.org/19885002/diff/51002/Source/core/platform/chro...
Source/core/platform/chromium/support/WebCrypto.cpp:115: m_testInterface = 0;
So much test-specific code...

[eroman, 2013-07-24 01:46:01]

Being able to test on the blink side helps with the iterative development
process.

I would like to keep the test interface at least in the short term while Bryan
and I are working on this.

[eroman, 2013-07-24 01:46:18]

s/blink/chromium

[eroman, 2013-07-24 02:45:36]

Sigh, this is going to need some more work :(

It occurs to me that holding a reference to the CryptoOperation from the result
wrapper is a bad idea, since it will result in a leak if an operation is
started, but the javascript never calls finish().

(which in fact I have a layout test for)

I abort from the ~CryptoOperation destructor, but now that will never be
reached.

I will let you know when this is fixed.

[eroman, 2013-07-24 18:46:44]

Fixed the issues in patchset 8:

(1)  Removed the m_testInterface pollution:

https://codereview.chromium.org/19885002/diff2/51002:71001/Source/core/p
latform/chromium/support/WebCrypto.cpp

(2) Split up CryptoOperation into a seprate reference-counted core
(CryptoOperationImpl) so the result handle does not extend the lifetime of
CryptoOperation. Explained in the comments here:

https://codereview.chromium.org/19885002/diff2/51002:71001/Source/modules/cry...

[abarth-chromium, 2013-07-24 22:40:28]

I'm sorry, this CL has gotten too big and I've read it too many times to be able
to give you a high-quality review.  If you're happy with it, let's land it and
iterate.

https://codereview.chromium.org/19885002/diff/71001/Source/modules/crypto/Cry...
File Source/modules/crypto/CryptoOperation.h (right):

https://codereview.chromium.org/19885002/diff/71001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.h:70: class CryptoOperationImpl : public
WebKit::WebCryptoOperationResultInterface, public
ThreadSafeRefCounted<CryptoOperationImpl> {
WebCryptoOperationResultInterface -> WebCryptoOperationResultPrivate ? 
Everything is an "interface"

[abarth-chromium, 2013-07-24 22:40:33]

LGTM

[eroman, 2013-07-24 23:36:25]

Committed patchset #9 manually as r154860 (presubmit successful).