Support for the new WebCrypto digest by chunks API.

Support for the new WebCrypto digest by chunks API.

This provides the implementation for the WebCrypto sychronous digest by chunk
APIs. This is in addition to the current one-time digest functions.

BUG=349514
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260756

[jww, 2014-03-14 23:01:00]

This is the Chromium-side counterpart to
https://codereview.chromium.org/195983024/.

[eroman, 2014-03-14 23:34:04]

Will hammer out the blink side (API) before reviewing the implementation
details. Cheers

[jww, 2014-03-25 21:46:06]

On 2014/03/14 23:34:04, eroman wrote:
> Will hammer out the blink side (API) before reviewing the implementation
> details. Cheers

Waiting for https://codereview.chromium.org/195983024/ to land, but otherwise
this patch is up to date and ready for review, whenever you have a moment.

[eroman, 2014-03-25 23:26:23]

This is looking pretty good! Thanks for the new tests. Just a few comments, then
I'll give it another review pass before signing off.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto.h:94: blink::WebCryptoDigestor*
CreateDigestor(
* Please add a precondition comment saying that algorithm_id is for a hash.
* Can you change the signature to be so either it returns a scoped_ptr<>, or it
takes a scoped_ptr<>* as input?

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:660:
DigestorNSS(blink::WebCryptoAlgorithmId algorithm_id)
Mark as explicit

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:661: : hash_context_(0),
algorithm_id_(algorithm_id) {}
use NULL rather than 0.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:671: virtual bool consume(const
unsigned char* data, unsigned int size) OVERRIDE {
In general we omit OVERRIDE for objects  coming from blink. Otherwise it is
harder to change APIs without 3-sided patches.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:689: unsigned int&
result_data_size) OVERRIDE {
ditto here

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:697: Status
finishWithWebArrayAndStatus(blink::WebArrayBuffer& result) {
FinishWithWebArrayAndStatus. Also make result be a pointer.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:713: Status init() {
Init

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:728: Status
finishInternal(unsigned char* result, unsigned int* result_size) {
FinishInternal

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_openssl.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:157:
DigestorOpenSSL(blink::WebCryptoAlgorithmId algorithm_id)
explicit

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:162: virtual bool
consume(const unsigned char* data, unsigned int size) OVERRIDE {
ditto on OVERRIDE. I expect this will be pretty stable so shouldn't be an issue,
but just in case.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:166: Status
consumeWithStatus(const unsigned char* data, unsigned int size) {
For ConsumeWithStatus (should use chromium style for non-virtual methods)

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:187: virtual Status
finishWithWebArrayAndStatus(blink::WebArrayBuffer& result) {
Make this non-virtual. Also use chromium-style function name. Also take pointer
to WebArrayBuffer rather than reference.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:205: DigestorOpenSSL() {}
Is this necessary?

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:530: return 0;
return NULL in chromium land

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.h (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.h:91: blink::WebCryptoAlgorithm
algorithm);
I thought on the blink side we were taking AlgorithmId for the hash rather than
Algorithm (in which case this should mirror that).

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto_unittest.cc:670: static const size_t
CHUNK_SIZE = 129;
kChunkSizeBytes

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_impl.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_impl.cc:255:
blink::WebCryptoAlgorithm::adoptParamsAndCreate(algorithm_id, NULL);
Let's get rid of this, and have the shared_crypto version take an ID as well.

[jww, 2014-03-26 00:42:31]

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto.h:94: blink::WebCryptoDigestor*
CreateDigestor(
On 2014/03/25 23:26:23, eroman wrote:
> * Please add a precondition comment saying that algorithm_id is for a hash.
> * Can you change the signature to be so either it returns a scoped_ptr<>, or
it
> takes a scoped_ptr<>* as input?

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:661: : hash_context_(0),
algorithm_id_(algorithm_id) {}
On 2014/03/25 23:26:23, eroman wrote:
> use NULL rather than 0.

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:671: virtual bool consume(const
unsigned char* data, unsigned int size) OVERRIDE {
On 2014/03/25 23:26:23, eroman wrote:
> In general we omit OVERRIDE for objects  coming from blink. Otherwise it is
> harder to change APIs without 3-sided patches.

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:689: unsigned int&
result_data_size) OVERRIDE {
On 2014/03/25 23:26:23, eroman wrote:
> ditto here

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:697: Status
finishWithWebArrayAndStatus(blink::WebArrayBuffer& result) {
On 2014/03/25 23:26:23, eroman wrote:
> FinishWithWebArrayAndStatus. Also make result be a pointer.

This is meant to be an explicit method to get a WebArrayBuffer result because
the other digest function requires a WebArrayBuffer result. Do you think we
should return an array here and move the WebArrayBuffer creation to those other
methods?

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:713: Status init() {
On 2014/03/25 23:26:23, eroman wrote:
> Init

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:728: Status
finishInternal(unsigned char* result, unsigned int* result_size) {
On 2014/03/25 23:26:23, eroman wrote:
> FinishInternal

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_openssl.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:157:
DigestorOpenSSL(blink::WebCryptoAlgorithmId algorithm_id)
On 2014/03/25 23:26:23, eroman wrote:
> explicit

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:162: virtual bool
consume(const unsigned char* data, unsigned int size) OVERRIDE {
On 2014/03/25 23:26:23, eroman wrote:
> ditto on OVERRIDE. I expect this will be pretty stable so shouldn't be an
issue,
> but just in case.

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:166: Status
consumeWithStatus(const unsigned char* data, unsigned int size) {
On 2014/03/25 23:26:23, eroman wrote:
> For ConsumeWithStatus (should use chromium style for non-virtual methods)

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:187: virtual Status
finishWithWebArrayAndStatus(blink::WebArrayBuffer& result) {
On 2014/03/25 23:26:23, eroman wrote:
> Make this non-virtual. Also use chromium-style function name. Also take
pointer
> to WebArrayBuffer rather than reference.

See my question in platform_crypto_nss.cc about the result arg.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_openssl.cc:205: DigestorOpenSSL() {}
On 2014/03/25 23:26:23, eroman wrote:
> Is this necessary?
Removed.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:530: return 0;
On 2014/03/25 23:26:23, eroman wrote:
> return NULL in chromium land

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.h (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.h:91: blink::WebCryptoAlgorithm
algorithm);
On 2014/03/25 23:26:23, eroman wrote:
> I thought on the blink side we were taking AlgorithmId for the hash rather
than
> Algorithm (in which case this should mirror that).

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto_unittest.cc:670: static const size_t
CHUNK_SIZE = 129;
On 2014/03/25 23:26:23, eroman wrote:
> kChunkSizeBytes

Done.

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_impl.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_impl.cc:255:
blink::WebCryptoAlgorithm::adoptParamsAndCreate(algorithm_id, NULL);
On 2014/03/25 23:26:23, eroman wrote:
> Let's get rid of this, and have the shared_crypto version take an ID as well.

Done.

[eroman, 2014-03-26 00:49:08]

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:697: Status
finishWithWebArrayAndStatus(blink::WebArrayBuffer& result) {
On 2014/03/26 00:42:31, jww wrote:
> On 2014/03/25 23:26:23, eroman wrote:
> > FinishWithWebArrayAndStatus. Also make result be a pointer.
> 
> This is meant to be an explicit method to get a WebArrayBuffer result because
> the other digest function requires a WebArrayBuffer result. Do you think we
> should return an array here and move the WebArrayBuffer creation to those
other
> methods?

What i meant, is in chromium-style an out parameter should not be a non-const
reference. Instead it should be a pointer. So I just want you to change

blink::WebArrayBuffer& result
to
blink::WebArrayBuffer* result

[eroman, 2014-03-26 00:50:12]

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:697: Status
finishWithWebArrayAndStatus(blink::WebArrayBuffer& result) {
On 2014/03/26 00:49:08, eroman wrote:
> On 2014/03/26 00:42:31, jww wrote:
> > On 2014/03/25 23:26:23, eroman wrote:
> > > FinishWithWebArrayAndStatus. Also make result be a pointer.
> > 
> > This is meant to be an explicit method to get a WebArrayBuffer result
because
> > the other digest function requires a WebArrayBuffer result. Do you think we
> > should return an array here and move the WebArrayBuffer creation to those
> other
> > methods?
> 
> What i meant, is in chromium-style an out parameter should not be a non-const
> reference. Instead it should be a pointer. So I just want you to change
> 
> blink::WebArrayBuffer& result
> to
> blink::WebArrayBuffer* result


And then instead of
result = ...
you do
*result = ...

Nothing much else changes

[jww, 2014-03-26 00:57:24]

On 2014/03/26 00:50:12, eroman wrote:
>
https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
> File content/child/webcrypto/platform_crypto_nss.cc (right):
> 
>
https://codereview.chromium.org/200763005/diff/80001/content/child/webcrypto/...
> content/child/webcrypto/platform_crypto_nss.cc:697: Status
> finishWithWebArrayAndStatus(blink::WebArrayBuffer& result) {
> On 2014/03/26 00:49:08, eroman wrote:
> > On 2014/03/26 00:42:31, jww wrote:
> > > On 2014/03/25 23:26:23, eroman wrote:
> > > > FinishWithWebArrayAndStatus. Also make result be a pointer.
> > > 
> > > This is meant to be an explicit method to get a WebArrayBuffer result
> because
> > > the other digest function requires a WebArrayBuffer result. Do you think
we
> > > should return an array here and move the WebArrayBuffer creation to those
> > other
> > > methods?
> > 
> > What i meant, is in chromium-style an out parameter should not be a
non-const
> > reference. Instead it should be a pointer. So I just want you to change
> > 
> > blink::WebArrayBuffer& result
> > to
> > blink::WebArrayBuffer* result
> 
> 
> And then instead of
> result = ...
> you do
> *result = ...
> 
> Nothing much else changes

Ah-ha, got! Sorry for the misunderstanding. Fixed.

[eroman, 2014-03-26 01:43:54]

patchset 7 LGTM after these comments

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:663: virtual ~DigestorNSS()
OVERRIDE {
no need for override here, suggest removing (in principle i like override, just
a nuisance for blink<-->chromium)

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:709: HASHContext* hash_context_;
[style] members go after functions. Move all the members to the very end of the
private block.

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
File content/child/webcrypto/platform_crypto_openssl.cc (right):

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_openssl.cc:200: bool initialized_;
same comment: the member variables should be at the very end.

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
File content/child/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:678: std::vector<uint8>
chunk(begin, begin + chunk_length);
Not necessary to copy into a std::vector<>.

However I think it is actually a good idea to, because it is more likely to
catch illegal memory accesses this way.

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
File content/child/webcrypto/webcrypto_impl.h (right):

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.h:93: const blink::WebCryptoAlgorithmId
algorithm_id);
IMPORTANT: In the blink interface the input is non-const. This probably needs to
similarly be non-const or maybe it will create an overload rather than
overriding. Worth testing either way.

[jww, 2014-03-26 18:49:40]

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:663: virtual ~DigestorNSS()
OVERRIDE {
On 2014/03/26 01:43:54, eroman wrote:
> no need for override here, suggest removing (in principle i like override,
just
> a nuisance for blink<-->chromium)

Done.

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:709: HASHContext* hash_context_;
On 2014/03/26 01:43:54, eroman wrote:
> [style] members go after functions. Move all the members to the very end of
the
> private block.

Done.

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
File content/child/webcrypto/platform_crypto_openssl.cc (right):

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_openssl.cc:200: bool initialized_;
On 2014/03/26 01:43:54, eroman wrote:
> same comment: the member variables should be at the very end.

Done.

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
File content/child/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:678: std::vector<uint8>
chunk(begin, begin + chunk_length);
On 2014/03/26 01:43:54, eroman wrote:
> Not necessary to copy into a std::vector<>.
> 
> However I think it is actually a good idea to, because it is more likely to
> catch illegal memory accesses this way.

Done.

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
File content/child/webcrypto/webcrypto_impl.h (right):

https://codereview.chromium.org/200763005/diff/240001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.h:93: const blink::WebCryptoAlgorithmId
algorithm_id);
On 2014/03/26 01:43:54, eroman wrote:
> IMPORTANT: In the blink interface the input is non-const. This probably needs
to
> similarly be non-const or maybe it will create an overload rather than
> overriding. Worth testing either way.

Good catch! I ran some tests, and it seems like it overrode it (rather than
overloaded it) anyway, but obviously should match anyway.

[jww, 2014-03-26 18:49:51]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:52:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jww@chromium.org/200763005/320001

[commit-bot: I haz the power, 2014-03-26 19:24:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 19:24:11]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[jww, 2014-03-27 18:13:48]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-03-27 18:14:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jww@chromium.org/200763005/320001

[commit-bot: I haz the power, 2014-03-27 18:36:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 18:36:58]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[eroman, 2014-03-27 19:27:47]

Failure is because you need to remove the const from createDigestor()

[jww, 2014-03-31 19:51:57]

On 2014/03/27 19:27:47, eroman wrote:
> Failure is because you need to remove the const from createDigestor()

Whoops! Good call. Thanks.

[jww, 2014-03-31 19:52:02]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-03-31 19:54:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jww@chromium.org/200763005/340001

[commit-bot: I haz the power, 2014-03-31 21:12:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 21:12:42]

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

[jww, 2014-03-31 23:32:24]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-03-31 23:32:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jww@chromium.org/200763005/360001

[commit-bot: I haz the power, 2014-04-01 02:31:35]

Change committed as 260756