[webcrypto] Make operations run on worker threads.

[webcrypto] Make operations run on a background thread so they don't block the blink thread.

More details on threading strategy provided in webcrypto_impl.cc.

BUG=245025

[Ryan Sleevi, 2014-04-18 00:51:26]

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

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:18: }
Is it legal to template forward declare like this? And I don't mean C++ legal, I
mean Blink style legal.

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:49: //     target blink thread during
structured clone.
s/blink/Blink

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:52: //     called from the target
blink thread during structured clone
s/blink/Blink/

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

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto.cc:838: // surfaced as WebCrypto errors
which isn't quite correct.
Why isn't it quite correct? I tried to make sure the spec could accomodate this.

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

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto.h:8: #include <vector>
style nit: newline

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

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:57: //              
std::vector<> flavor of functions directly.
At the risk of being a PITA, can you rename the functions, rather than overload?
Seeing Encrypt call Encrypt is surprising, and I had to dig through the impl to
see that, ah yes, the function args are different.

EncryptVector / EncryptWebArrayBuffer, even though that will cause churn, I
know.

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

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:36: // takes hundreds of milliseconds.
"hundreds of milliseconds to several seconds" (eg: on low power Chromebooks)

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:43: // blink Web Workers.
s/blink/Blink/

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:65: //   handle() which wraps an
NSS/OpenSSL type may not be, and should only be
s/handle() /handle(), /
s/type may/type, may/

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:69: //   the target blink thread.
s/blink/Blink/

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:72: //               destroyed from
the webcrypto worker pool if the blink worker
s/blink/Blink/

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:93:
base::LazyInstance<CryptoThreadPool> crypto_thread_pool =
LeakyLazyInstance

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:117: else {
use braces on 115 and 117 to match 117

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:136: }
inconsistent braces. Brace ALL the things or brace NONE of the things

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:152: }
This comment and this function do not make sense in and of themselves.

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:224: // If |is_asymetric| is false,
then |public_key| is understood to mean the
asymmetric

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:228: bool is_asymetric;
asymmetric

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:390: CompleteWithError(state->status,
&state->result);
Why do you allow CompleteWithError() here and in DoVerifyReply, but in all other
cases use CompleteWithBuffer/CompleteWithKey directly?

It would likely be better to rename CompleteWithBuffer to something like
CompleteWithBufferOrError / CompleteWithKeyOrError

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
File content/child/webcrypto/webcrypto_impl.h (left):

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.h:85: blink::WebArrayBuffer& result);
I thought this was used by jww's SRI bits? or is that createDigestor?

[eroman, 2014-04-18 18:45:56]

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

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:18: }
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> Is it legal to template forward declare like this? And I don't mean C++ legal,
I
> mean Blink style legal.

I believe so. It is used heavily in Blink proper, and I also found some usages
in the Blink API of WebVector in particular:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:49: //     target blink thread during
structured clone.
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> s/blink/Blink

Done. I did a search through all the files and updated blink --> Blink.

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

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto.cc:838: // surfaced as WebCrypto errors
which isn't quite correct.
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> Why isn't it quite correct? I tried to make sure the spec could accomodate
this.

I wrote that to highlight that the behavior is observable to the user, and not
just an implementation detail.

For example:

  * In the synchronous approach, if the database is corrupted you get an error
immediately when reading out of database. Whereas in the async approach it looks
like you successfully retrieved a value from the database, however it will fail
when using it.

One consequence of this is that you can persist an invalid key when the import
is async: you can deserialize it (success), and then immediately serialize it,
even though it is rubbish.

Whatever pattern is better, they are at the very least *different*.

I have modified the wording to reflect this and not pass judgement on whether it
is "incorrect".

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

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:57: //              
std::vector<> flavor of functions directly.
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> At the risk of being a PITA, can you rename the functions, rather than
overload?
> Seeing Encrypt call Encrypt is surprising, and I had to dig through the impl
to
> see that, ah yes, the function args are different.
> 
> EncryptVector / EncryptWebArrayBuffer, even though that will cause churn, I
> know.

Agreed. My plan is to refactor the tests to call the std::vector<> version
directly before committing. I wanted to reach agreement that switching to
std::vector<> is the right approach before doing that though.

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

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:36: // takes hundreds of milliseconds.
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> "hundreds of milliseconds to several seconds" (eg: on low power Chromebooks)

Done.

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:65: //   handle() which wraps an
NSS/OpenSSL type may not be, and should only be
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> s/handle() /handle(), /
> s/type may/type, may/

Done.

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:93:
base::LazyInstance<CryptoThreadPool> crypto_thread_pool =
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> LeakyLazyInstance

Done.

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:117: else {
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> use braces on 115 and 117 to match 117

Done.

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:136: }
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> inconsistent braces. Brace ALL the things or brace NONE of the things

Done.

I agree.

Not sure how this happened, probably the result of edits.

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:152: }
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> This comment and this function do not make sense in and of themselves.

Changed the comment to hopefully clarify things:

// Gets a task runner for the current thread. The current thread is either:
//
//   * The main Blink thread
//   * A Blink worker thread
//
// A different mechanism is needed for posting to these threads. The main
// thread has an associated message loop and can simply use
// base::ThreadTaskRunnerHandle. Whereas the worker threads are managed by
// blink and need to be indirected through WorkerThreadTaskRunner.

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:224: // If |is_asymetric| is false,
then |public_key| is understood to mean the
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> asymmetric

Done throughout.

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:390: CompleteWithError(state->status,
&state->result);
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> Why do you allow CompleteWithError() here and in DoVerifyReply, but in all
other
> cases use CompleteWithBuffer/CompleteWithKey directly?
> 
> It would likely be better to rename CompleteWithBuffer to something like
> CompleteWithBufferOrError / CompleteWithKeyOrError

Agreed. Done

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
File content/child/webcrypto/webcrypto_impl.h (left):

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.h:85: blink::WebArrayBuffer& result);
On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> I thought this was used by jww's SRI bits? or is that createDigestor?

We have switched all the callsites over to createDigestor() which is more
flexible (allows multi-part digest operations), so this is unused now. I am
deleting the API from the blink side, so figured I wouldn't bother updating it
for this conversion. I can split this off to a separate changelist as well if
you prefer.

[eroman, 2014-04-18 20:02:27]

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
File content/child/webcrypto/webcrypto_impl.h (left):

https://codereview.chromium.org/233733004/diff/120001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.h:85: blink::WebArrayBuffer& result);
On 2014/04/18 18:45:56, eroman wrote:
> On 2014/04/18 00:51:26, Ryan Sleevi wrote:
> > I thought this was used by jww's SRI bits? or is that createDigestor?
> 
> We have switched all the callsites over to createDigestor() which is more
> flexible (allows multi-part digest operations), so this is unused now. I am
> deleting the API from the blink side, so figured I wouldn't bother updating it
> for this conversion. I can split this off to a separate changelist as well if
> you prefer.

Aaaand it is now deleted from Blink.

[Ryan Sleevi, 2014-04-24 02:10:40]

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:4: 
IWYU - why aren't you #including "content/child/webcrypto/jwk.h" first and
foremost?

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:8: #include "base/json/json_reader.h"
style nit: line break between lines 7 and 8

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:308: const base::StringPiece key_str(
IWYU - include base/strings/string_piece.h

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
File content/child/webcrypto/jwk.h (right):

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/jwk.h:26: Status ExportKeyJwk(const blink::WebCryptoKey&
key, std::vector<uint8>* buffer);
IWYU - include <vector>

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

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:9: #include "base/basictypes.h"
nit: newline between 8 and 9

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:55: //     TODO(eroman): Change it so
import happens in worker pool too.
File a bug to track this?

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

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:128: //               to call
into NSS if the serialized form already exists.
File a bug?

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:172: OVERRIDE {
I thought this bug in clang-format was fixed?

From discussion, it was supposed to wrap as

virtual bool ThreadSafeSerializeForClone(
    blink::WebVector<uint8>* key_data) OVERRIDE {

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:203:
key_data->assign(Uint8VectorStart(serialized_key_), serialized_key_.size());
Note that this doesn't save any allocations with vector (or blink::WebVector).
C++98 defines vector::assign in terms of vector::erase and vector::insert(), and
blink::WebVector is implemented similarly. I also don't believe (but am having
trouble finding the citable reference) that it's valid to supply NULL/0 to
insert() - primarily because it guarantees that first will be de-referenced
(ergo must be valid), although last may not. It is safe for blink::WebVector
(which special handles this), but may be worth noting here.

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

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:76: //               stopped then the
potential for races seems limited.
When do you stop the worker thread?

Note that this description might be clarified by using different terms...
"worker thread" could be "thread on the worker pool" or "Blink worker thread",
and it's not clear which you refer to.

WebWorker thread?

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:150: //   * A Blink worker thread
Similar to the previous comments, it might be more helpful to define these as
"Web Worker thread", as the spec at least calls them "Web workers" -
http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#workers

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:155: // blink and need to be
indirected through WorkerThreadTaskRunner.
s/blink/Blink/

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:177: // has vanished (which can happen
for Blink web worker threads).
+1 to this comment ("web worker threads") :)

[eroman, 2014-04-24 20:59:37]

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:4: 
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> IWYU - why aren't you #including "content/child/webcrypto/jwk.h" first and
> foremost?

Done.

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:8: #include "base/json/json_reader.h"
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> style nit: line break between lines 7 and 8

Done.

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:308: const base::StringPiece key_str(
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> IWYU - include base/strings/string_piece.h

Done.

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
File content/child/webcrypto/jwk.h (right):

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/jwk.h:26: Status ExportKeyJwk(const blink::WebCryptoKey&
key, std::vector<uint8>* buffer);
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> IWYU - include <vector>

Done.

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

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:9: #include "base/basictypes.h"
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> nit: newline between 8 and 9

Done.

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:55: //     TODO(eroman): Change it so
import happens in worker pool too.
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> File a bug to track this?

Done.

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

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:128: //               to call
into NSS if the serialized form already exists.
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> File a bug?

Done.

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:172: OVERRIDE {
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> I thought this bug in clang-format was fixed?
> 
> From discussion, it was supposed to wrap as
> 
> virtual bool ThreadSafeSerializeForClone(
>     blink::WebVector<uint8>* key_data) OVERRIDE {

Yep seems to be fixed. I synced and re-ran format and it did that.

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

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:76: //               stopped then the
potential for races seems limited.
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> When do you stop the worker thread?
> 
> Note that this description might be clarified by using different terms...
> "worker thread" could be "thread on the worker pool" or "Blink worker thread",
> and it's not clear which you refer to.
> 
> WebWorker thread?

Right, when the blink WebWorker thread is stopped.

I changed the wording a bit, and added more details in:
  http://crbug.com/366840

I intend to address this (since I think it is likely a real race) but since it
is going to add more complexity my preference is to do it as a follow-up CL to
simplify review and testing.

My thinking is to add a destruction observer on the web worker threads, and
track all the outstanding crypto operations for that thread. When it is
destroyed, I will destroy the state of outstanding operations, so the blink
object is destroyed from proper thread.

https://codereview.chromium.org/233733004/diff/140001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:155: // blink and need to be
indirected through WorkerThreadTaskRunner.
On 2014/04/24 02:10:41, Ryan Sleevi wrote:
> s/blink/Blink/

Done.

[Ryan Sleevi, 2014-04-25 23:19:02]

Having better understood the raciness issue (http://crbug.com/366840), I feel
like we should hold off landing this until a good solution is in place. I'm not
comfortable with the general race and hope it doesn't happen.

https://codereview.chromium.org/233733004/diff/200001/content/child/webcrypto...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/233733004/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:5: #include "jwk.h"
IWYU: Full path here - content/child/webcrypto/jwk.h

[eroman, 2014-04-25 23:43:30]

I am about to land a fix in Blink for that:
https://codereview.chromium.org/253563002/

So I will go ahead and remove the TODO from this changelist, since it will no
longer be applicable.

[eroman, 2014-04-26 00:32:06]

OK the TODO is gone, as the issue I was documenting will be resolved before this
is committed.

[Ryan Sleevi, 2014-04-26 00:37:39]

lgtm

[eroman, 2014-04-28 16:40:42]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-04-28 16:41:02]

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

[commit-bot: I haz the power, 2014-04-28 17:04:12]

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

[commit-bot: I haz the power, 2014-04-28 17:04:12]

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

[eroman, 2014-04-28 18:00:48]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-04-28 18:02:21]

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

[eroman, 2014-04-28 19:27:47]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2014-04-28 20:01:44]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:01:59]

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

[commit-bot: I haz the power, 2014-04-28 20:09:06]

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

[commit-bot: I haz the power, 2014-04-28 20:09:07]

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

[eroman, 2014-04-28 20:12:07]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:12:47]

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

[commit-bot: I haz the power, 2014-04-28 20:17:12]

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

[commit-bot: I haz the power, 2014-04-28 20:17:12]

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

[eroman, 2014-04-28 20:18:43]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:20:44]

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

[commit-bot: I haz the power, 2014-04-28 20:23:06]

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

[commit-bot: I haz the power, 2014-04-28 20:23:06]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg
  tryserver.chromium on linux_chromium_rel

[eroman, 2014-04-28 23:11:56]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:12:34]

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

[commit-bot: I haz the power, 2014-04-28 23:58:16]

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

[commit-bot: I haz the power, 2014-04-28 23:58:17]

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

[eroman, 2014-04-29 00:47:22]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:48:00]

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

[commit-bot: I haz the power, 2014-04-29 09:13:54]

Change committed as 266798

[jsbell, 2014-04-29 16:59:46]

On 2014/04/29 09:13:54, I haz the power (commit-bot) wrote:
> Change committed as 266798

Reverted - 

This was causing a large number of layout test crashes in crypto/

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[eroman, 2014-04-29 18:13:54]

NOTE: There was a bug in this changelist and it has been reverted.

The bug would manifest only non-clang builds so it slipped by my testing :(

The problem was using this pattern:

   state->origin_thread->PostTask(FROM_HERE,
                                  base::Bind(DoEncryptReply, Passed(&state)))

C++ does not define the evaluation order of arguments, and the "this" pointer is
implicitly treated as an argument.

On clang "state->origin_thread" was being evaluated before Passed(&state),
whereas on gcc builds the opposite happens. If Passed() executes first then
|state| is cleared.

[eroman, 2014-04-29 20:05:30]

PTAL at the changes between patchset 13 and 14

[Ryan Sleevi, 2014-04-29 20:12:45]

On 2014/04/29 20:05:30, eroman wrote:
> PTAL at the changes between patchset 13 and 14

Please open a new CL before landing. So request-th the infra gods.

[Ryan Sleevi, 2014-04-29 20:15:59]

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

https://codereview.chromium.org/233733004/diff/350001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:165: }
Not LGTM. I don't like this co-opting of function pointers and templates.

You could just as easily have updated your code to use

void SomeFoo(scoped_ptr<State> passed_state) {
  State* state = passed_state.get();
  state->stuff;
  state->someotherstuff;
  state->result->PostTask(..., Passed(&passed_state));
}

Regardless of the .Pass()/Passed(), State* itself will remain both valid and
pointing to the same object, regardless of which scoped_ptr owns it.

[eroman, 2014-04-29 22:24:51]

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

https://codereview.chromium.org/233733004/diff/350001/content/child/webcrypto...
content/child/webcrypto/webcrypto_impl.cc:165: }
On 2014/04/29 20:16:00, Ryan Sleevi wrote:
> Not LGTM. I don't like this co-opting of function pointers and templates.
> 
> You could just as easily have updated your code to use
> 
> void SomeFoo(scoped_ptr<State> passed_state) {
>   State* state = passed_state.get();
>   state->stuff;
>   state->someotherstuff;
>   state->result->PostTask(..., Passed(&passed_state));
> }
> 
> Regardless of the .Pass()/Passed(), State* itself will remain both valid and
> pointing to the same object, regardless of which scoped_ptr owns it.

Ok I can do that. I thought the new way was superior in that it saves redundancy
(with error handling and posting of tasks).