WebCrypto: Remove support for multi-part operations.

WebCrypto: Remove support for multi-part operations.

This matches the recent change to the standard:
https://dvcs.w3.org/hg/webcrypto-api/rev/a0dce4c0a74d

In particular, CryptoOperation does not exist anymore (i.e. no CryptoOperation.process(), CryptoOperation.abort(), CryptoOperation.finish(), CryptoOperation.algorithm).

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

[eroman, 2013-08-16 03:21:20]

Adam/James: I am hoping this will address your concerns on the ownership.

[abarth-chromium, 2013-08-16 04:53:06]

https://codereview.chromium.org/23164012/diff/2001/Source/core/platform/chrom...
File Source/core/platform/chromium/support/WebCrypto.cpp (right):

https://codereview.chromium.org/23164012/diff/2001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCrypto.cpp:31: #include <string.h> //
for memcpy()
Please alphabetize this include with the rest of the includes.  Also, there's no
need for the comment.  It's going to get out of date very fast.

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

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:36: #include
"bindings/v8/custom/V8ArrayBufferCustom.h" // MUST precede ScriptPromiseResolver
for compilation to work.
We should fix these include order dependencies.

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:87:
m_promiseResolver->fulfill(WebCore::Key::create(key));
WebCore::Key -> Key

We're in the WebCore namespace here.

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:102: ASSERT_NOT_REACHED();
Rather than handling this unreachable condition, why not RELEASE_ASSERT?

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:109: delete this;
:(

I don't understand why we keep needing this |delete this| pattern.  Why not just
have the Chromium side own the object and delete it when its done?

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:221: CryptoOperationResult* result = new
CryptoOperationResult();
This is a naked call to |new|.  We shouldn't have any naked calls to new.  Why
can't this be:

OwnPtr<CryptoOperationResult> result = adoptPtr(new CryptoOperationResult());

Then the object will be deleted automatically when it goes out of scope.  If the
Chromium side needs to take ownership of the object (e.g., because it wants to
complete the operation later), then we can call release() and give the Chromium
side ownership.

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:299: CryptoOperationResult* result = new
CryptoOperationResult();
Another naked |new|.  None of these should be necessary.

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

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.idl:34: // These all return Promises.
If you look at other code that uses Promises, we're currently using a typedef in
the IDL.  That's better documentation than a comment until we fix this issue in
the IDL compiler.

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

https://codereview.chromium.org/23164012/diff/2001/public/platform/WebCrypto....
public/platform/WebCrypto.h:55: virtual ~WebCryptoOperationResult() { }
We can make this destructor public so that the Chromium side can delete the
object when its done with it.

[eroman, 2013-08-16 06:45:58]

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

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:109: delete this;
On 2013/08/16 04:53:07, abarth wrote:
> :(
> 
> I don't understand why we keep needing this |delete this| pattern.  Why not
just
> have the Chromium side own the object and delete it when its done?

This is the same model as callbacks, in which the callback deletes itself after
it has run.

There are only 2 locations where deletion happens now, which I think is an
improvement.

If the ownership is moved over to the Chromium side, then each implementation
which invokes the callback would have to additionally delete the result object
after it is done, which is more error prone.

i.e.
  result->completeWithBuffer(x);
  delete result;

The pattern above could then be wrapped in another object, but just seems like
more complexity.

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:221: CryptoOperationResult* result = new
CryptoOperationResult();
On 2013/08/16 04:53:07, abarth wrote:
> This is a naked call to |new|.  We shouldn't have any naked calls to new.  Why
> can't this be:
> 
> OwnPtr<CryptoOperationResult> result = adoptPtr(new CryptoOperationResult());
> 
> Then the object will be deleted automatically when it goes out of scope.  If
the
> Chromium side needs to take ownership of the object (e.g., because it wants to
> complete the operation later), then we can call release() and give the
Chromium
> side ownership.

This was to enable the implementations of encrypt(), decrypt(), sign(), etc to
finish EITHER synchronously or asynchronously.

In the current model,  the pointer is passed to encrypt(), decrypt(), etc.  If
those operations complete synchronously, then the result can be destroyed upon
return. Otherwise the caller has taken ownership of the result until it
completes.

I didn't find the OwnPtr<> pattern to express this better, but I'll think about
it more tomorrow.

[abarth-chromium, 2013-08-16 07:22:39]

On 2013/08/16 06:45:58, eroman wrote:
>
https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
> File Source/modules/crypto/SubtleCrypto.cpp (right):
> 
>
https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
> Source/modules/crypto/SubtleCrypto.cpp:109: delete this;
> On 2013/08/16 04:53:07, abarth wrote:
> > :(
> > 
> > I don't understand why we keep needing this |delete this| pattern.  Why not
> just
> > have the Chromium side own the object and delete it when its done?
> 
> This is the same model as callbacks, in which the callback deletes itself
after
> it has run.

I'm not sure what callbacks you're talking about.  Is that a Blink concept?  We
generally naked calls to |new| and explicit calls to |delete|.  They're too hard
to get right.  I feel like you're fighting against this programing practice. 
We've used this pattern extensively across a very large code base and it's
worked out fairly well so far.  We still get ourselves into trouble even in this
restricted subset of the language, but we appear to get ourselves into less
trouble than we would if we had manual calls to |delete|.

> There are only 2 locations where deletion happens now, which I think is an
> improvement.

This CL is definitely an improvement, but I'm asking you to adopt the programing
patterns we use throughout the rest of the Blink codebase.

> If the ownership is moved over to the Chromium side, then each implementation
> which invokes the callback would have to additionally delete the result object
> after it is done, which is more error prone.

I disagree.  The Chromium side should use scoped_ptr to make that automatic. 
For a while we experimented with returning a wrapper type instead of a raw
pointer that forced the caller to move the pointer into a scoped_ptr before
being able to access it.  We didn't manage to deploy that very much in the API,
however.

> i.e.
>   result->completeWithBuffer(x);
>   delete result;

That's not the pattern I would recommend for the caller.  Instead, I would
recommend something along these lines:

scoped_ptr<WebFoo> result = bar->createFoo();
result->completeWithBuffer(x);

Now the scoped_ptr will ensure that |result| is deleted.

> The pattern above could then be wrapped in another object, but just seems like
> more complexity.

Look, we've had this discussion repeatedly.  If you want to change the memory
management idioms we use in Blink, you're welcome to do that, but this code
review isn't the place to do that.  Instead, I'd recommend you raise the issue
on blink-dev.  However, I would ask you to gain more experience with the idioms
we use before dismissing them.

>
https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
> Source/modules/crypto/SubtleCrypto.cpp:221: CryptoOperationResult* result =
new
> CryptoOperationResult();
> On 2013/08/16 04:53:07, abarth wrote:
> > This is a naked call to |new|.  We shouldn't have any naked calls to new. 
Why
> > can't this be:
> > 
> > OwnPtr<CryptoOperationResult> result = adoptPtr(new
CryptoOperationResult());
> > 
> > Then the object will be deleted automatically when it goes out of scope.  If
> the
> > Chromium side needs to take ownership of the object (e.g., because it wants
to
> > complete the operation later), then we can call release() and give the
> Chromium
> > side ownership.
> 
> This was to enable the implementations of encrypt(), decrypt(), sign(), etc to
> finish EITHER synchronously or asynchronously.

Whether they finish synchronously or asynchronously isn't relevant.  If the
operation needs to finish asynchronously, we can release() the pointer to
Chromium.  Somehow we've managed to implement a very large codebase without
manual calls to |delete|.  I don't think there's anything particularly unique
about these crypto operations that makes OwnPtr unsuitable.

> In the current model,  the pointer is passed to encrypt(), decrypt(), etc.  If
> those operations complete synchronously, then the result can be destroyed upon
> return. Otherwise the caller has taken ownership of the result until it
> completes.

I understand how the current model works.  I'm telling you that it is
inconsistent with the memory management idioms we use in the remainder of the
Blink codebase.

> I didn't find the OwnPtr<> pattern to express this better, but I'll think
about
> it more tomorrow.

I suspect that's because you're not familiar with OwnPtr.  Like any idiom, it
rewards repeated use.  I would ask you to try using the idiom for a while before
forming a judgement about it.

[abarth-chromium, 2013-08-16 07:43:37]

Another way to frame my feedback is that the ownership model in this CL is too
complicated.  We've decided as a project that we're not smart enough engineers
to handle ownership models that are as complex as this one.  Instead, we require
that you explain the ownership model to the C++ type system when you write your
CL.  We have a couple different types to help with that.  The one that's
appropriate in this case is OwnPtr.  If you can explain the ownership model to
OwnPtr, then the C++ type system can take responsibility for understanding it,
and the folks who maintain the code can worry about other issues.

It's unfortunate that we lose the ownership typing information when we cross the
API boundary.  That's something we should seek to improve by having a type in
the API that represents an ownership transfer across the API boundary (e.g.,
some sort of "Web"PassOwnPtr).  I don't think we need to solve that problem in
this CL, however.  Instead, we just need to follow Blink's style guide and wrap
every call to |new| either in |adoptRef| or |adoptPtr|.  The type system will
take care of the rest.

[eroman, 2013-08-16 16:23:24]

Thanks Adam!

I'm definitely trying to match the blink model and appreciate you spending this
time. Sorry for sucking, I will try to get it right on the next patchset.

[eroman, 2013-08-16 22:59:41]

https://codereview.chromium.org/23164012/diff/2001/Source/core/platform/chrom...
File Source/core/platform/chromium/support/WebCrypto.cpp (right):

https://codereview.chromium.org/23164012/diff/2001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCrypto.cpp:31: #include <string.h> //
for memcpy()
On 2013/08/16 04:53:07, abarth wrote:
> Please alphabetize this include with the rest of the includes.  Also, there's
no
> need for the comment.  It's going to get out of date very fast.

Done.

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

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.cpp:36: #include
"bindings/v8/custom/V8ArrayBufferCustom.h" // MUST precede ScriptPromiseResolver
for compilation to work.
On 2013/08/16 04:53:07, abarth wrote:
> We should fix these include order dependencies.

The issue is that the header file inlines this templetized function:

template<typename T>
void ScriptPromiseResolver::fulfill(PassRefPtr<T> value)
{
    ASSERT(v8::Context::InContext());
    fulfill(toV8(value.get(), v8::Object::New(), m_isolate));
}

Depending what type T you call it with, it needs to be able to resolve the
appropriate toV8(T*) overload. Since there is no global header which declares
all the toV8() functions this causes a header ordering dependency.

Another solution would be to remove the function and have callers do the toV8()
part themselves.

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

https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.idl:34: // These all return Promises.
On 2013/08/16 04:53:07, abarth wrote:
> If you look at other code that uses Promises, we're currently using a typedef
in
> the IDL.  That's better documentation than a comment until we fix this issue
in
> the IDL compiler.

Done.

Note that I couldn't find other IDL files which do that, just a bug reference to
266700.

[abarth-chromium, 2013-08-19 19:11:39]

On 2013/08/16 22:59:41, eroman wrote:
> Note that I couldn't find other IDL files which do that, just a bug reference
to
> 266700.

Yeah, it hadn't landed yet.

[abarth-chromium, 2013-08-19 19:13:41]

lgtm

[commit-bot: I haz the power, 2013-08-19 19:13:52]

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

[commit-bot: I haz the power, 2013-08-19 19:14:18]

Failed to apply patch for Source/modules/crypto/SubtleCrypto.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/crypto/SubtleCrypto.idl
  Hunk #1 FAILED at 28.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/modules/crypto/SubtleCrypto.idl.rej

Patch:       Source/modules/crypto/SubtleCrypto.idl
Index: Source/modules/crypto/SubtleCrypto.idl
diff --git a/Source/modules/crypto/SubtleCrypto.idl
b/Source/modules/crypto/SubtleCrypto.idl
index
b283fe25c2b1e596f4576c6c00769bf455fdeeb0..11eb26b0027ce57968522b2b7068d5e2ee2f856b
100644
--- a/Source/modules/crypto/SubtleCrypto.idl
+++ b/Source/modules/crypto/SubtleCrypto.idl
@@ -28,16 +28,19 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+// FIXME: http://crbug.com/266700
+typedef any Promise;
+
 [
     NoInterfaceObject,
 ] interface SubtleCrypto {
-    [RaisesException] CryptoOperation encrypt(Dictionary algorithm, Key key);
-    [RaisesException] CryptoOperation decrypt(Dictionary algorithm, Key key);
-    [RaisesException] CryptoOperation sign(Dictionary algorithm, Key key);
-    [RaisesException, ImplementedAs=verifySignature] CryptoOperation
verify(Dictionary algorithm, Key key, ArrayBufferView signature);
-    [RaisesException] CryptoOperation digest(Dictionary algorithm);
+    [RaisesException] Promise encrypt(Dictionary algorithm, Key key,
ArrayBufferView data);
+    [RaisesException] Promise decrypt(Dictionary algorithm, Key key,
ArrayBufferView data);
+    [RaisesException] Promise sign(Dictionary algorithm, Key key,
ArrayBufferView data);
+    [RaisesException, ImplementedAs=verifySignature] Promise verify(Dictionary
algorithm, Key key, ArrayBufferView signature, ArrayBufferView data);
+    [RaisesException] Promise digest(Dictionary algorithm, ArrayBufferView
data);
 
-    [RaisesException] any generateKey(Dictionary algorithm, boolean
extractable, DOMString[] keyUsages);
-    [RaisesException] any importKey(DOMString format, ArrayBufferView keyData,
Dictionary algorithm, boolean extractable, DOMString[] keyUsages);
+    [RaisesException] Promise generateKey(Dictionary algorithm, boolean
extractable, DOMString[] keyUsages);
+    [RaisesException] Promise importKey(DOMString format, ArrayBufferView
keyData, Dictionary algorithm, boolean extractable, DOMString[] keyUsages);
 };

[commit-bot: I haz the power, 2013-08-19 19:43:44]

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

[commit-bot: I haz the power, 2013-08-19 20:44:53]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-08-19 21:31:09]

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

[commit-bot: I haz the power, 2013-08-19 21:49:02]

Change committed as 156338