Reliably cancel in-progress CryptoResult(Impl) upon shutdown.

Reliably cancel in-progress CryptoResult(Impl) upon shutdown.

Move CryptoResult to the Oilpan heap and in order to support such
Oilpan-hosted CryptoResults mainly, also separate out the signalling
of crypto operation cancellation.

CryptoResults are wrapped up (WebCryptoResult) and passed to the
embedder's webcrypto layer upon initiating a crypto operation.
Should the Blink thread that initiated that operation be shut
down (before that crypto operation completes, possibly), it will
go through its shutdown steps, including notifying CryptoResults
that it is stopping. At which point the implementation has to
effectively cancel the ongoing crypto operation.

The Oilpan heaps belonging to the thread shutting down cannot be
reliably used to communicate cancellation status, as they're about
to be detached and destructed also. Hence, we keep a separate
off-heap cancellation object that can live beyond the Blink thread
that initiated it. And can be used by the webcrypto layer to 
safely query for cancellation status after the Blink thread has
been shut down entirely.

As webcrypto operations are handled on separate threads, the
cancellation object references can be safely accessed from any
thread, including handling their eventual destruction. The same goes
for the Oilpan-hosted CryptoResult -- external references (by way
of WebPrivatePtr<>) can be held and destructed from any thread.

R=haraken,eroman,tkent
BUG=440450
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199878

[sof, 2015-07-14 13:23:40]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-07-14 13:23:40]

please take a look.

The handling of CryptResultImpl with ENABLE(OILPAN) worked out by accident so
far, I think.

[sof, 2015-07-14 21:06:57]

Not entirely ready, Oilpan bot ran into a flaky crash on
crypto/subtle/aes-export-key.html

Not able to reproduce locally so far; it could be related to crbug.com/440450 ,
but I'm assuming not & will have to take a closer look tomorrow.

[sof, 2015-07-15 05:25:28]

On 2015/07/14 21:06:57, sof wrote:
> Not entirely ready, Oilpan bot ran into a flaky crash on
> crypto/subtle/aes-export-key.html
> 
> Not able to reproduce locally so far; it could be related to crbug.com/440450
,
> but I'm assuming not & will have to take a closer look tomorrow.

That crash is an untidy shutdown of a Worker (not being able to release all
Persistent<>s upon cleanup().)

So, better fix what underlies http://crbug.com/440450 first. :)

[haraken, 2015-07-15 07:55:03]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-07-15 07:55:05]

Thanks for digging! LGTM once the worker shutdown issue is resolved.

https://codereview.chromium.org/1228373006/diff/20001/Source/modules/crypto/C...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1228373006/diff/20001/Source/modules/crypto/C...
Source/modules/crypto/CryptoResultImpl.cpp:78: m_result = nullptr;

Maybe it would be worth adding ASSERT(!m_result) to the destructor.

https://codereview.chromium.org/1228373006/diff/20001/Source/modules/crypto/C...
Source/modules/crypto/CryptoResultImpl.cpp:181: ScriptState* scriptState =
m_resolver->scriptState();

It would be better to have:

  if(!scriptState->contextIsValid())
    return;

https://codereview.chromium.org/1228373006/diff/20001/Source/modules/crypto/C...
Source/modules/crypto/CryptoResultImpl.cpp:218: ScriptState* scriptState =
m_resolver->scriptState();

Ditto.

[sof, 2015-07-15 07:58:05]

On 2015/07/15 07:55:05, haraken wrote:
> Thanks for digging! LGTM once the worker shutdown issue is resolved.
> 

Thanks for the review; resolving that threading issue for pooled crypto
operations might take a while.

Also running into this via https://codereview.chromium.org/1233173002/

[sof, 2015-07-15 12:47:49]

sigbjornf@opera.com changed reviewers:
+ eroman@chromium.org

[sof, 2015-07-15 12:47:49]

Think I have addressed the issue now. In a reasonably clean fashion (but the
lifetime handling is unconventional, for sure.)

Added eroman@ as reviewer, as this touches on crypto/ innards. And depends on
https://codereview.chromium.org/1240573011/ to be reliable.

With both those two in place, the various flaky crasher issues
http://crbug.com/440450 clears up&out.

[eroman, 2015-07-15 21:34:06]

not lgtm

https://codereview.chromium.org/1228373006/diff/100001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.h (right):

https://codereview.chromium.org/1228373006/diff/100001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.h:101: WebCryptoResult* m_resultOwner;
This doesn't make sense.

WebCryptoResult is a copiable stack-allocated class, which holds a pointer to
CryptoResultImpl.

Consider what happens in this case:

WebCryptoResult x(impl);   // impl->m_resultOwner is now &x
{
  WebCryptoResult y = x;     // assignment changes impl->m_resultOwner to &y
}
// Now y is destroyed, and it sets m_impl->m_resultOwner to nullptr


With you change, upon leaving that second scope (and destroying y), the
underlying WebCryptoResultImpl is left with a null m_resultOwner, even though it
still references the WebCryptoResultImpl.

[sof, 2015-07-16 05:12:15]

On 2015/07/15 21:34:06, eroman wrote:
> not lgtm
> 
>
https://codereview.chromium.org/1228373006/diff/100001/Source/modules/crypto/...
> File Source/modules/crypto/CryptoResultImpl.h (right):
> 
>
https://codereview.chromium.org/1228373006/diff/100001/Source/modules/crypto/...
> Source/modules/crypto/CryptoResultImpl.h:101: WebCryptoResult* m_resultOwner;
> This doesn't make sense.
> 
> WebCryptoResult is a copiable stack-allocated class, which holds a pointer to
> CryptoResultImpl.
> 
> Consider what happens in this case:
> 
> WebCryptoResult x(impl);   // impl->m_resultOwner is now &x
> {
>   WebCryptoResult y = x;     // assignment changes impl->m_resultOwner to &y
> }
> // Now y is destroyed, and it sets m_impl->m_resultOwner to nullptr
> 
> 
> With you change, upon leaving that second scope (and destroying y), the
> underlying WebCryptoResultImpl is left with a null m_resultOwner, even though
it
> still references the WebCryptoResultImpl.

This seems like the approach to use to address the ownership the block of bugs
that issue 440450 exposes, do you an alternate mechanism you'd like to
(constructively) propose?

We could strengthen the implementation of assign() to have it pass the ownership
of the underlying CryptoResultImpl by also clearing out the incoming
WebCryptoResult. At the cost of ignoring it being a const.

The above snippet describes a use that doesn't occur. WebCryptoResult is merely
a result receptacle.

[sof, 2015-07-21 21:40:22]

Elaborated the (un)registration of WebCryptoResults from their underlying
CryptoResultImpl, along with verifying that when an outcome/result is reported
via a completeWith*() method it happens over the expected WebCryptoResult.

One thing that I realized while working through the details here, is that the
Blink Web*Result types that wrap a WebPrivatePtr<T> can only be (de)allocated on
the Oilpan thread that holds the T (heap) object. The WebPrivatePtr<T> will
internally allocate a Persistent<>, and that needs to happen on an Oilpan
thread. If it ends up being done on a non-Oilpan thread (like a crypto worker
thread or the media player threads), the Persistent<> allocation &
initialization will fail.

This isn't currently an issue we're running into, but it is a restriction I've
not come across before or seen mentioned. That restriction does help simplify
the implementation here, as threading issues mostly fall away.

[sof, 2015-07-22 13:11:39]

On 2015/07/21 21:40:22, sof wrote:
> Elaborated the (un)registration of WebCryptoResults from their underlying
> CryptoResultImpl, along with verifying that when an outcome/result is reported
> via a completeWith*() method it happens over the expected WebCryptoResult.
> 
> One thing that I realized while working through the details here, is that the
> Blink Web*Result types that wrap a WebPrivatePtr<T> can only be (de)allocated
on
> the Oilpan thread that holds the T (heap) object. The WebPrivatePtr<T> will
> internally allocate a Persistent<>, and that needs to happen on an Oilpan
> thread. If it ends up being done on a non-Oilpan thread (like a crypto worker
> thread or the media player threads), the Persistent<> allocation &
> initialization will fail.
> 
> This isn't currently an issue we're running into, but it is a restriction I've
> not come across before or seen mentioned. That restriction does help simplify
> the implementation here, as threading issues mostly fall away.

A similar issue is faced for encryptedmedia/ result values in
https://codereview.chromium.org/1249913002/

Based on it, I've arrived at the conclusion that we cannot reliably expose
Oilpan objects via Web*Result values that are accessed, used & destructed on
other threads. With the Web*Result holding a reference to the Oilpan object via
a strong Persistent<>. (This is for non-Oilpan thread usage, we've got support
in place for handling Persistent<>s between Oilpan threads.)

Hence, CryptoResult<> should not be on the Oilpan heap, but must remain a
threadsafe ref-counted object. I've updated the CL accordingly, sync'ing with
https://codereview.chromium.org/1249913002/ . ptal.

[haraken, 2015-07-22 15:16:39]

On 2015/07/22 13:11:39, sof wrote:
> On 2015/07/21 21:40:22, sof wrote:
> > Elaborated the (un)registration of WebCryptoResults from their underlying
> > CryptoResultImpl, along with verifying that when an outcome/result is
reported
> > via a completeWith*() method it happens over the expected WebCryptoResult.
> > 
> > One thing that I realized while working through the details here, is that
the
> > Blink Web*Result types that wrap a WebPrivatePtr<T> can only be
(de)allocated
> on
> > the Oilpan thread that holds the T (heap) object. The WebPrivatePtr<T> will
> > internally allocate a Persistent<>, and that needs to happen on an Oilpan
> > thread. If it ends up being done on a non-Oilpan thread (like a crypto
worker
> > thread or the media player threads), the Persistent<> allocation &
> > initialization will fail.
> > 
> > This isn't currently an issue we're running into, but it is a restriction
I've
> > not come across before or seen mentioned. That restriction does help
simplify
> > the implementation here, as threading issues mostly fall away.
> 
> A similar issue is faced for encryptedmedia/ result values in
> https://codereview.chromium.org/1249913002/
> 
> Based on it, I've arrived at the conclusion that we cannot reliably expose
> Oilpan objects via Web*Result values that are accessed, used & destructed on
> other threads. With the Web*Result holding a reference to the Oilpan object
via
> a strong Persistent<>. (This is for non-Oilpan thread usage, we've got support
> in place for handling Persistent<>s between Oilpan threads.)
> 
> Hence, CryptoResult<> should not be on the Oilpan heap, but must remain a
> threadsafe ref-counted object. I've updated the CL accordingly, sync'ing with
> https://codereview.chromium.org/1249913002/ . ptal.

Thanks for a lot of study. Would it be an option to make WebPrivatePtr<> hold a
CrossThreadPersistent, instead of a Persistent?

I was thinking that that wouldn't be an option because it is anyway a bad thing
to create & access & destruct one WebPrivatePtr by multiple threads. However, it
seems that now we're going to accept the behavior, so I begin to wonder that the
CrossThreadPersistent may be an option.

[sof, 2015-07-22 15:26:48]

On 2015/07/22 15:16:39, haraken wrote:
> On 2015/07/22 13:11:39, sof wrote:
> > On 2015/07/21 21:40:22, sof wrote:
> > > Elaborated the (un)registration of WebCryptoResults from their underlying
> > > CryptoResultImpl, along with verifying that when an outcome/result is
> reported
> > > via a completeWith*() method it happens over the expected WebCryptoResult.
> > > 
> > > One thing that I realized while working through the details here, is that
> the
> > > Blink Web*Result types that wrap a WebPrivatePtr<T> can only be
> (de)allocated
> > on
> > > the Oilpan thread that holds the T (heap) object. The WebPrivatePtr<T>
will
> > > internally allocate a Persistent<>, and that needs to happen on an Oilpan
> > > thread. If it ends up being done on a non-Oilpan thread (like a crypto
> worker
> > > thread or the media player threads), the Persistent<> allocation &
> > > initialization will fail.
> > > 
> > > This isn't currently an issue we're running into, but it is a restriction
> I've
> > > not come across before or seen mentioned. That restriction does help
> simplify
> > > the implementation here, as threading issues mostly fall away.
> > 
> > A similar issue is faced for encryptedmedia/ result values in
> > https://codereview.chromium.org/1249913002/
> > 
> > Based on it, I've arrived at the conclusion that we cannot reliably expose
> > Oilpan objects via Web*Result values that are accessed, used & destructed on
> > other threads. With the Web*Result holding a reference to the Oilpan object
> via
> > a strong Persistent<>. (This is for non-Oilpan thread usage, we've got
support
> > in place for handling Persistent<>s between Oilpan threads.)
> > 
> > Hence, CryptoResult<> should not be on the Oilpan heap, but must remain a
> > threadsafe ref-counted object. I've updated the CL accordingly, sync'ing
with
> > https://codereview.chromium.org/1249913002/ . ptal.
> 
> Thanks for a lot of study. Would it be an option to make WebPrivatePtr<> hold
a
> CrossThreadPersistent, instead of a Persistent?
> 
> I was thinking that that wouldn't be an option because it is anyway a bad
thing
> to create & access & destruct one WebPrivatePtr by multiple threads. However,
it
> seems that now we're going to accept the behavior, so I begin to wonder that
the
> CrossThreadPersistent may be an option.

No, the threads in question are not Oilpan attached.. (and outside of Blink.)

[sof, 2015-07-22 15:53:42]

On 2015/07/22 15:26:48, sof wrote:
> On 2015/07/22 15:16:39, haraken wrote:
> > On 2015/07/22 13:11:39, sof wrote:
> > > On 2015/07/21 21:40:22, sof wrote:
> > > > Elaborated the (un)registration of WebCryptoResults from their
underlying
> > > > CryptoResultImpl, along with verifying that when an outcome/result is
> > reported
> > > > via a completeWith*() method it happens over the expected
WebCryptoResult.
> > > > 
> > > > One thing that I realized while working through the details here, is
that
> > the
> > > > Blink Web*Result types that wrap a WebPrivatePtr<T> can only be
> > (de)allocated
> > > on
> > > > the Oilpan thread that holds the T (heap) object. The WebPrivatePtr<T>
> will
> > > > internally allocate a Persistent<>, and that needs to happen on an
Oilpan
> > > > thread. If it ends up being done on a non-Oilpan thread (like a crypto
> > worker
> > > > thread or the media player threads), the Persistent<> allocation &
> > > > initialization will fail.
> > > > 
> > > > This isn't currently an issue we're running into, but it is a
restriction
> > I've
> > > > not come across before or seen mentioned. That restriction does help
> > simplify
> > > > the implementation here, as threading issues mostly fall away.
> > > 
> > > A similar issue is faced for encryptedmedia/ result values in
> > > https://codereview.chromium.org/1249913002/
> > > 
> > > Based on it, I've arrived at the conclusion that we cannot reliably expose
> > > Oilpan objects via Web*Result values that are accessed, used & destructed
on
> > > other threads. With the Web*Result holding a reference to the Oilpan
object
> > via
> > > a strong Persistent<>. (This is for non-Oilpan thread usage, we've got
> support
> > > in place for handling Persistent<>s between Oilpan threads.)
> > > 
> > > Hence, CryptoResult<> should not be on the Oilpan heap, but must remain a
> > > threadsafe ref-counted object. I've updated the CL accordingly, sync'ing
> with
> > > https://codereview.chromium.org/1249913002/ . ptal.
> > 
> > Thanks for a lot of study. Would it be an option to make WebPrivatePtr<>
hold
> a
> > CrossThreadPersistent, instead of a Persistent?
> > 
> > I was thinking that that wouldn't be an option because it is anyway a bad
> thing
> > to create & access & destruct one WebPrivatePtr by multiple threads.
However,
> it
> > seems that now we're going to accept the behavior, so I begin to wonder that
> the
> > CrossThreadPersistent may be an option.
> 
> No, the threads in question are not Oilpan attached.. (and outside of Blink.)

To some extent, this is a question about what we will allow "foreign" threads to
do inside of Blink & Oilpan.

[sof, 2015-07-22 21:19:23]

On 2015/07/22 15:53:42, sof wrote:
> On 2015/07/22 15:26:48, sof wrote:
> > On 2015/07/22 15:16:39, haraken wrote:
> > > On 2015/07/22 13:11:39, sof wrote:
> > > > On 2015/07/21 21:40:22, sof wrote:
> > > > > Elaborated the (un)registration of WebCryptoResults from their
> underlying
> > > > > CryptoResultImpl, along with verifying that when an outcome/result is
> > > reported
> > > > > via a completeWith*() method it happens over the expected
> WebCryptoResult.
> > > > > 
> > > > > One thing that I realized while working through the details here, is
> that
> > > the
> > > > > Blink Web*Result types that wrap a WebPrivatePtr<T> can only be
> > > (de)allocated
> > > > on
> > > > > the Oilpan thread that holds the T (heap) object. The WebPrivatePtr<T>
> > will
> > > > > internally allocate a Persistent<>, and that needs to happen on an
> Oilpan
> > > > > thread. If it ends up being done on a non-Oilpan thread (like a crypto
> > > worker
> > > > > thread or the media player threads), the Persistent<> allocation &
> > > > > initialization will fail.
> > > > > 
> > > > > This isn't currently an issue we're running into, but it is a
> restriction
> > > I've
> > > > > not come across before or seen mentioned. That restriction does help
> > > simplify
> > > > > the implementation here, as threading issues mostly fall away.
> > > > 
> > > > A similar issue is faced for encryptedmedia/ result values in
> > > > https://codereview.chromium.org/1249913002/
> > > > 
> > > > Based on it, I've arrived at the conclusion that we cannot reliably
expose
> > > > Oilpan objects via Web*Result values that are accessed, used &
destructed
> on
> > > > other threads. With the Web*Result holding a reference to the Oilpan
> object
> > > via
> > > > a strong Persistent<>. (This is for non-Oilpan thread usage, we've got
> > support
> > > > in place for handling Persistent<>s between Oilpan threads.)
> > > > 
> > > > Hence, CryptoResult<> should not be on the Oilpan heap, but must remain
a
> > > > threadsafe ref-counted object. I've updated the CL accordingly, sync'ing
> > with
> > > > https://codereview.chromium.org/1249913002/ . ptal.
> > > 
> > > Thanks for a lot of study. Would it be an option to make WebPrivatePtr<>
> hold
> > a
> > > CrossThreadPersistent, instead of a Persistent?
> > > 
> > > I was thinking that that wouldn't be an option because it is anyway a bad
> > thing
> > > to create & access & destruct one WebPrivatePtr by multiple threads.
> However,
> > it
> > > seems that now we're going to accept the behavior, so I begin to wonder
that
> > the
> > > CrossThreadPersistent may be an option.
> > 
> > No, the threads in question are not Oilpan attached.. (and outside of
Blink.)
> 
> To some extent, this is a question about what we will allow "foreign" threads
to
> do inside of Blink & Oilpan.

ps#4 of https://codereview.chromium.org/1249913002/ explores the alternative of
having cross-thread *Result objects on the heap.

The main issue is GC interaction, to my mind.

[sof, 2015-07-24 08:26:44]

On 2015/07/22 21:19:23, sof wrote:
>
...
> 
> ps#4 of https://codereview.chromium.org/1249913002/ explores the alternative
of
> having cross-thread *Result objects on the heap.
> 
> The main issue is GC interaction, to my mind.

With that having landed as r199421, rebased and switched WebCryptoResult's
WebPrivatePtr<> to be cross-thread destructible.

Consequently, CryptoResult can (again!) be on the Oilpan heap; moved it back.
ptal?

[eroman, 2015-07-25 01:44:38]

Can you give some more context on what this change is trying to accomplish?

I certainly appreciate the work being done and don't want to stand in your way,
but WebCryptoResult is already extremely fidgety as it is due to the threading
constraints, and this seems like it adds yet more indirection.

The bug references 440450 but that is fairly vague. The remaining problems I was
aware of for 440450 come from
https://code.google.com/p/chromium/issues/detail?id=413518 and not specifically
the WebCryptoResult...

Why not keep CryptoResultImpl reference counted?

Is the issue that to make the ScriptPromiseResolver live on oilpan heap it
cannot be a member of ScriptResultImpl?

My high level issue with this changelist remains that WebCryptoResult is a
copiable class, and from an API perspective it is a violation to treat it as a
movable type and have a single WebCryptoResult owner for CryptoResultImpl.

If we need to make CryptoResultImpl have a pointer back to the then we should
change that, and for instance pass a PassOwnPtr<> around instead (not sure what
the Blink API equivalent would be). Or consider a different approach where the
WebCryptoResult holds a non-pointer type like a numeric handle.

To help frame our conversation, I also put together some diagrams and a document
showing how WebCryptoResult currently works (without this change). The
modifications in this CL would add an extra back pointer from CryptoResultImpl
to WebCryptoResult. This was as much for my sake to remember how it all works :)

https://docs.google.com/document/d/1d29wPnZskmtMJcIgvlFKvJ7vXONlTmK0YZi9Pgm8-...

https://codereview.chromium.org/1228373006/diff/180001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.h (right):

https://codereview.chromium.org/1228373006/diff/180001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.h:94: Mutex m_mutex;
Does this work? My understanding of this code is it relies on allowing recursive
locking (for instance in cancel()), so need RecursiveMutex

[sof, 2015-07-26 18:31:56]

On 2015/07/25 01:44:38, eroman wrote:
> Can you give some more context on what this change is trying to accomplish?
> 

Besides making WebCryptoResult hold a CrossThreadPersistent<> to a
CryptoResultImpl (via WebPrivatePtr<>), it is going after what happens after
cancel() has been called on CryptoResultImpl.

I hope that is clearly enough described in the above comments & in
CryptoResultImpl.h, but to repeat/rephrase, should the background thread
complete a crypto operation after that cancel(), it will check cancellation
status of the CryptoResultImpl before attempting to forward a reply by posting a
task to the main(?) thread.

What prevents the execution context, web worker contexts in particular, from
having been torn down completely & gone by that stage? The thread-savvy
ref-counted reference (or persistent) that the WebCryptoResult holds will not
prevent that teardown from running to completion. i.e., touching
CryptoResultImpl after cancel() seems unsafe, hence the addition of a
synchronous cancellation of the WebCryptoResult here, which prevents such
access. I'm worried about workers and their heaps, in particular, as the
CryptoResultImpl would have been created on the worker's heap.

That said, I cannot reproduce a failure now due to this with the crypto/subtle
tests (without Oilpan or with, using cross-thread persistent). So, either there
are timing issues that makes this hard to reproduce, or I'm assuming the
background crypto threads will outlive worker threads longer than they actually
are, rendering this issue a non-problem in practice. ?

I'm happy to hold off on changing the cancellation behavior of CryptoResultImpl
(by cancelling the registered WebCryptoResult, or some equivalent protocol)
until the current cancellation manifests itself as a real problem -- I thought
440450 and the TestExpectations exemptions connected to it represented that --
and only switch CryptoResultImpl to being on the Oilpan heap when it's enabled,
along with using cross-thread persistents to it from WebCryptoResult.

That would enable moving ScriptPromiseResolver to the heap proper,
https://codereview.chromium.org/1233173002/ , which is the motivation for this
CL.

[Thanks for the writeup and diagram btw, clear as could be wished for & aligns
with what I've been able to derive from the code and pre-existing commments. If
that document isn't referred to already someplace, it might be worth adding an
href some place central (the CryptoResultImpl.h header comment?) ]

[eroman, 2015-07-27 19:13:25]

On Sun, Jul 26, 2015 at 11:31 AM, <sigbjornf@opera.com> wrote:

> On 2015/07/25 01:44:38, eroman wrote:
>
>> Can you give some more context on what this change is trying to
>> accomplish?
>>
>
>
> Besides making WebCryptoResult hold a CrossThreadPersistent<> to a
> CryptoResultImpl (via WebPrivatePtr<>), it is going after what happens
> after
> cancel() has been called on CryptoResultImpl.
>
> I hope that is clearly enough described in the above comments & in
> CryptoResultImpl.h, but to repeat/rephrase, should the background thread
> complete a crypto operation after that cancel(), it will check cancellation
> status of the CryptoResultImpl before attempting to forward a reply by
> posting a
> task to the main(?) thread.
>

Unless I am misunderstand the writing, I don't agree with the above.

Cancellation is only checked _after_ posting a task to the blink thread.
(Both in the current approach, and in the proposed changelist.) [1]

Posting the task from the crypto worker thread back to the blink thread
(WebWorker or otherwise) is not guaranteed to succeed, which is the source
of the confusing destruction semantics.

[1] See webcrypto_impl.cc


>
> What prevents the execution context, web worker contexts in particular,
> from
> having been torn down completely & gone by that stage? The thread-savvy
> ref-counted reference (or persistent) that the WebCryptoResult holds will
> not
> prevent that teardown from running to completion. i.e., touching
> CryptoResultImpl after cancel() seems unsafe, hence the addition of a
> synchronous cancellation of the WebCryptoResult here, which prevents such
> access. I'm worried about workers and their heaps, in particular, as the
> CryptoResultImpl would have been created on the worker's heap.
>

Isn't it the case that today when the execution context is torn down
currently, the ActiveDOMObjects are stopped, and hence:
  (1) CryptoResultImpl::Resolver::stop() is called
  (2) CryptoResultImpl::clearResolver() is called
  (3) m_resolver is set to nullptr

Therefore when calling any of the CryptoResultImpl::completeWith*() methods
after cancellation has occurred, m_result will be nullptr and they will all
early abort?


> That said, I cannot reproduce a failure now due to this with the
> crypto/subtle
> tests (without Oilpan or with, using cross-thread persistent). So, either
> there
> are timing issues that makes this hard to reproduce, or I'm assuming the
> background crypto threads will outlive worker threads longer than they
> actually
> are, rendering this issue a non-problem in practice. ?
>

When trying to reproduce the races, be sure to uncomment the expected
failure lines, otherwise the failures may be getting hidden.

That said, the races when aborting WebWorkers are timing dependent, and may
be hard to reproduce (there has also been work done on issue 413518 which
may have fixed some things?)

The crypto worker thread can be made to live arbitrarily long after
termination of the WebWorker that originated the operation. Maybe sticking
some PlatformThread::Sleep() into webcrypto_impl.cc would help make things
more reproducible.


>
> I'm happy to hold off on changing the cancellation behavior of
> CryptoResultImpl
> (by cancelling the registered WebCryptoResult, or some equivalent protocol)
> until the current cancellation manifests itself as a real problem -- I
> thought
> 440450 and the TestExpectations exemptions connected to it represented
> that --
> and only switch CryptoResultImpl to being on the Oilpan heap when it's
> enabled,
> along with using cross-thread persistents to it from WebCryptoResult.
>

I appreciate the spirit of this change, and don't mean to be difficult. I
just want to make sure I really understand this before signing off. Please
bear with me as we do some back and forths. I too really want to see the
WebWorker related bugs be fixed :)


>
> That would enable moving ScriptPromiseResolver to the heap proper,
> https://codereview.chromium.org/1233173002/ , which is the motivation for
> this
> CL.


For my own benefit, as I am not very familiar with oilpan:
  * Do oilpan garbage collected objects need to be exclusive to a
particular thread?
  * What happens when these objects are held on chromium-threads? Are they
properly traced here too, or does it require additional infrastructure?




> [Thanks for the writeup and diagram btw, clear as could be wished for &
> aligns
> with what I've been able to derive from the code and pre-existing
> commments. If
> that document isn't referred to already someplace, it might be worth
> adding an
> href some place central (the CryptoResultImpl.h header comment?) ]
>

That sounds like a good idea, I will add a link to it.
That class has gone through several iterations to date, and in my opinion
is quite difficult to understand.


> https://codereview.chromium.org/1228373006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof1, 2015-07-27 20:35:45]

Den 7/27/2015 21:13, Eric Roman skreiv:
>
>
> On Sun, Jul 26, 2015 at 11:31 AM, <sigbjornf@opera.com
> <mailto:sigbjornf@opera.com>> wrote:
>
>     On 2015/07/25 01:44:38, eroman wrote:
>
>         Can you give some more context on what this change is trying to
>         accomplish?
>
>
>
>     Besides making WebCryptoResult hold a CrossThreadPersistent<> to a
>     CryptoResultImpl (via WebPrivatePtr<>), it is going after what
>     happens after
>     cancel() has been called on CryptoResultImpl.
>
>     I hope that is clearly enough described in the above comments & in
>     CryptoResultImpl.h, but to repeat/rephrase, should the background thread
>     complete a crypto operation after that cancel(), it will check
>     cancellation
>     status of the CryptoResultImpl before attempting to forward a reply
>     by posting a
>     task to the main(?) thread.
>
>
> Unless I am misunderstand the writing, I don't agree with the above.
>
> Cancellation is only checked _after_ posting a task to the blink thread.
> (Both in the current approach, and in the proposed changelist.) [1]
>

_Before_ doing the operation and _only_ on the crypto thread atm, no? e.g.,

 
https://code.google.com/p/chromium/codesearch#chromium/src/components/webcryp...

--sigbjorn

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[eroman, 2015-07-27 21:13:36]

On Mon, Jul 27, 2015 at 1:35 PM, Sigbjorn Finne <sof@opera.com> wrote:

> Den 7/27/2015 21:13, Eric Roman skreiv:
>
>>
>>
>> On Sun, Jul 26, 2015 at 11:31 AM, <sigbjornf@opera.com
>> <mailto:sigbjornf@opera.com>> wrote:
>>
>>     On 2015/07/25 01:44:38, eroman wrote:
>>
>>         Can you give some more context on what this change is trying to
>>         accomplish?
>>
>>
>>
>>     Besides making WebCryptoResult hold a CrossThreadPersistent<> to a
>>     CryptoResultImpl (via WebPrivatePtr<>), it is going after what
>>     happens after
>>     cancel() has been called on CryptoResultImpl.
>>
>>     I hope that is clearly enough described in the above comments & in
>>     CryptoResultImpl.h, but to repeat/rephrase, should the background
>> thread
>>     complete a crypto operation after that cancel(), it will check
>>     cancellation
>>     status of the CryptoResultImpl before attempting to forward a reply
>>     by posting a
>>     task to the main(?) thread.
>>
>>
>> Unless I am misunderstand the writing, I don't agree with the above.
>>
>> Cancellation is only checked _after_ posting a task to the blink thread.
>> (Both in the current approach, and in the proposed changelist.) [1]
>>
>>
> _Before_ doing the operation and _only_ on the crypto thread atm, no? e.g.,
>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/webcryp...
>
>
Oh right, I guess we have those checks too, thanks for point it out.

However they are purely optimization. From a correctness standpoint we
can't get away from checking cancellation on the blink thread (without
holding a lock here), since cancellation happens on a different thread.

--sigbjorn
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof1, 2015-07-28 07:37:44]

Den 7/27/2015 23:13, Eric Roman skreiv:
>
>
> On Mon, Jul 27, 2015 at 1:35 PM, Sigbjorn Finne <sof@opera.com
> <mailto:sof@opera.com>> wrote:
>
>     Den 7/27/2015 21:13, Eric Roman skreiv:
>
>
>
>         On Sun, Jul 26, 2015 at 11:31 AM, <sigbjornf@opera.com
>         <mailto:sigbjornf@opera.com>
>         <mailto:sigbjornf@opera.com <mailto:sigbjornf@opera.com>>> wrote:
>
>              On 2015/07/25 01:44:38, eroman wrote:
>
>                  Can you give some more context on what this change is
>         trying to
>                  accomplish?
>
>
>
>              Besides making WebCryptoResult hold a
>         CrossThreadPersistent<> to a
>              CryptoResultImpl (via WebPrivatePtr<>), it is going after what
>              happens after
>              cancel() has been called on CryptoResultImpl.
>
>              I hope that is clearly enough described in the above
>         comments & in
>              CryptoResultImpl.h, but to repeat/rephrase, should the
>         background thread
>              complete a crypto operation after that cancel(), it will check
>              cancellation
>              status of the CryptoResultImpl before attempting to forward
>         a reply
>              by posting a
>              task to the main(?) thread.
>
>
>         Unless I am misunderstand the writing, I don't agree with the above.
>
>         Cancellation is only checked _after_ posting a task to the blink
>         thread.
>         (Both in the current approach, and in the proposed changelist.) [1]
>
>
>     _Before_ doing the operation and _only_ on the crypto thread atm,
>     no? e.g.,
>
>
>    
https://code.google.com/p/chromium/codesearch#chromium/src/components/webcryp...
>
> Oh right, I guess we have those checks too, thanks for point it out.
>
> However they are purely optimization. From a correctness standpoint we
> can't get away from checking cancellation on the blink thread (without
> holding a lock here), since cancellation happens on a different thread.
>

That access of the CryptoResultImpl from the crypto thread is the 
concern. I wrongly placed it in the above prose, but if I'd 
misunderstood who performed it, the WebCryptoResult additions in this CL 
would have fallen away entirely.

I'll follow up on your other feedback next.

--sigbjorn

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof1, 2015-07-28 12:58:58]

Den 7/27/2015 21:13, Eric Roman skreiv:
>
>
...
>
>     What prevents the execution context, web worker contexts in
>     particular, from
>     having been torn down completely & gone by that stage? The thread-savvy
>     ref-counted reference (or persistent) that the WebCryptoResult holds
>     will not
>     prevent that teardown from running to completion. i.e., touching
>     CryptoResultImpl after cancel() seems unsafe, hence the addition of a
>     synchronous cancellation of the WebCryptoResult here, which prevents
>     such
>     access. I'm worried about workers and their heaps, in particular, as the
>     CryptoResultImpl would have been created on the worker's heap.
>
>
> Isn't it the case that today when the execution context is torn down
> currently, the ActiveDOMObjects are stopped, and hence:
>    (1) CryptoResultImpl::Resolver::stop() is called
>    (2) CryptoResultImpl::clearResolver() is called
>    (3) m_resolver is set to nullptr
>
> Therefore when calling any of the CryptoResultImpl::completeWith*()
> methods after cancellation has occurred, m_result will be nullptr and
> they will all early abort?
>

Yes, once/if we get to run a Blink task executing completeWith*(), 
details will work out fine for already cancelled results.

>
>     That said, I cannot reproduce a failure now due to this with the
>     crypto/subtle
>     tests (without Oilpan or with, using cross-thread persistent). So,
>     either there
>     are timing issues that makes this hard to reproduce, or I'm assuming the
>     background crypto threads will outlive worker threads longer than
>     they actually
>     are, rendering this issue a non-problem in practice. ?
>
>
> When trying to reproduce the races, be sure to uncomment the expected
> failure lines, otherwise the failures may be getting hidden.
>
> That said, the races when aborting WebWorkers are timing dependent, and
> may be hard to reproduce (there has also been work done on issue 413518
> which may have fixed some things?)
>
> The crypto worker thread can be made to live arbitrarily long after
> termination of the WebWorker that originated the operation. Maybe
> sticking some PlatformThread::Sleep() into webcrypto_impl.cc would help
> make things more reproducible.
>
>
>     I'm happy to hold off on changing the cancellation behavior of
>     CryptoResultImpl
>     (by cancelling the registered WebCryptoResult, or some equivalent
>     protocol)
>     until the current cancellation manifests itself as a real problem --
>     I thought
>     440450 and the TestExpectations exemptions connected to it
>     represented that --
>     and only switch CryptoResultImpl to being on the Oilpan heap when
>     it's enabled,
>     along with using cross-thread persistents to it from WebCryptoResult.
>
>
> I appreciate the spirit of this change, and don't mean to be difficult.
> I just want to make sure I really understand this before signing off.
> Please bear with me as we do some back and forths. I too really want to
> see the WebWorker related bugs be fixed :)
>

Well, I've been unsuccessful for this past day in reproducing a failure 
due to cancellation. Even when introducing timing delays to make it more 
likely to surface across web worker shutdowns. So, given the complexity 
that "WebCryptoResult cancellation" adds, I don't want to push for that 
to be used here. We can revisit later, if needs be.

(Removing the cancelled() usage on the webcrypto thread side might well 
be a sufficient cure, I think.)

I've uploaded a new patchset which limits the changes to using a 
cross-thread persistent for CryptoResults + moves the CryptoResult(Impl) 
objects to the Oilpan heap.

>
>     That would enable moving ScriptPromiseResolver to the heap proper,
>     https://codereview.chromium.org/1233173002/ , which is the
>     motivation for this
>     CL.
>
>
> For my own benefit, as I am not very familiar with oilpan:
>    * Do oilpan garbage collected objects need to be exclusive to a
> particular thread?

Objects are allocated on a heap belonging to the thread making the 
allocation. Meaning that a thread has to be "attached" to Oilpan, 
whereby it has its own heap. Multithreaded code can work over those 
objects, just like outside of Oilpan.

When the objects become garbage, their destructors will run on the 
thread that allocated them.

>    * What happens when these objects are held on chromium-threads? Are
> they properly traced here too, or does it require additional infrastructure?
>

As long as pointers to those objects are kept by way of 
WebPrivatePtr<T>s from Chromium threads, we can handle it. Under the 
hood, a strong reference (of type Persistent<TT>) is held for the Ts 
that are allocated on the Oilpan heap. Which means that the reference is 
known to Oilpan such that it will be traced and the object is points to 
be kept alive across a GC without having to record & maintain traceable 
objects on the Chromium side also.

Persistent<> have a same-thread requirement as regards to 
release/destruction, though. Meaning that destruction must happen on the 
same thread as the thread that allocated it (i.e., the destruction of 
the Persistent<> reference, the heap object it points to & its orderly 
destruction is a separate matter that the Oilpan GC takes care of.) 
Should the WebPrivatePtr<>-wrapped persistent be passed along to another 
Chromium thread and could end up be destructed there, we need to mark it 
as "cross thread destructible". That's what the extra 
WebPrivatePtrDestructionCrossThread argument in 
https://codereview.chromium.org/1228373006/patch/200001/210006 takes 
care of. The thread destructing the persistent-holding WebPrivatePtr<> 
doesn't have to be the allocating thread, nor be an Oilpan attached thread.

Such cross-thread use is currently rare, hence it not being the default 
for WebPrivatePtr<>. Notice also that thread allocating the object 
that's wrapped in a cross-thread persistent must remain alive until the 
persistent is destructed, otherwise we risk either having a dangling 
pointer into its finalized heap or have that heap be reported as leaking 
on shutdown That's an underlying concern here for webcrypto and worker 
threads.

--sigbjorn

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof, 2015-07-29 12:13:50]

Hmm, crypto/subtle/aes-export-key.html is flakily failing on the Oilpan debug
bot with the last, cut-down patchset.

[sof, 2015-07-29 13:30:15]

On 2015/07/29 12:13:50, sof wrote:
> Hmm, crypto/subtle/aes-export-key.html is flakily failing on the Oilpan debug
> bot with the last, cut-down patchset.

I can reproduce on another workstation; due to webcrypto threads calling
cancelled() on an already stopped&shutdown worker.

So, either drop those calls or adopt something close to ps#10 -- which approach
is preferable? (We need to get this CL going along.)

[sof, 2015-07-30 11:10:42]

On 2015/07/29 13:30:15, sof wrote:
> On 2015/07/29 12:13:50, sof wrote:
> > Hmm, crypto/subtle/aes-export-key.html is flakily failing on the Oilpan
debug
> > bot with the last, cut-down patchset.
> 
> I can reproduce on another workstation; due to webcrypto threads calling
> cancelled() on an already stopped&shutdown worker.
> 
> So, either drop those calls or adopt something close to ps#10 -- which
approach
> is preferable? (We need to get this CL going along.)

I've decided that I'd prefer to remove those cross-thread cancelled() calls;
uploaded https://codereview.chromium.org/1264833002/.

If accepted, this CL can stay unchanged, but the then-unused
WebCryptoResult::cancelled() method & implementation can be removed. Via this CL
or in a followup, whichever is preferable.

I realized that Oilpan's handling of cross-thread persistents could be better;
filed https://code.google.com/p/chromium/issues/detail?id=515432 for that.

[sof, 2015-07-31 08:11:17]

On 2015/07/30 11:10:42, sof wrote:
> On 2015/07/29 13:30:15, sof wrote:
> > On 2015/07/29 12:13:50, sof wrote:
> > > Hmm, crypto/subtle/aes-export-key.html is flakily failing on the Oilpan
> debug
> > > bot with the last, cut-down patchset.
> > 
> > I can reproduce on another workstation; due to webcrypto threads calling
> > cancelled() on an already stopped&shutdown worker.
> > 
> > So, either drop those calls or adopt something close to ps#10 -- which
> approach
> > is preferable? (We need to get this CL going along.)
> 
> I've decided that I'd prefer to remove those cross-thread cancelled() calls;
> uploaded https://codereview.chromium.org/1264833002/.
> 
> If accepted, this CL can stay unchanged, but the then-unused
> WebCryptoResult::cancelled() method & implementation can be removed. Via this
CL
> or in a followup, whichever is preferable.
> 
> I realized that Oilpan's handling of cross-thread persistents could be better;
> filed https://code.google.com/p/chromium/issues/detail?id=515432 for that.

See https://codereview.chromium.org/1264833002/ why keeping those cancelled()
checks on the webcrypto side is preferable.

Adjusted this CL instead, separating out the cancellation status handling from
the (on heap) CryptoResult.

[eroman, 2015-07-31 23:40:30]

LGTM!

Thanks for working through my concerns and handling this issue. I find this
modified design easier to reason about.

Also note, please update the changelist description to reflect the latest
patchset.

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:77: m_result->clearResolver();
optional: It might be clearer to remove this line, and instead make it part of
CryptoResultImpl::cancel(). (Not really part of your change, but noticed it
since modifying cancel).

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:249: m_cancel.release();
optional: It might be more idiomatic to write this as:
   m_cancel.clear();

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.h (right):

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.h:53: //  * cancel() can only be called
from the origin thread.
I suggest generalizing this comment, by replacing it (and the line below) with
something like:

 * All method of CryptoResultImpl must be called from the origin thread (where
it was constructed). The exception is that ref(), deref(), and destruction may
happen on another thread.

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.h:84: class Resolver;
style nit: I thought these declarations were expected before the constructor,
but not sure if it is spelled out anywhere.

https://codereview.chromium.org/1228373006/diff/220001/Source/platform/export...
File Source/platform/exported/WebCryptoResult.cpp (right):

https://codereview.chromium.org/1228373006/diff/220001/Source/platform/export...
Source/platform/exported/WebCryptoResult.cpp:41: if (!m_cancel->cancelled())
You could call the wrapper method for these:

  if (!cancelled())
     ...

[eroman, 2015-08-01 00:13:14]

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:139: return;
IMPORTANT: This return doesn't seem right. In the edge case where this codepath
is reached, m_cancel will be left as a nullptr, which will cause a crash in when
calling the WebCryptoResult::completeWith*() methods.

Instead you should initialize it at a minimum to a valid pointer here (and
preferably to a value indicating it is cancelled).

[sof, 2015-08-01 06:44:30]

sigbjornf@opera.com changed reviewers:
+ mkwst@chromium.org, tkent@chromium.org
- sof@opera.com

[sof, 2015-08-01 06:44:31]

Thanks much for the review, we arrived at something quite reasonable in the end.

mkwst, tkent: rubberstamp on the public/ change, please? (including you both in
this OOO-ful period) tia.

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:77: m_result->clearResolver();
On 2015/07/31 23:40:30, eroman wrote:
> optional: It might be clearer to remove this line, and instead make it part of
> CryptoResultImpl::cancel(). (Not really part of your change, but noticed it
> since modifying cancel).

Moved clearResolver() (it was moved over in some of the earlier patchsets here
also.)

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:139: return;
On 2015/08/01 00:13:14, eroman wrote:
> IMPORTANT: This return doesn't seem right. In the edge case where this
codepath
> is reached, m_cancel will be left as a nullptr, which will cause a crash in
when
> calling the WebCryptoResult::completeWith*() methods.
> 
> Instead you should initialize it at a minimum to a valid pointer here (and
> preferably to a value indicating it is cancelled).

The WebCryptoResult ctor will complain if that happens; initialized m_cancel as
cancelled.

It seems wasteful to be able to initiate a webcrypto operation if the object is
in a stopped state, when it is considered cancelled already.

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:249: m_cancel.release();
On 2015/07/31 23:40:30, eroman wrote:
> optional: It might be more idiomatic to write this as:
>    m_cancel.clear();

Done.

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.h (right):

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.h:53: //  * cancel() can only be called
from the origin thread.
On 2015/07/31 23:40:30, eroman wrote:
> I suggest generalizing this comment, by replacing it (and the line below) with
> something like:
> 
>  * All method of CryptoResultImpl must be called from the origin thread (where
> it was constructed). The exception is that ref(), deref(), and destruction may
> happen on another thread.

Reworded.

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.h:84: class Resolver;
On 2015/07/31 23:40:30, eroman wrote:
> style nit: I thought these declarations were expected before the constructor,
> but not sure if it is spelled out anywhere.

Swapped the ordering back to what it was.

https://codereview.chromium.org/1228373006/diff/220001/Source/platform/export...
File Source/platform/exported/WebCryptoResult.cpp (right):

https://codereview.chromium.org/1228373006/diff/220001/Source/platform/export...
Source/platform/exported/WebCryptoResult.cpp:41: if (!m_cancel->cancelled())
On 2015/07/31 23:40:30, eroman wrote:
> You could call the wrapper method for these:
> 
>   if (!cancelled())
>      ...

Done.

[haraken, 2015-08-01 15:25:54]

(I'm not following the details but want to say thank you :-)

[tkent, 2015-08-02 23:39:14]

public lgtm

[sof, 2015-08-03 05:09:24]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-08-03 05:09:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1228373006/#ps240001
(title: "initialize stopped crypto result as cancelled")

[commit-bot: I haz the power, 2015-08-03 05:09:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228373006/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1228373006/240001

[commit-bot: I haz the power, 2015-08-03 06:44:36]

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199878

[eroman, 2015-08-03 19:00:29]

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:139: return;
On 2015/08/01 06:44:30, sof wrote:
> On 2015/08/01 00:13:14, eroman wrote:
> > IMPORTANT: This return doesn't seem right. In the edge case where this
> codepath
> > is reached, m_cancel will be left as a nullptr, which will cause a crash in
> when
> > calling the WebCryptoResult::completeWith*() methods.
> > 
> > Instead you should initialize it at a minimum to a valid pointer here (and
> > preferably to a value indicating it is cancelled).
> 
> The WebCryptoResult ctor will complain if that happens; initialized m_cancel
as
> cancelled.
> 
> It seems wasteful to be able to initiate a webcrypto operation if the object
is
> in a stopped state, when it is considered cancelled already.

The WebCryptoResult ctor will merely ASSERT().

It is important to handle this, otherwise we are going to crash later.

If we can reach this codepath, then we will be able to reach the completeWith*()
codepath as well, which is then going to crash....

The only case this doesn't matter is if you can prove that this branch is never
entered.

[sof, 2015-08-03 19:03:07]

On 2015/08/03 19:00:29, eroman wrote:
>
https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
> File Source/modules/crypto/CryptoResultImpl.cpp (right):
> 
>
https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
> Source/modules/crypto/CryptoResultImpl.cpp:139: return;
> On 2015/08/01 06:44:30, sof wrote:
> > On 2015/08/01 00:13:14, eroman wrote:
> > > IMPORTANT: This return doesn't seem right. In the edge case where this
> > codepath
> > > is reached, m_cancel will be left as a nullptr, which will cause a crash
in
> > when
> > > calling the WebCryptoResult::completeWith*() methods.
> > > 
> > > Instead you should initialize it at a minimum to a valid pointer here (and
> > > preferably to a value indicating it is cancelled).
> > 
> > The WebCryptoResult ctor will complain if that happens; initialized m_cancel
> as
> > cancelled.
> > 
> > It seems wasteful to be able to initiate a webcrypto operation if the object
> is
> > in a stopped state, when it is considered cancelled already.
> 
> The WebCryptoResult ctor will merely ASSERT().
> 
> It is important to handle this, otherwise we are going to crash later.
> 
> If we can reach this codepath, then we will be able to reach the
completeWith*()
> codepath as well, which is then going to crash....
> 
> The only case this doesn't matter is if you can prove that this branch is
never
> entered.

Isn't it handled now?

[eroman, 2015-08-03 19:45:35]

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:139: return;
On 2015/08/03 19:00:29, eroman wrote:
> On 2015/08/01 06:44:30, sof wrote:
> > On 2015/08/01 00:13:14, eroman wrote:
> > > IMPORTANT: This return doesn't seem right. In the edge case where this
> > codepath
> > > is reached, m_cancel will be left as a nullptr, which will cause a crash
in
> > when
> > > calling the WebCryptoResult::completeWith*() methods.
> > > 
> > > Instead you should initialize it at a minimum to a valid pointer here (and
> > > preferably to a value indicating it is cancelled).
> > 
> > The WebCryptoResult ctor will complain if that happens; initialized m_cancel
> as
> > cancelled.
> > 
> > It seems wasteful to be able to initiate a webcrypto operation if the object
> is
> > in a stopped state, when it is considered cancelled already.
> 
> The WebCryptoResult ctor will merely ASSERT().
> 
> It is important to handle this, otherwise we are going to crash later.
> 
> If we can reach this codepath, then we will be able to reach the
completeWith*()
> codepath as well, which is then going to crash....
> 
> The only case this doesn't matter is if you can prove that this branch is
never
> entered.

Ah, I didn't notice that you addressed this in the following patchset, please
disregard then :)

Still LGTM

[sof, 2015-08-03 19:59:56]

On 2015/08/03 19:45:35, eroman wrote:
>
https://codereview.chromium.org/1228373006/diff/220001/Source/modules/crypto/...
> File Source/modules/crypto/CryptoResultImpl.cpp (right):
> 
....
> > 
> > The WebCryptoResult ctor will merely ASSERT().
> > 
> > It is important to handle this, otherwise we are going to crash later.
> > 
> > If we can reach this codepath, then we will be able to reach the
> completeWith*()
> > codepath as well, which is then going to crash....
> > 
> > The only case this doesn't matter is if you can prove that this branch is
> never
> > entered.
> 
> Ah, I didn't notice that you addressed this in the following patchset, please
> disregard then :)
> 

ok, thanks for confirming - no shortage of patchsets to sift through here :)