webcrypto: remove cross-thread cancellation checking.

webcrypto: remove cross-thread cancellation checking.

The operations performed by the crypto worker threads all check if the
issuing Blink thread has cancelled the operation before going ahead.
This is not a sound thing to do in general, as the lifetime of that
thread and its heap is outside of our control.

Remove the checks and unconditionally perform the crypto operations.

R=
BUG=440450

[sof, 2015-07-30 10:59:30]

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

[sof, 2015-07-30 10:59:31]

please take a look.

(cf https://codereview.chromium.org/1228373006/ for what led to this change.)

[eroman, 2015-07-30 19:39:22]

So in the Oilpan world we are expecting that despite WebCryptoResult holding a
"CryptoResult" pointer in m_impl, that pointer will become invalid when the
originating (blink) thread is destroyed? And hence it is necessary that the
pointer only be accessed from the originating (blink) thread?

Is it just that (blink) thread destruction implicitly clears all the
oilpan-allocated objects (i.e the heap is owned by that thread), or is it that
cross-thread references do not keep oilpan objects alive at all? If the latter
is the case, then we could also have a race where posting a task back to the
blink thread, which then accesses the CryptoResultImpl only to find that it was
already deallocated.

Although these cancellation() checks are not essential, they did serve a purpose
to help reclaim resources when closing pages. Because crypto operations can be
arbitrarily slow, and they are asynchronous, and the scheduling of those tasks
is done FIFO, it is possible for pages/webworkers to start a lot of work and
stall the crypto thread for minutes until all preceding tasks have completed.

Closing the originating page or killing the webworker prior to this change would
prevent those abandoned tasks from being run. Whereas with this change they
would all be run until completion, making it more likely the renderer needs to
be killed to recover.

I think it would be better to keep the cancellation checks in some form to
improve the above situation. What if we tracked cancellation through a separate
ref-counted-thread-safe object that is not part of the oilpan heap:

   WebCryptoResult -----> CancellationFlag  <------ WebCryptoResultImpl

This way "CancellationFlag" could outlive the WebCryptoResultImpl, as it would
be managed by the regular heap.

I am still fine with your original approach of clearing the pointer held by
WebCryptoResult, I just want to see WebCryptoResult changed to be
non-copiable/assignable at an API level if we go that route (for instance
passing a WebCryptoResult* across the Blink API rather than a copiable
instance).

What do you think?

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

On 2015/07/30 19:39:22, eroman wrote:
> So in the Oilpan world we are expecting that despite WebCryptoResult holding a
> "CryptoResult" pointer in m_impl, that pointer will become invalid when the
> originating (blink) thread is destroyed? And hence it is necessary that the
> pointer only be accessed from the originating (blink) thread?
> 
> Is it just that (blink) thread destruction implicitly clears all the
> oilpan-allocated objects (i.e the heap is owned by that thread), or is it that
> cross-thread references do not keep oilpan objects alive at all? If the latter
> is the case, then we could also have a race where posting a task back to the
> blink thread, which then accesses the CryptoResultImpl only to find that it
was
> already deallocated.
> 
> Although these cancellation() checks are not essential, they did serve a
purpose
> to help reclaim resources when closing pages. Because crypto operations can be
> arbitrarily slow, and they are asynchronous, and the scheduling of those tasks
> is done FIFO, it is possible for pages/webworkers to start a lot of work and
> stall the crypto thread for minutes until all preceding tasks have completed.
> 
> Closing the originating page or killing the webworker prior to this change
would
> prevent those abandoned tasks from being run. Whereas with this change they
> would all be run until completion, making it more likely the renderer needs to
> be killed to recover.
> 
> I think it would be better to keep the cancellation checks in some form to
> improve the above situation. What if we tracked cancellation through a
separate
> ref-counted-thread-safe object that is not part of the oilpan heap:
> 
>    WebCryptoResult -----> CancellationFlag  <------ WebCryptoResultImpl
> 
> This way "CancellationFlag" could outlive the WebCryptoResultImpl, as it would
> be managed by the regular heap.
> 
> I am still fine with your original approach of clearing the pointer held by
> WebCryptoResult, I just want to see WebCryptoResult changed to be
> non-copiable/assignable at an API level if we go that route (for instance
> passing a WebCryptoResult* across the Blink API rather than a copiable
> instance).
> 
> What do you think?

If you consider the cancellation checks not just optimizations for webcrypto
operations, but want to avoid the above use case, then let's go that route of
splitting out the cancellation checking.
https://codereview.chromium.org/1228373006/ tries to address.

But I do think observing the origining Blink thread being destructed would be
more apt, rather than indirectly do so via the result objects it hands out.

[eroman, 2015-08-01 00:00:17]

On Fri, Jul 31, 2015 at 1:02 AM, <sigbjornf@opera.com> wrote:

> On 2015/07/30 19:39:22, eroman wrote:
>
>> So in the Oilpan world we are expecting that despite WebCryptoResult
>> holding a
>> "CryptoResult" pointer in m_impl, that pointer will become invalid when
>> the
>> originating (blink) thread is destroyed? And hence it is necessary that
>> the
>> pointer only be accessed from the originating (blink) thread?
>>
>
> Is it just that (blink) thread destruction implicitly clears all the
>> oilpan-allocated objects (i.e the heap is owned by that thread), or is it
>> that
>> cross-thread references do not keep oilpan objects alive at all? If the
>> latter
>> is the case, then we could also have a race where posting a task back to
>> the
>> blink thread, which then accesses the CryptoResultImpl only to find that
>> it
>>
> was
>
>> already deallocated.
>>
>
> Although these cancellation() checks are not essential, they did serve a
>>
> purpose
>
>> to help reclaim resources when closing pages. Because crypto operations
>> can be
>> arbitrarily slow, and they are asynchronous, and the scheduling of those
>> tasks
>> is done FIFO, it is possible for pages/webworkers to start a lot of work
>> and
>> stall the crypto thread for minutes until all preceding tasks have
>> completed.
>>
>
> Closing the originating page or killing the webworker prior to this change
>>
> would
>
>> prevent those abandoned tasks from being run. Whereas with this change
>> they
>> would all be run until completion, making it more likely the renderer
>> needs to
>> be killed to recover.
>>
>
> I think it would be better to keep the cancellation checks in some form to
>> improve the above situation. What if we tracked cancellation through a
>>
> separate
>
>> ref-counted-thread-safe object that is not part of the oilpan heap:
>>
>
>     WebCryptoResult -----> CancellationFlag  <------ WebCryptoResultImpl
>>
>
> This way "CancellationFlag" could outlive the WebCryptoResultImpl, as it
>> would
>> be managed by the regular heap.
>>
>
> I am still fine with your original approach of clearing the pointer held by
>> WebCryptoResult, I just want to see WebCryptoResult changed to be
>> non-copiable/assignable at an API level if we go that route (for instance
>> passing a WebCryptoResult* across the Blink API rather than a copiable
>> instance).
>>
>
> What do you think?
>>
>
> If you consider the cancellation checks not just optimizations for
> webcrypto
> operations, but want to avoid the above use case, then let's go that route
> of
> splitting out the cancellation checking.
> https://codereview.chromium.org/1228373006/ tries to address.
>

SGTM.


>
> But I do think observing the origining Blink thread being destructed would
> be
> more apt, rather than indirectly do so via the result objects it hands out.
>
> https://codereview.chromium.org/1264833002/


Sure we can explore observing thread destruction.
But I think in practice it would be more awkward.

If blink thread termination were the only signal to indicate that crypto
operations should be cancelled, it would not handle the case where pages
using webcrypto were unloaded. I believe the way it works today, multiple
execution contexts may be on the main blink thread, and when the page is
navigated away, the active dom objects of the page are stopped (which would
in turn mark the crypto result as cancelled). If we are relying on thread
destruction instead, then this will not be observed.

The other oddity would be how to keep track of things.
Either every WebCrypto operation that was started would have to register a
thread destruction observer, and then de-register it on completion, or it
would need to ensure that it registers itself the first time requests come
from a new thread, and then keep track of which requests were bound to
which thread. This seems like it will amount to more book-keeping then just
keeping the cancellation flag.

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