[webcrypto] Fix a race in BlinkPlatformImpl::crypto(), by using eager initilization of WebCryptoImpl

[webcrypto] Fix a race in BlinkPlatformImpl::crypto(), by using eager initilization of WebCryptoImpl.

BUG=371243, 245025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270215

[Ryan Sleevi, 2014-05-09 18:58:39]

This seems like it attempts to paper over the issue, rather than look at
architectural resolution.

Either way, I'm not familiar enough with this code - or the overall
architectural design issues - to be a good reviewer, but to my naive nose, it
has a distinctive code smell.

[eroman, 2014-05-09 19:00:26]

What smells to you?

[Ryan Sleevi, 2014-05-09 19:12:30]

On 2014/05/09 19:00:26, eroman wrote:
> What smells to you?

blink::Platform being accessed from multiple threads.

This hides the lazy-init into a member init, but overall, it seems there are a
number of members - including some functions like the setSharedTimer - that are
inherently thread-unsafe.

Someone more familiar with blink::Platform should be reviewing this.

[eroman, 2014-05-09 19:19:04]

@jam: Could you review this?

[jam, 2014-05-09 20:18:29]

I agree with Ryan that calling webcrypto::Init() in every WebCrypto method is
error-prone; someone could forget to do this in the future, or remove an
existing one.

I see two alternatives:
-adding a lock inside the crypto() method to guard against the creation of the
object
-looking at what webcrypto::Init() does in detail:
  -for NSS, platform_crypto_nss.cc calls crypto::EnsureNSSInit()
    -ChromeRenderProcessObserver::ChromeRenderProcessObserver already calls this
through crypto::InitNSSSafely() as it needs to be done before the sandbox is
engaged, so this call is not needed (and probably useless because of the
sandbox)
      -as an aside, the crypto::InitNSSSafely() call should move to content as
every embedder of content will need this
  -for OpenSSL, platform_crypto_openssl.cc calls crypto::EnsureOpenSSLInit()
which ends up in OpenSSLInitSingleton::OpenSSLInitSingleton()
    -can you guys comment on how expensive that call is? I don't see it being
made in other startup code, so we will need to call it explicitly. if it's
cheap, we can do it on startup like the NSS case, otherwise within a lock
lazily?

[eroman, 2014-05-12 23:21:25]

To answer the question of performance, on my z620 running Linux:
  - EnsureNSSInit() averaged 70ms
  - EnsureOpenSSLInit() averaged 1ms

I didn't measure memory cost, but certainly not zero-cost either. So I think
these should be kept lazy when possible rather than process initialization.
Thanks for pointing out that for Linux, Chrome initializes NSS before engaging
sandbox. Interestingly the tests using NSS are being run from content_shell
which I thought used sandbox, so not sure what to make of that.

To move forward with this bug fix and address the concerns, it sounds like you
would be satisfied if I add a single lock around the crypto() accessor? (The
downside is it means extra work in the non-webworker case).

Alternately I could reduce the number of EnsureNSSInit() by putting them only at
key construction callsites, and for digest.

[Ryan Sleevi, 2014-05-12 23:26:35]

On May 12, 2014 4:21 PM, <eroman@chromium.org> wrote:
>
> To answer the question of performance, on my z620 running Linux:
>   - EnsureNSSInit() averaged 70ms
>   - EnsureOpenSSLInit() averaged 1ms
>

This doesn't sound right. How were you testing? This cost is already
supposed to be paid at renderer startup so that its a noop by the time
WebCrypto calls it.

> I didn't measure memory cost, but certainly not zero-cost either. So I
think
> these should be kept lazy when possible rather than process
initialization.
> Thanks for pointing out that for Linux, Chrome initializes NSS before
engaging
> sandbox. Interestingly the tests using NSS are being run from
content_shell
> which I thought used sandbox, so not sure what to make of that.
>
> To move forward with this bug fix and address the concerns, it sounds
like you
> would be satisfied if I add a single lock around the crypto() accessor?
(The
> downside is it means extra work in the non-webworker case).
>
> Alternately I could reduce the number of EnsureNSSInit() by putting them
only at
> key construction callsites, and for digest.
>
> https://codereview.chromium.org/278513004/

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

[eroman, 2014-05-12 23:33:00]

See my explanation above. I conducted tests using content_shell.
The pre-initialization is Chrome-Linux specific


On Mon, May 12, 2014 at 4:26 PM, Ryan Sleevi <rsleevi@chromium.org> wrote:

>
> On May 12, 2014 4:21 PM, <eroman@chromium.org> wrote:
> >
> > To answer the question of performance, on my z620 running Linux:
> >   - EnsureNSSInit() averaged 70ms
> >   - EnsureOpenSSLInit() averaged 1ms
> >
>
> This doesn't sound right. How were you testing? This cost is already
> supposed to be paid at renderer startup so that its a noop by the time
> WebCrypto calls it.
>
> > I didn't measure memory cost, but certainly not zero-cost either. So I
> think
> > these should be kept lazy when possible rather than process
> initialization.
> > Thanks for pointing out that for Linux, Chrome initializes NSS before
> engaging
> > sandbox. Interestingly the tests using NSS are being run from
> content_shell
> > which I thought used sandbox, so not sure what to make of that.
> >
> > To move forward with this bug fix and address the concerns, it sounds
> like you
> > would be satisfied if I add a single lock around the crypto() accessor?
> (The
> > downside is it means extra work in the non-webworker case).
> >
> > Alternately I could reduce the number of EnsureNSSInit() by putting them
> only at
> > key construction callsites, and for digest.
> >
> > https://codereview.chromium.org/278513004/
>

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

[Ryan Sleevi, 2014-05-12 23:40:52]

Sorry, I suppose the question wasn't clear. Your response is still lacking
a bit of information, or at least is unclear.

1) What platforms were you testing on.
  - From your response, I suppose you were only testing on Linux, but
that's not clear.
2) content_shell engages the sandbox, but does not engage the warmup.
  - jam@ pointed out that the warmup lives in the ChromeRenderProcessObserver
- specifically
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch...

So it's not clear that your observed times reflect what users' will see,
which is why I wanted to highlight the disparity and request more
information.

jam@ has already highlighted the need to (in a separate CL) move the
sandbox init into something clearer. I don't have a good answer for
"where", so that's all the more reason to keep that as a separate CL.

I'd ask for more information, specifically:
1) What's the cost on Mac/Windows
  - Theory: I expect it to be rather lower, since it's not dlopen()-ing any
libraries off disk like it is on Linux
2) What's the cost on Linux when the sandbox is already warmed (eg: how
it's actually used)
  - Theory: I expect it to be virtually a no-op, since it's already
dlopen()'d all the necessary bits

That is, in both cases, I expect the lock is an added bit of unnecessary
complexity, and that the time you measured (70ms) is an artifact that only
shows up in testing, and would disappear once the NSS pre-init was moved
from the //chrome layer into the //content layer, since now //content
renderers have an NSS dependency.


On Mon, May 12, 2014 at 4:32 PM, Eric Roman <eroman@chromium.org> wrote:

> See my explanation above. I conducted tests using content_shell.
> The pre-initialization is Chrome-Linux specific
>
>
> On Mon, May 12, 2014 at 4:26 PM, Ryan Sleevi <rsleevi@chromium.org> wrote:
>
>>
>> On May 12, 2014 4:21 PM, <eroman@chromium.org> wrote:
>> >
>> > To answer the question of performance, on my z620 running Linux:
>> >   - EnsureNSSInit() averaged 70ms
>> >   - EnsureOpenSSLInit() averaged 1ms
>> >
>>
>> This doesn't sound right. How were you testing? This cost is already
>> supposed to be paid at renderer startup so that its a noop by the time
>> WebCrypto calls it.
>>
>> > I didn't measure memory cost, but certainly not zero-cost either. So I
>> think
>> > these should be kept lazy when possible rather than process
>> initialization.
>> > Thanks for pointing out that for Linux, Chrome initializes NSS before
>> engaging
>> > sandbox. Interestingly the tests using NSS are being run from
>> content_shell
>> > which I thought used sandbox, so not sure what to make of that.
>> >
>> > To move forward with this bug fix and address the concerns, it sounds
>> like you
>> > would be satisfied if I add a single lock around the crypto() accessor?
>> (The
>> > downside is it means extra work in the non-webworker case).
>> >
>> > Alternately I could reduce the number of EnsureNSSInit() by putting
>> them only at
>> > key construction callsites, and for digest.
>> >
>> > https://codereview.chromium.org/278513004/
>>
>
>

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

[eroman, 2014-05-13 00:08:41]

Ryan, I can come back with numbers for other platforms, but I am not sure I
understand your proposal.

Are you suggesting that WebCryptoImpl should be eagerly initialized and
then call EnsureNSSInit()/EnsureOpenSSLInit() in its constructor, relying
on the fact that the crypto system has already been initialized and hence
low cost?

I don't agree with that approach. It would be adding more bloat by having
people pay for a feature they may not used.
Currently NSS is not initialized at renderer startup. Well, with the
exception that it is initialized early for Linux, to work around sandbox
restrictions. But certainly not universally on all platforms. Similarly
OpenSSL is not being initialized at process startup, so forcing it for
WebCrypto would be a regression. (This observation based on static code
analysis, will run some tests in Chrome to confirm).

While performance numbers are interesting, they don't express the total
cost. There is also memory to be considered.

Fundamentally since this work can be done lazily (and currently is), I am
of the opinion that it should remain lazy, or we risk bloating Chrome
further.
Just because the web platform includes WebCrypto/WebRTC doesn't mean that
ever web page will be using it.

Apologies if I am misunderstanding the suggestion.


On Mon, May 12, 2014 at 4:40 PM, Ryan Sleevi <rsleevi@chromium.org> wrote:

> Sorry, I suppose the question wasn't clear. Your response is still lacking
> a bit of information, or at least is unclear.
>
> 1) What platforms were you testing on.
>   - From your response, I suppose you were only testing on Linux, but
> that's not clear.
> 2) content_shell engages the sandbox, but does not engage the warmup.
>   - jam@ pointed out that the warmup lives in the ChromeRenderProcessObserver
> - specifically
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch...
>
> So it's not clear that your observed times reflect what users' will see,
> which is why I wanted to highlight the disparity and request more
> information.
>
> jam@ has already highlighted the need to (in a separate CL) move the
> sandbox init into something clearer. I don't have a good answer for
> "where", so that's all the more reason to keep that as a separate CL.
>
> I'd ask for more information, specifically:
> 1) What's the cost on Mac/Windows
>   - Theory: I expect it to be rather lower, since it's not dlopen()-ing
> any libraries off disk like it is on Linux
> 2) What's the cost on Linux when the sandbox is already warmed (eg: how
> it's actually used)
>   - Theory: I expect it to be virtually a no-op, since it's already
> dlopen()'d all the necessary bits
>
> That is, in both cases, I expect the lock is an added bit of unnecessary
> complexity, and that the time you measured (70ms) is an artifact that only
> shows up in testing, and would disappear once the NSS pre-init was moved
> from the //chrome layer into the //content layer, since now //content
> renderers have an NSS dependency.
>
>
> On Mon, May 12, 2014 at 4:32 PM, Eric Roman <eroman@chromium.org> wrote:
>
>> See my explanation above. I conducted tests using content_shell.
>> The pre-initialization is Chrome-Linux specific
>>
>>
>> On Mon, May 12, 2014 at 4:26 PM, Ryan Sleevi <rsleevi@chromium.org>wrote:
>>
>>>
>>> On May 12, 2014 4:21 PM, <eroman@chromium.org> wrote:
>>> >
>>> > To answer the question of performance, on my z620 running Linux:
>>> >   - EnsureNSSInit() averaged 70ms
>>> >   - EnsureOpenSSLInit() averaged 1ms
>>> >
>>>
>>> This doesn't sound right. How were you testing? This cost is already
>>> supposed to be paid at renderer startup so that its a noop by the time
>>> WebCrypto calls it.
>>>
>>> > I didn't measure memory cost, but certainly not zero-cost either. So I
>>> think
>>> > these should be kept lazy when possible rather than process
>>> initialization.
>>> > Thanks for pointing out that for Linux, Chrome initializes NSS before
>>> engaging
>>> > sandbox. Interestingly the tests using NSS are being run from
>>> content_shell
>>> > which I thought used sandbox, so not sure what to make of that.
>>> >
>>> > To move forward with this bug fix and address the concerns, it sounds
>>> like you
>>> > would be satisfied if I add a single lock around the crypto()
>>> accessor? (The
>>> > downside is it means extra work in the non-webworker case).
>>> >
>>> > Alternately I could reduce the number of EnsureNSSInit() by putting
>>> them only at
>>> > key construction callsites, and for digest.
>>> >
>>> > https://codereview.chromium.org/278513004/
>>>
>>
>>
>

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

[Ryan Sleevi, 2014-05-13 00:16:15]

On Mon, May 12, 2014 at 5:08 PM, Eric Roman <eroman@chromium.org> wrote:

> Ryan, I can come back with numbers for other platforms, but I am not sure
> I understand your proposal.
>
> Are you suggesting that WebCryptoImpl should be eagerly initialized and
> then call EnsureNSSInit()/EnsureOpenSSLInit() in its constructor, relying
> on the fact that the crypto system has already been initialized and hence
> low cost?
>
> I don't agree with that approach. It would be adding more bloat by having
> people pay for a feature they may not used.
> Currently NSS is not initialized at renderer startup. Well, with the
> exception that it is initialized early for Linux, to work around sandbox
> restrictions. But certainly not universally on all platforms. Similarly
> OpenSSL is not being initialized at process startup, so forcing it for
> WebCrypto would be a regression. (This observation based on static code
> analysis, will run some tests in Chrome to confirm).
>
> While performance numbers are interesting, they don't express the total
> cost. There is also memory to be considered.
>

Eric,

You've changed the topic of discussion here, so it's no surprise we
disagree. I was merely providing an explanation for you that your metric
used to justify lazy init - CPU time - is a false metric.

If we want to talk memory usage, then we certainly can do that - but let's
get numbers for that if we're going to talk relative costs.

However, independent of memory usage, it still seems to stand to reason
that you can be vastly more efficient, and without utilizing a lock.

When ::crypto() is accessed, simply call the Init function - always. You
know at the type that ::crypto() is called, the platform expects to perform
an operation (otherwise, you wouldn't have been lazy-init'ing there), and
you now avoid the need to liberally sprinkle Init calls through each of the
functions, which jam@ correctly points out makes it very easy to have
mistakes that would not be detected (especially by tests that may exercise
different code paths).

If that's not acceptable, then I think the best next step is to take a step
back and make sure that we're on the same page, because changing
requirements (CPU vs memory) doesn't quite help find good solutions.


>
> Fundamentally since this work can be done lazily (and currently is), I am
> of the opinion that it should remain lazy, or we risk bloating Chrome
> further.
> Just because the web platform includes WebCrypto/WebRTC doesn't mean that
> ever web page will be using it.
>

> Apologies if I am misunderstanding the suggestion.
>
>
> On Mon, May 12, 2014 at 4:40 PM, Ryan Sleevi <rsleevi@chromium.org> wrote:
>
>> Sorry, I suppose the question wasn't clear. Your response is still
>> lacking a bit of information, or at least is unclear.
>>
>> 1) What platforms were you testing on.
>>   - From your response, I suppose you were only testing on Linux, but
>> that's not clear.
>> 2) content_shell engages the sandbox, but does not engage the warmup.
>>   - jam@ pointed out that the warmup lives in the
ChromeRenderProcessObserver
>> - specifically
>>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch...
>>
>> So it's not clear that your observed times reflect what users' will see,
>> which is why I wanted to highlight the disparity and request more
>> information.
>>
>> jam@ has already highlighted the need to (in a separate CL) move the
>> sandbox init into something clearer. I don't have a good answer for
>> "where", so that's all the more reason to keep that as a separate CL.
>>
>> I'd ask for more information, specifically:
>> 1) What's the cost on Mac/Windows
>>   - Theory: I expect it to be rather lower, since it's not dlopen()-ing
>> any libraries off disk like it is on Linux
>> 2) What's the cost on Linux when the sandbox is already warmed (eg: how
>> it's actually used)
>>   - Theory: I expect it to be virtually a no-op, since it's already
>> dlopen()'d all the necessary bits
>>
>> That is, in both cases, I expect the lock is an added bit of unnecessary
>> complexity, and that the time you measured (70ms) is an artifact that only
>> shows up in testing, and would disappear once the NSS pre-init was moved
>> from the //chrome layer into the //content layer, since now //content
>> renderers have an NSS dependency.
>>
>>
>> On Mon, May 12, 2014 at 4:32 PM, Eric Roman <eroman@chromium.org> wrote:
>>
>>> See my explanation above. I conducted tests using content_shell.
>>> The pre-initialization is Chrome-Linux specific
>>>
>>>
>>> On Mon, May 12, 2014 at 4:26 PM, Ryan Sleevi <rsleevi@chromium.org>wrote:
>>>
>>>>
>>>> On May 12, 2014 4:21 PM, <eroman@chromium.org> wrote:
>>>> >
>>>> > To answer the question of performance, on my z620 running Linux:
>>>> >   - EnsureNSSInit() averaged 70ms
>>>> >   - EnsureOpenSSLInit() averaged 1ms
>>>> >
>>>>
>>>> This doesn't sound right. How were you testing? This cost is already
>>>> supposed to be paid at renderer startup so that its a noop by the time
>>>> WebCrypto calls it.
>>>>
>>>> > I didn't measure memory cost, but certainly not zero-cost either. So
>>>> I think
>>>> > these should be kept lazy when possible rather than process
>>>> initialization.
>>>> > Thanks for pointing out that for Linux, Chrome initializes NSS before
>>>> engaging
>>>> > sandbox. Interestingly the tests using NSS are being run from
>>>> content_shell
>>>> > which I thought used sandbox, so not sure what to make of that.
>>>> >
>>>> > To move forward with this bug fix and address the concerns, it sounds
>>>> like you
>>>> > would be satisfied if I add a single lock around the crypto()
>>>> accessor? (The
>>>> > downside is it means extra work in the non-webworker case).
>>>> >
>>>> > Alternately I could reduce the number of EnsureNSSInit() by putting
>>>> them only at
>>>> > key construction callsites, and for digest.
>>>> >
>>>> > https://codereview.chromium.org/278513004/
>>>>
>>>
>>>
>>
>

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

[eroman, 2014-05-13 00:33:27]

PTAL: Moved the initialization into BlinkPlatformImpl::crypto()

[Ryan Sleevi, 2014-05-13 05:47:19]

lgtm

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

https://codereview.chromium.org/278513004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.h:25: // from concurrent threads.
lazy vs one-time is an artifact of how it's used, not what it does. The original
comment was clearer in this respect

// Do one-time initialization. It is safe to call this multiple times and from
concurrent threads.

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

https://codereview.chromium.org/278513004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_impl.h:26: void EnsureInit();
why a member instead of a static? Given that it's not virtual, it's not exactly
a swappable implementation.

[jam, 2014-05-13 16:39:26]

lgtm

[eroman, 2014-05-13 18:25:07]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-05-13 18:26:33]

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

[eroman, 2014-05-13 18:27:15]

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

https://codereview.chromium.org/278513004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.h:25: // from concurrent threads.
On 2014/05/13 05:47:20, Ryan Sleevi wrote:
> lazy vs one-time is an artifact of how it's used, not what it does. The
original
> comment was clearer in this respect
> 
> // Do one-time initialization. It is safe to call this multiple times and from
> concurrent threads.

Changed back to original comment, but with a mention that it can be called
concurrently.

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

https://codereview.chromium.org/278513004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_impl.h:26: void EnsureInit();
On 2014/05/13 05:47:20, Ryan Sleevi wrote:
> why a member instead of a static? Given that it's not virtual, it's not
exactly
> a swappable implementation.

Good idea, done.

[commit-bot: I haz the power, 2014-05-13 21:40:22]

Change committed as 270215