WebCrypto: Add SHA-1 support to crypto.subtle.digest().

WebCrypto: Add SHA-1 support to crypto.subtle.digest().

This change sets up more WebKit API interfaces for embedders to implement CryptoOperation, and connects them together using SHA-1 as an end-to-end test.

The SHA-1 support is implemented on the blink side as a proof of concept, since it is just a few lines of extra code. However this will be moved over to the chromium side once more plumbing is in place.

Not that this change does not make CryptoOperation inherit from Promise. Instead it directly returns a Promise from finish() and abort(). This was done to avoid potential cycles and memory leaks. Will follow up with the working group to resolve the spec's behavior.

BUG=245025
R=abarth@chromium.org

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

[eroman, 2013-07-12 02:58:43]

https://codereview.chromium.org/19082002/diff/11/Source/modules/crypto/Crypto...
File Source/modules/crypto/CryptoOperation.cpp (right):

https://codereview.chromium.org/19082002/diff/11/Source/modules/crypto/Crypto...
Source/modules/crypto/CryptoOperation.cpp:34: #include
"bindings/v8/custom/V8ArrayBufferCustom.h" // MUST precede ScriptPromiseResolver
for compilation to work.
Not sure whether this is working as intended or not, however I found it is
necessary to include V8ArrayBufferCustom.h before ScriptPromiseResolver.h.

The reason being the inlined ScriptPromiseResolver::resolve<T>() calls toV8(T*).
When T is ArrayBuffer, the necessary toV8() function is declared in
V8ArrayBufferCustom.h.

[abarth-chromium, 2013-07-12 03:53:23]

https://codereview.chromium.org/19082002/diff/11/Source/modules/crypto/Crypto...
File Source/modules/crypto/CryptoOperation.cpp (right):

https://codereview.chromium.org/19082002/diff/11/Source/modules/crypto/Crypto...
Source/modules/crypto/CryptoOperation.cpp:34: #include
"bindings/v8/custom/V8ArrayBufferCustom.h" // MUST precede ScriptPromiseResolver
for compilation to work.
On 2013/07/12 02:58:43, eroman wrote:
> Not sure whether this is working as intended or not, however I found it is
> necessary to include V8ArrayBufferCustom.h before ScriptPromiseResolver.h.
> 
> The reason being the inlined ScriptPromiseResolver::resolve<T>() calls
toV8(T*).
> When T is ArrayBuffer, the necessary toV8() function is declared in
> V8ArrayBufferCustom.h.

Interesting.  Welcome to the bleeding edge...

https://codereview.chromium.org/19082002/diff/11/Source/modules/crypto/Crypto...
File Source/modules/crypto/CryptoOperation.h (right):

https://codereview.chromium.org/19082002/diff/11/Source/modules/crypto/Crypto...
Source/modules/crypto/CryptoOperation.h:91: ScriptObject m_promise;
This isn't good.  You can't hold a ScriptObject as a member of an object that
might be retained by JavaScript.  That creates a cycle and will cause a memory
leak...

[eroman, 2013-07-12 04:24:37]

https://codereview.chromium.org/19082002/diff/11/Source/modules/crypto/Crypto...
File Source/modules/crypto/CryptoOperation.h (right):

https://codereview.chromium.org/19082002/diff/11/Source/modules/crypto/Crypto...
Source/modules/crypto/CryptoOperation.h:91: ScriptObject m_promise;
On 2013/07/12 03:53:24, abarth wrote:
> This isn't good.  You can't hold a ScriptObject as a member of an object that
> might be retained by JavaScript.  That creates a cycle and will cause a memory
> leak...

I have changed it so the accessor returns a copy of ScriptObject instead, does
that avoid the problem?

I don't quite understand how ScriptValue/ScriptObject work yet; I'll try to
better educate myself tomorrow so your comment makes sense.

[abarth-chromium, 2013-07-12 04:34:37]

On 2013/07/12 04:24:37, eroman wrote:
> I have changed it so the accessor returns a copy of ScriptObject instead, does
> that avoid the problem?

I'll have to look more closely tomorrow to be able to answer this question.

> I don't quite understand how ScriptValue/ScriptObject work yet; I'll try to
> better educate myself tomorrow so your comment makes sense.

ScriptValue (ScriptObject is the same in this regard) is a value type that hold
a RefPtr to an "impl" object that holds a strong reference to a V8 object.  That
means you can freely copy ScriptValue around similar to how you can freely copy
RefPtr or scoped_refptr around.  The trouble comes when you try to mix the
RefCounted C++ memory model with V8's garbage collected memory model.

The general approach is that V8 hold strong references into C++ and C++ holds
weak references back to V8.  ScriptValue "breaks" this model because it's a
strong reference from C++ into V8.  The V8 garbage collector isn't able to trace
through C++ references, so if there's a strong reference cycle involving C++,
neither memory model is able to reclaim the memory, and we have a leak. 
ScriptValue is meant to be passed around by C++ during a given call stack but
not retained in the heap.

ScriptPromiseResolver is similar in that it also represents a strong reference
from C++ into V8.  The difference is that ScriptPromiseResolver has a lifecycle.
 Once you've resolved the promise, the object drops its references into V8,
breaking the cycle.

[abarth-chromium, 2013-07-12 16:58:30]

https://codereview.chromium.org/19082002/diff/16001/Source/modules/crypto/Cry...
File Source/modules/crypto/CryptoOperation.cpp (right):

https://codereview.chromium.org/19082002/diff/16001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:38: #include
"public/web/WebArrayBuffer.h"
This is a backwards dependency.  You can fix this by moving
public/web/WebArrayBuffer.h to public/platform/WebArrayBuffer.h and
Source/web/WebArrayBuffer.cpp to
Source/core/platform/chromium/support/WebArrayBuffer.cpp.

https://codereview.chromium.org/19082002/diff/16001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:55: }
Should this clear m_result when done too?

https://codereview.chromium.org/19082002/diff/16001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:125:
m_promiseResolver->reject(ScriptValue(v8::Null()));
We should give ScriptValue a factor method for making null so you don't need to
use the V8 API directly (e.g., ScriptValue::createNull()   ).

https://codereview.chromium.org/19082002/diff/16001/public/platform/Platform.h
File public/platform/Platform.h (left):

https://codereview.chromium.org/19082002/diff/16001/public/platform/Platform....
public/platform/Platform.h:545: 
We're actually supposed to have two blank lines between each of these sections.

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

https://codereview.chromium.org/19082002/diff/16001/public/platform/WebCrypto...
public/platform/WebCrypto.h:38: class WebArrayBuffer;
Technically we need to move WebArrayBuffer from public/web to public/platform in
order for this dependency to work.  You've forward declared this class, but you
won't be able to #include it.

[abarth-chromium, 2013-07-12 17:41:42]

I think there's a larger memory leak problem here.  Suppose the web page calls
then(func) on the promse but never calls finish() or abort().  Now we'll leak
memory because the CryptoOperation will retain the PromiseResolve, which will
retain the Promise, which will retain func, which could retain the
CryptoOperation.

The only way to avoid this memory leak would be to both recognize this cycle and
determine that nothing else could cause the promise to resolve.

Instead, I'd recommend changing the API to only vend a Promise object once we
know that the promise will eventually resolve one way or another.  This "no
unbounded promise" rule should resolve the memory leak.  Concretely, that means
finish() and abort() should return a Promise rather than a CryptoOperation and
CryptoOperation should not inherit from Promise.

[eroman, 2013-07-12 20:38:18]

Thanks for the reviews!

Good point about the circular reference, I am raising this issue with the WG.
For now I have done as you recommended, and made finish() and abort() return a
Promise

https://codereview.chromium.org/19082002/diff/16001/Source/modules/crypto/Cry...
File Source/modules/crypto/CryptoOperation.cpp (right):

https://codereview.chromium.org/19082002/diff/16001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:38: #include
"public/web/WebArrayBuffer.h"
On 2013/07/12 16:58:31, abarth wrote:
> This is a backwards dependency.  You can fix this by moving
> public/web/WebArrayBuffer.h to public/platform/WebArrayBuffer.h and
> Source/web/WebArrayBuffer.cpp to
> Source/core/platform/chromium/support/WebArrayBuffer.cpp.

Done. I also added a temporary forwarding header from web/WebArrayBuffer.h -->
platform/WebArrayBuffer.h, since there is code that depends on this in chromium.

https://codereview.chromium.org/19082002/diff/16001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:55: }
On 2013/07/12 16:58:31, abarth wrote:
> Should this clear m_result when done too?

I would prefer not to clear m_result, for debugging purposes.

<optional_justification>
One bug which an embedder might do is to set the result more than once (right
now there is just one result type, ArrayBuffer; however in the future there will
be more type-specific methods).

I would like to ASSERT that one and only one method is called. If I delete
m_result right here then such errors will be use-after-frees, rather than
hitting my ASSERT() on m_state.

Perhaps this is weak reasoning, and certainly tools like valgrind should catch
such mis-uses. So I am happy to change the code.
</optional_justification>

https://codereview.chromium.org/19082002/diff/16001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:125:
m_promiseResolver->reject(ScriptValue(v8::Null()));
On 2013/07/12 16:58:31, abarth wrote:
> We should give ScriptValue a factor method for making null so you don't need
to
> use the V8 API directly (e.g., ScriptValue::createNull()   ).

Done.

https://codereview.chromium.org/19082002/diff/16001/public/platform/Platform.h
File public/platform/Platform.h (left):

https://codereview.chromium.org/19082002/diff/16001/public/platform/Platform....
public/platform/Platform.h:545: 
On 2013/07/12 16:58:31, abarth wrote:
> We're actually supposed to have two blank lines between each of these
sections.

Done. I updated the other inconsistencies in this file.

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

https://codereview.chromium.org/19082002/diff/16001/public/platform/WebCrypto...
public/platform/WebCrypto.h:38: class WebArrayBuffer;
On 2013/07/12 16:58:31, abarth wrote:
> Technically we need to move WebArrayBuffer from public/web to public/platform
in
> order for this dependency to work.  You've forward declared this class, but
you
> won't be able to #include it.

Done.

Note that I see other files in chromium which #include from public/web. Should
those be changed?

  content/renderer/pepper/pepper_websocket_host.cc
  content/renderer/skia_benchmarking_extension.cc
  content/renderer/v8_value_converter_impl.cc
  webkit/plugins/ppapi/host_array_buffer_var.h
  webkit/plugins/ppapi/v8_var_converter.cc

[jamesr, 2013-07-12 20:39:57]

On 2013/07/12 20:38:18, eroman wrote:
>
https://codereview.chromium.org/19082002/diff/16001/public/platform/WebCrypto...
> public/platform/WebCrypto.h:38: class WebArrayBuffer;
> On 2013/07/12 16:58:31, abarth wrote:
> > Technically we need to move WebArrayBuffer from public/web to
public/platform
> in
> > order for this dependency to work.  You've forward declared this class, but
> you
> > won't be able to #include it.
> 
> Done.
> 
> Note that I see other files in chromium which #include from public/web. Should
> those be changed?
> 
>   content/renderer/pepper/pepper_websocket_host.cc
>   content/renderer/skia_benchmarking_extension.cc
>   content/renderer/v8_value_converter_impl.cc
>   webkit/plugins/ppapi/host_array_buffer_var.h
>   webkit/plugins/ppapi/v8_var_converter.cc

Chromium code can #include from public/web.  The restriction is that
public/platform code can't depend on public/web.  The layer cake goes

Chromium
public/web
(blink guts)
public/platform
Chromium (again)

[Ryan Sleevi, 2013-07-13 03:44:57]

On 2013/07/12 17:41:42, abarth wrote:
> I think there's a larger memory leak problem here.  Suppose the web page calls
> then(func) on the promse but never calls finish() or abort().  Now we'll leak
> memory because the CryptoOperation will retain the PromiseResolve, which will
> retain the Promise, which will retain func, which could retain the
> CryptoOperation.
> 
> The only way to avoid this memory leak would be to both recognize this cycle
and
> determine that nothing else could cause the promise to resolve.
> 
> Instead, I'd recommend changing the API to only vend a Promise object once we
> know that the promise will eventually resolve one way or another.  This "no
> unbounded promise" rule should resolve the memory leak.  Concretely, that
means
> finish() and abort() should return a Promise rather than a CryptoOperation and
> CryptoOperation should not inherit from Promise.

This is the same problem that I raised with slightlyoff@, for exactly the same
reason. He was convinced that "unresolvable until some script action" promises
are not only not an issue, but will be prevalent as other APIs are adopted.

I'm not entirely convinced of this, and the WG is in process of discussing
multi-part operations in general - whether adopting MSFT's Streams API or
something else (jonas has proposed a chunked reader operation).

[eroman, 2013-07-16 00:11:32]

@abarth: Anything else you want me to do in this CL?

[abarth-chromium, 2013-07-16 00:29:10]

LGTM.  Thanks!

https://codereview.chromium.org/19082002/diff/45001/Source/modules/crypto/Cry...
File Source/modules/crypto/CryptoOperation.cpp (right):

https://codereview.chromium.org/19082002/diff/45001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:81: m_promiseResolver =
ScriptPromiseResolver::create();
Can we delay creating the m_promiseResolver until we're ready to vend the
promise?  As written, there's a risk that m_promiseResolver could retain
something and cause a leak.

https://codereview.chromium.org/19082002/diff/45001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:113: return
m_promiseResolver->promise();
e.g., we could create it right here.

https://codereview.chromium.org/19082002/diff/45001/Source/modules/crypto/Cry...
File Source/modules/crypto/CryptoOperation.idl (right):

https://codereview.chromium.org/19082002/diff/45001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.idl:40: any abort();
It's too bad we can't use the type Promise here, but I think you're right that
we can't.

https://codereview.chromium.org/19082002/diff/45001/public/README
File public/README (right):

https://codereview.chromium.org/19082002/diff/45001/public/README#newcode42
public/README:42: Note that public/platform should not depend on public/web.
Thanks for adding this!

[eroman, 2013-07-16 07:28:06]

https://codereview.chromium.org/19082002/diff/45001/Source/modules/crypto/Cry...
File Source/modules/crypto/CryptoOperation.cpp (right):

https://codereview.chromium.org/19082002/diff/45001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:81: m_promiseResolver =
ScriptPromiseResolver::create();
On 2013/07/16 00:29:10, abarth wrote:
> Can we delay creating the m_promiseResolver until we're ready to vend the
> promise?  As written, there's a risk that m_promiseResolver could retain
> something and cause a leak.

Done.

https://codereview.chromium.org/19082002/diff/45001/Source/modules/crypto/Cry...
Source/modules/crypto/CryptoOperation.cpp:113: return
m_promiseResolver->promise();
On 2013/07/16 00:29:10, abarth wrote:
> e.g., we could create it right here.

Done.

[commit-bot: I haz the power, 2013-07-16 07:28:55]

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

[commit-bot: I haz the power, 2013-07-16 12:10:09]

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

[eroman, 2013-07-16 17:36:43]

Committed patchset #14 manually as r154310 (presubmit successful).

[eroman, 2013-07-16 23:06:27]

I reverted this changelist due to a crash that happened on buildbot.

I have fixed the crash in patchset 16:

https://codereview.chromium.org/19082002/diff2/79001:85001/Source/modules/cry...

The problem was that during garbage collection, ~CryptoOperation() would call
abort(), which would create a ScriptPromiseResolver.

The solution is to not reject the unnecessary ScriptPromiseResolver during
destruction.

In the process of testing I also found that there was an omission in the layout
tests, which I have corrected in patchset 17 (the first completion proceeded
directly to next test).

Sorry for the noise; PTAL if it still looks good to you.

[abarth-chromium, 2013-07-17 00:30:32]

LGTM

[eroman, 2013-07-17 01:16:38]

Committed patchset #18 manually as r154344 (presubmit successful).