Remove dependency of SSLPlatformKeyTaskRunner on single-threaded SequencedWorkerPool.

Remove dependency of SSLPlatformKeyTaskRunner on single-threaded SequencedWorkerPool.

Use a base::Thread bound to a LazyInstance (so it's never joined) instead,
keeping CONTINUE_ON_SHUTDOWN semantics.

Make the API explicitly use SingleThreadTaskRunner for both SSLPlatformKeyTaskRunner
and ThreadedSSLPrivateKey as they appear to have a hard requirement on this being
the case: https://codereview.chromium.org/2077413009/#msg16

Explicitly document that the exposed SingleThreadTaskRunner has CONTINUE_ON_SHUTDOWN
semantics as this is also a critical requirement.

BUG=622400
Committed: https://crrev.com/ed5ccb96b02293af2f9daa6f3c21b757f843304e
Cr-Commit-Position: refs/heads/master@{#409668}

[gab, 2016-06-28 21:09:35]

Patchset #1 (id:1) has been deleted

[gab, 2016-06-28 21:30:45]

Description was changed from

==========
Remove dependency of SSLPlatformKeyTaskRunner on single-threaded
SequencedWorkerPool.

BUG=622400
==========

to

==========
Remove dependency of SSLPlatformKeyTaskRunner on single-threaded
SequencedWorkerPool.

Make the API explicitly use SingleThreadTaskRunner for both
SSLPlatformKeyTaskRunner 
and ThreadedSSLPrivateKey as they appear to have a hard requirement on this
being
the case: https://codereview.chromium.org/2077413009/#msg16

Explicitly document that the exposed SingleThreadTaskRunner has
CONTINUE_ON_SHUTDOWN
semantics as this is also a critical requirement.

BUG=622400
==========

[gab, 2016-06-29 18:26:34]

Description was changed from

==========
Remove dependency of SSLPlatformKeyTaskRunner on single-threaded
SequencedWorkerPool.

Make the API explicitly use SingleThreadTaskRunner for both
SSLPlatformKeyTaskRunner 
and ThreadedSSLPrivateKey as they appear to have a hard requirement on this
being
the case: https://codereview.chromium.org/2077413009/#msg16

Explicitly document that the exposed SingleThreadTaskRunner has
CONTINUE_ON_SHUTDOWN
semantics as this is also a critical requirement.

BUG=622400
==========

to

==========
Remove dependency of SSLPlatformKeyTaskRunner on single-threaded
SequencedWorkerPool.

Use a base::Thread bound to a LazyInstance (so it's never joined) instead,
keeping CONTINUE_ON_SHUTDOWN semantics.

Make the API explicitly use SingleThreadTaskRunner for both
SSLPlatformKeyTaskRunner 
and ThreadedSSLPrivateKey as they appear to have a hard requirement on this
being
the case: https://codereview.chromium.org/2077413009/#msg16

Explicitly document that the exposed SingleThreadTaskRunner has
CONTINUE_ON_SHUTDOWN
semantics as this is also a critical requirement.

BUG=622400
==========

[gab, 2016-06-29 18:28:10]

gab@chromium.org changed reviewers:
+ rsleevi@chromium.org

[gab, 2016-06-29 18:28:10]

Ryan PTAL, I'm pretty sure this is equivalent. A MessageLoop with TYPE_DEFAULT
pretty much only pumps tasks the same way a single-threaded single-taskrunner
SequencedWorkerPool would and the LazyInstance has it never destroy the thread
just like the previous SequencedWorkerPool LazyInstance.

[Ryan Sleevi, 2016-06-29 18:29:38]

I won't have time to review this this week, because it'll take time and testing.

[gab, 2016-06-29 19:48:58]

On 2016/06/29 18:29:38, Ryan Sleevi wrote:
> I won't have time to review this this week, because it'll take time and
testing.

Ok an ETA for next week works for me, thanks.

[Ryan Sleevi, 2016-07-01 01:09:30]

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_task_runner.h (right):

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:20: // CONTINUE_ON_SHUTDOWN semantics.
Simply from reading this header, it's unclear what "CONTINUE_ON_SHUTDOWN
semantics" are or imply.

If I codesearch for it, I get results from SequencedWorkedPool and TaskTracker
and TaskScheduler - but I'm not sure what I'm supposed to be using/assuming
here.

Rather than incorporate-by-relation, could you just explicitly document what
those guarantees are?

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:28: static
scoped_refptr<base::SingleThreadTaskRunner> Get();
DESIGN: From a design perspective, it should be an anti-pattern to have a public
constructor and an implicit singleton getter.

Going this route (that is, making the singleton-ness an API contract, which I
believe is an anti-pattern for design), we should make the constructor private.

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:31: base::Thread worker_thread_;
Have you tested and run Chrome with this? With DCHECKs enabled?

As indicated on email, I tried this approach in the past - and a number of
crashes were the result. Just curious whether you've done cursory testing yet,
or if you're requesting I do that.

(I mention this because client cert tests are platform-interaction tests that
affect global system state, so can't easily be unittests; simply running Chrome
manually on a Google-enrolled device often triggers a number of bugs quickly,
during browsing or shutdown)

[gab, 2016-07-05 15:40:42]

Thanks, replies below.

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_task_runner.h (right):

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:20: // CONTINUE_ON_SHUTDOWN semantics.
On 2016/07/01 01:09:30, Ryan Sleevi wrote:
> Simply from reading this header, it's unclear what "CONTINUE_ON_SHUTDOWN
> semantics" are or imply.
> 
> If I codesearch for it, I get results from SequencedWorkedPool and TaskTracker
> and TaskScheduler - but I'm not sure what I'm supposed to be using/assuming
> here.

The only definitions are on SequencedWorkerPool and TaskTraits and the
TaskTraits definition is an exact copy of the SequencedWorkerPool definition (on
the road to TaskScheduler replacing it).

> 
> Rather than incorporate-by-relation, could you just explicitly document what
> those guarantees are?

I made it more specific by using TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN.
These are //base idioms that chrome devs should be familiar with (or easily find
the definition of under TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) IMO.

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:28: static
scoped_refptr<base::SingleThreadTaskRunner> Get();
On 2016/07/01 01:09:30, Ryan Sleevi wrote:
> DESIGN: From a design perspective, it should be an anti-pattern to have a
public
> constructor and an implicit singleton getter.

I thought it was even weirder to have essentially that (with
GetSSLPlatformKeyTaskRunner()) but not as a static getter.

The constructor has to be public for the LazyInstance (and can't friend it
because "friend class base::LazyInstance<SSLPlatformKeyTaskRunner>::Leaky;"
can't be declared inside the very declaration of SSLPlatformKeyTaskRunner (or at
least I'm getting cryptic compile errors when adding that line).

> 
> Going this route (that is, making the singleton-ness an API contract, which I
> believe is an anti-pattern for design), we should make the constructor
private.

I'm happy to follow whichever design you prefer as owner here. I changed it
because I thought the static Get() highlighted better the fact that it was
backed by a LazyInstance and there really should only be one. I agree that the
public constructor is weird...

Is this API single-threaded? i.e. would a raw pointer do? Or do you really need
the thread-safe initialization of LazyInstance? With a raw pointer the
constructor could be private :-).

Anyways, happy to leave the old design in place if you prefer I was just trying
to augment documentation on it given it's an API achieving a very specific
purpose with very specific semantics.

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:31: base::Thread worker_thread_;
On 2016/07/01 01:09:30, Ryan Sleevi wrote:
> Have you tested and run Chrome with this? With DCHECKs enabled?
> 

Yep, runs fine in a Debug build on Win10 corp:
 - startup
 - navigated https pages
 - net speed test for stress
 - clean shutdown

> As indicated on email, I tried this approach in the past - and a number of
> crashes were the result. Just curious whether you've done cursory testing yet,
> or if you're requesting I do that.

I think the difference might be that the thread is held in a
LazyInstance::Leaky? This effectively gives it the same semantics as the
SequencedWorkerPool thread (plus a MessageLoop but minus the overhead of the
SequencedWorkerPool logic which is overkill for a single thread).

> 
> (I mention this because client cert tests are platform-interaction tests that
> affect global system state, so can't easily be unittests; simply running
Chrome
> manually on a Google-enrolled device often triggers a number of bugs quickly,
> during browsing or shutdown)

Got it, manual test is clean and all bots are also happy.

[Ryan Sleevi, 2016-07-06 18:57:07]

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_task_runner.h (right):

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:20: // CONTINUE_ON_SHUTDOWN semantics.
On 2016/07/05 15:40:42, gab wrote:
> I made it more specific by using TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN.
> These are //base idioms that chrome devs should be familiar with (or easily
find
> the definition of under TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) IMO.

While I agree people should be familiar with //base idioms, I don't believe that
saying "CONTINUE_ON_SHUTDOWN" is itself a suitable reference to a //base idiom
:)

Your improved comment meets my concern, but I did want to make sure it was clear
why I was concerned.

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:28: static
scoped_refptr<base::SingleThreadTaskRunner> Get();
On 2016/07/05 15:40:42, gab wrote:
> I thought it was even weirder to have essentially that (with
> GetSSLPlatformKeyTaskRunner()) but not as a static getter.

Why? The two are very different statements about API contracts.

To be clear, Foo::Get() makes a strong requirement in the API design of Foo that
it's always a Singleton-y object. Having a naked getter that returns Foo, and
which Foo is constructible, is a very different API contract - it says Foo can
be stack or heap allocated, could be a singleton or could be multiple instances,
etc - none of that is intrinsic to the design of Foo. The properties of there
only being "one Foo" is something in the GetFoo() method - and that can be
independent, there could be "one Foo" to many foo, etc.

I'm a very strong believer that the sort of implicit-in-class-design-Singleton
(aka the ::Get() ) method generally leads to worse design.

> 
> The constructor has to be public for the LazyInstance (and can't friend it
> because "friend class base::LazyInstance<SSLPlatformKeyTaskRunner>::Leaky;"
> can't be declared inside the very declaration of SSLPlatformKeyTaskRunner (or
at
> least I'm getting cryptic compile errors when adding that line).

That's because you don't friend ::Leaky. You friend the LazyInstance *traits*

friend struct base::DefaultLazyInstanceTraits<ClassName>

(But still, I'm not a fan of this because it still forces the Singleton-ness
into the class contract)

> I'm happy to follow whichever design you prefer as owner here. I changed it
> because I thought the static Get() highlighted better the fact that it was
> backed by a LazyInstance and there really should only be one. 

"And there really should be only one" is the design complaint I have - baking
those sorts of design assumptions - anywhere in Chrome - in tends to be a recipe
for pain.

> Is this API single-threaded? i.e. would a raw pointer do? Or do you really
need
> the thread-safe initialization of LazyInstance? With a raw pointer the
> constructor could be private :-).

I'm extremely uncomfortable that this was even a suggestion, because it suggests
playing even faster-and-looser with API contracts, rather than making things
defined and predictable.

> Anyways, happy to leave the old design in place if you prefer I was just
trying
> to augment documentation on it given it's an API achieving a very specific
> purpose with very specific semantics.

Hopefully explained the reasoning why, but yes, let's keep it the same.

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:31: base::Thread worker_thread_;
On 2016/07/05 15:40:42, gab wrote:
> > As indicated on email, I tried this approach in the past - and a number of
> > crashes were the result. Just curious whether you've done cursory testing
yet,
> > or if you're requesting I do that.
> 
> I think the difference might be that the thread is held in a
> LazyInstance::Leaky? This effectively gives it the same semantics as the
> SequencedWorkerPool thread (plus a MessageLoop but minus the overhead of the
> SequencedWorkerPool logic which is overkill for a single thread).

Well, no, the ::Leaky doesn't address the concerns.

There were a number of structures in //base tied to an AtExit manager and which
were used by MessageLoop/MessagePump, including at the time, logging.

All of these will spectacularly crash on shutdown if this thread keeps running,
because the main thread's AtExit manager is gone. Even the semantics for
MessageLoop & SequencedTaskRunner were, at the time, coupled to AtExit and
shutdown semantics.

The SequencedWorkerPool also had some dependencies (For shutdown cleanup), but I
resolved the obvious ones that prevented this usage, and because it avoided
MessageLoop entirely, it avoided the AtExit dependency.

[gab, 2016-07-07 23:51:34]

Thanks, all addressed, PTAL.

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_task_runner.h (right):

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:20: // CONTINUE_ON_SHUTDOWN semantics.
On 2016/07/06 18:57:07, Ryan Sleevi (extremely slow) wrote:
> On 2016/07/05 15:40:42, gab wrote:
> > I made it more specific by using TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN.
> > These are //base idioms that chrome devs should be familiar with (or easily
> find
> > the definition of under TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) IMO.
> 
> While I agree people should be familiar with //base idioms, I don't believe
that
> saying "CONTINUE_ON_SHUTDOWN" is itself a suitable reference to a //base idiom
> :)
> 
> Your improved comment meets my concern, but I did want to make sure it was
clear
> why I was concerned.

Makes sense, thanks

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:28: static
scoped_refptr<base::SingleThreadTaskRunner> Get();
On 2016/07/06 18:57:06, Ryan Sleevi (extremely slow) wrote:
> On 2016/07/05 15:40:42, gab wrote:
> > I thought it was even weirder to have essentially that (with
> > GetSSLPlatformKeyTaskRunner()) but not as a static getter.
> 
> Why? The two are very different statements about API contracts.
> 
> To be clear, Foo::Get() makes a strong requirement in the API design of Foo
that
> it's always a Singleton-y object. Having a naked getter that returns Foo, and
> which Foo is constructible, is a very different API contract - it says Foo can
> be stack or heap allocated, could be a singleton or could be multiple
instances,
> etc - none of that is intrinsic to the design of Foo. The properties of there
> only being "one Foo" is something in the GetFoo() method - and that can be
> independent, there could be "one Foo" to many foo, etc.
> 
> I'm a very strong believer that the sort of implicit-in-class-design-Singleton
> (aka the ::Get() ) method generally leads to worse design.
> 
> > 
> > The constructor has to be public for the LazyInstance (and can't friend it
> > because "friend class base::LazyInstance<SSLPlatformKeyTaskRunner>::Leaky;"
> > can't be declared inside the very declaration of SSLPlatformKeyTaskRunner
(or
> at
> > least I'm getting cryptic compile errors when adding that line).
> 
> That's because you don't friend ::Leaky. You friend the LazyInstance *traits*
> 
> friend struct base::DefaultLazyInstanceTraits<ClassName>
> 
> (But still, I'm not a fan of this because it still forces the Singleton-ness
> into the class contract)
> 
> > I'm happy to follow whichever design you prefer as owner here. I changed it
> > because I thought the static Get() highlighted better the fact that it was
> > backed by a LazyInstance and there really should only be one. 
> 
> "And there really should be only one" is the design complaint I have - baking
> those sorts of design assumptions - anywhere in Chrome - in tends to be a
recipe
> for pain.
> 
> > Is this API single-threaded? i.e. would a raw pointer do? Or do you really
> need
> > the thread-safe initialization of LazyInstance? With a raw pointer the
> > constructor could be private :-).
> 
> I'm extremely uncomfortable that this was even a suggestion, because it
suggests
> playing even faster-and-looser with API contracts, rather than making things
> defined and predictable.
> 
> > Anyways, happy to leave the old design in place if you prefer I was just
> trying
> > to augment documentation on it given it's an API achieving a very specific
> > purpose with very specific semantics.
> 
> Hopefully explained the reasoning why, but yes, let's keep it the same.

Done.

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:31: base::Thread worker_thread_;
On 2016/07/06 18:57:06, Ryan Sleevi (extremely slow) wrote:
> On 2016/07/05 15:40:42, gab wrote:
> > > As indicated on email, I tried this approach in the past - and a number of
> > > crashes were the result. Just curious whether you've done cursory testing
> yet,
> > > or if you're requesting I do that.
> > 
> > I think the difference might be that the thread is held in a
> > LazyInstance::Leaky? This effectively gives it the same semantics as the
> > SequencedWorkerPool thread (plus a MessageLoop but minus the overhead of the
> > SequencedWorkerPool logic which is overkill for a single thread).
> 
> Well, no, the ::Leaky doesn't address the concerns.
> 
> There were a number of structures in //base tied to an AtExit manager and
which
> were used by MessageLoop/MessagePump, including at the time, logging.
> 
> All of these will spectacularly crash on shutdown if this thread keeps
running,
> because the main thread's AtExit manager is gone. Even the semantics for
> MessageLoop & SequencedTaskRunner were, at the time, coupled to AtExit and
> shutdown semantics.
> 
> The SequencedWorkerPool also had some dependencies (For shutdown cleanup), but
I
> resolved the obvious ones that prevented this usage, and because it avoided
> MessageLoop entirely, it avoided the AtExit dependency.

Ah got it, makes sense. Well there doesn't appear to be any of those anymore:
https://cs.chromium.org/search/?q=AtExitManager::Register.*%5C(&sq=package:ch...

I even tried "out\Debug\chrome --enable-logging --v=1" which ran happily through
loading HTTPS content and browser shutdown.

[gab, 2016-07-07 23:54:16]

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_task_runner.h (right):

https://codereview.chromium.org/2103883004/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:31: base::Thread worker_thread_;
On 2016/07/07 23:51:34, gab wrote:
> On 2016/07/06 18:57:06, Ryan Sleevi (extremely slow) wrote:
> > On 2016/07/05 15:40:42, gab wrote:
> > > > As indicated on email, I tried this approach in the past - and a number
of
> > > > crashes were the result. Just curious whether you've done cursory
testing
> > yet,
> > > > or if you're requesting I do that.
> > > 
> > > I think the difference might be that the thread is held in a
> > > LazyInstance::Leaky? This effectively gives it the same semantics as the
> > > SequencedWorkerPool thread (plus a MessageLoop but minus the overhead of
the
> > > SequencedWorkerPool logic which is overkill for a single thread).
> > 
> > Well, no, the ::Leaky doesn't address the concerns.
> > 
> > There were a number of structures in //base tied to an AtExit manager and
> which
> > were used by MessageLoop/MessagePump, including at the time, logging.
> > 
> > All of these will spectacularly crash on shutdown if this thread keeps
> running,
> > because the main thread's AtExit manager is gone. Even the semantics for
> > MessageLoop & SequencedTaskRunner were, at the time, coupled to AtExit and
> > shutdown semantics.
> > 
> > The SequencedWorkerPool also had some dependencies (For shutdown cleanup),
but
> I
> > resolved the obvious ones that prevented this usage, and because it avoided
> > MessageLoop entirely, it avoided the AtExit dependency.
> 
> Ah got it, makes sense. Well there doesn't appear to be any of those anymore:
>
https://cs.chromium.org/search/?q=AtExitManager::Register.*%5C(&sq=package:ch...
> 
> I even tried "out\Debug\chrome --enable-logging --v=1" which ran happily
through
> loading HTTPS content and browser shutdown.

On that note though, it would probably be better to explicitly make the Thread
non-joinable through a new base::Thread::Options as that would let APIs that
care not to run on a non-joinable thread (e.g. non-Leaky LazyInstance, etc.)
actually do their checks (right now this implementation wouldn't trigger their
checks I think if someone used something they shouldn't on this task runner).

I'm happy to do that if you agree.

[gab, 2016-07-07 23:57:40]

Patchset #5 (id:100001) has been deleted

[Ryan Sleevi, 2016-07-08 00:19:24]

On 2016/07/07 23:54:16, gab wrote:
> On that note though, it would probably be better to explicitly make the Thread
> non-joinable through a new base::Thread::Options as that would let APIs that
> care not to run on a non-joinable thread (e.g. non-Leaky LazyInstance, etc.)
> actually do their checks (right now this implementation wouldn't trigger their
> checks I think if someone used something they shouldn't on this task runner).
> 
> I'm happy to do that if you agree.

That super-SGTM. Were you thinking of this CL or a different/follow-up? Just
making sure I'm reviewing this when you're ready

[gab, 2016-07-08 00:30:31]

On 2016/07/08 00:19:24, Ryan Sleevi (extremely slow) wrote:
> On 2016/07/07 23:54:16, gab wrote:
> > On that note though, it would probably be better to explicitly make the
Thread
> > non-joinable through a new base::Thread::Options as that would let APIs that
> > care not to run on a non-joinable thread (e.g. non-Leaky LazyInstance, etc.)
> > actually do their checks (right now this implementation wouldn't trigger
their
> > checks I think if someone used something they shouldn't on this task
runner).
> > 
> > I'm happy to do that if you agree.
> 
> That super-SGTM. Were you thinking of this CL or a different/follow-up? Just
> making sure I'm reviewing this when you're ready

This CL, will update it tomorrow, thanks

[gab, 2016-07-12 03:30:22]

On 2016/07/08 00:30:31, gab wrote:
> On 2016/07/08 00:19:24, Ryan Sleevi (extremely slow) wrote:
> > On 2016/07/07 23:54:16, gab wrote:
> > > On that note though, it would probably be better to explicitly make the
> Thread
> > > non-joinable through a new base::Thread::Options as that would let APIs
that
> > > care not to run on a non-joinable thread (e.g. non-Leaky LazyInstance,
etc.)
> > > actually do their checks (right now this implementation wouldn't trigger
> their
> > > checks I think if someone used something they shouldn't on this task
> runner).
> > > 
> > > I'm happy to do that if you agree.
> > 
> > That super-SGTM. Were you thinking of this CL or a different/follow-up? Just
> > making sure I'm reviewing this when you're ready
> 
> This CL, will update it tomorrow, thanks

Update: this is happening (https://codereview.chromium.org/2135413003/) but as
part of grokking the existing base::Thread API/impl I ended up modernizing it
with safety checks, etc. (https://codereview.chromium.org/2145463002). Which is
unveiling that some consumers have been using the base::Thread API incorrectly
and triggering some of those checks so I'll to clean those up first, then land
those CLs, then update this one.

Will ping when ready for another review here (and, FYI, I'll be OOO for half of
this week so it might not be until next week).

[gab, 2016-07-22 16:49:57]

Patchset #5 (id:120001) has been deleted

[gab, 2016-07-26 19:30:25]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 19:30:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-07-26 19:32:12]

Patchset #5 (id:140001) has been deleted

[gab, 2016-07-26 19:33:11]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 19:33:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 21:05:25]

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

[commit-bot: I haz the power, 2016-07-26 21:05:26]

Dry run: This issue passed the CQ dry run.

[gab, 2016-07-26 21:36:15]

On 2016/07/12 03:30:22, gab wrote:
> On 2016/07/08 00:30:31, gab wrote:
> > On 2016/07/08 00:19:24, Ryan Sleevi (extremely slow) wrote:
> > > On 2016/07/07 23:54:16, gab wrote:
> > > > On that note though, it would probably be better to explicitly make the
> > Thread
> > > > non-joinable through a new base::Thread::Options as that would let APIs
> that
> > > > care not to run on a non-joinable thread (e.g. non-Leaky LazyInstance,
> etc.)
> > > > actually do their checks (right now this implementation wouldn't trigger
> > their
> > > > checks I think if someone used something they shouldn't on this task
> > runner).
> > > > 
> > > > I'm happy to do that if you agree.
> > > 
> > > That super-SGTM. Were you thinking of this CL or a different/follow-up?
Just
> > > making sure I'm reviewing this when you're ready
> > 
> > This CL, will update it tomorrow, thanks
> 
> Update: this is happening (https://codereview.chromium.org/2135413003/) but as
> part of grokking the existing base::Thread API/impl I ended up modernizing it
> with safety checks, etc. (https://codereview.chromium.org/2145463002). Which
is
> unveiling that some consumers have been using the base::Thread API incorrectly
> and triggering some of those checks so I'll to clean those up first, then land
> those CLs, then update this one.
> 
> Will ping when ready for another review here (and, FYI, I'll be OOO for half
of
> this week so it might not be until next week).

Alright, done :-). Took longer than expected with
https://codereview.chromium.org/2135413003/ which turned into
https://codereview.chromium.org/2145463002 which discovered
http://crbug.com/629139 but the world is now better off for it :-).

Oh and I also have https://codereview.chromium.org/2162053006/ and
https://codereview.chromium.org/2163023002/ in the pipeline to enable a few more
relevant checks when DCHECK_ALWAYS_ON (on top of when !NDEBUG as it is
currently) :-).

CQ is still happy, PTAnL, thanks!

[Ryan Sleevi, 2016-07-27 19:09:23]

LGTM % nits

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/ssl_platform_k...
File net/ssl/ssl_platform_key_task_runner.h (right):

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_task_runner.h:13: class SingleThreadTaskRunner;
BUG: wrong namespace; should be base, but you've got this unnamed.

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_task_runner.h:14: }
STYLE: }  // namespace base

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/threaded_ssl_p...
File net/ssl/threaded_ssl_private_key.h (right):

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/threaded_ssl_p...
net/ssl/threaded_ssl_private_key.h:21: class TaskRunner;
BUG: Need to update forward-decl

[gab, 2016-07-28 18:30:34]

Thanks, done. Will land after dependent CLs land.

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/ssl_platform_k...
File net/ssl/ssl_platform_key_task_runner.h (right):

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_task_runner.h:13: class SingleThreadTaskRunner;
On 2016/07/27 19:09:23, Ryan Sleevi (slow) wrote:
> BUG: wrong namespace; should be base, but you've got this unnamed.

Oops, good eyes, done.

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_task_runner.h:14: }
On 2016/07/27 19:09:23, Ryan Sleevi (slow) wrote:
> STYLE: }  // namespace base

I've been told (by danakj@) that for small fwd-decl blocks (< ~6) we typically
omit the closing comment.

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/threaded_ssl_p...
File net/ssl/threaded_ssl_private_key.h (right):

https://codereview.chromium.org/2103883004/diff/160001/net/ssl/threaded_ssl_p...
net/ssl/threaded_ssl_private_key.h:21: class TaskRunner;
On 2016/07/27 19:09:23, Ryan Sleevi (slow) wrote:
> BUG: Need to update forward-decl

Done.

[Ryan Sleevi, 2016-07-28 18:34:26]

Yeah, I've seen us either omit or include, but wanted to make sure either way it
was intentional. Sorry, I should have clarified it more as a STYLE NIT since I
was uncertain the "one true style" (the style guide and style in practice are
somewhat unclear)

So yes, still LGTM

[gab, 2016-08-03 20:00:59]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-08-03 20:01:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 20:57:27]

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

[commit-bot: I haz the power, 2016-08-03 20:57:28]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-08-03 22:55:02]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-08-03 22:56:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 23:46:10]

Description was changed from

==========
Remove dependency of SSLPlatformKeyTaskRunner on single-threaded
SequencedWorkerPool.

Use a base::Thread bound to a LazyInstance (so it's never joined) instead,
keeping CONTINUE_ON_SHUTDOWN semantics.

Make the API explicitly use SingleThreadTaskRunner for both
SSLPlatformKeyTaskRunner 
and ThreadedSSLPrivateKey as they appear to have a hard requirement on this
being
the case: https://codereview.chromium.org/2077413009/#msg16

Explicitly document that the exposed SingleThreadTaskRunner has
CONTINUE_ON_SHUTDOWN
semantics as this is also a critical requirement.

BUG=622400
==========

to

==========
Remove dependency of SSLPlatformKeyTaskRunner on single-threaded
SequencedWorkerPool.

Use a base::Thread bound to a LazyInstance (so it's never joined) instead,
keeping CONTINUE_ON_SHUTDOWN semantics.

Make the API explicitly use SingleThreadTaskRunner for both
SSLPlatformKeyTaskRunner 
and ThreadedSSLPrivateKey as they appear to have a hard requirement on this
being
the case: https://codereview.chromium.org/2077413009/#msg16

Explicitly document that the exposed SingleThreadTaskRunner has
CONTINUE_ON_SHUTDOWN
semantics as this is also a critical requirement.

BUG=622400
==========

[commit-bot: I haz the power, 2016-08-03 23:46:11]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-08-03 23:47:43]

Description was changed from

==========
Remove dependency of SSLPlatformKeyTaskRunner on single-threaded
SequencedWorkerPool.

Use a base::Thread bound to a LazyInstance (so it's never joined) instead,
keeping CONTINUE_ON_SHUTDOWN semantics.

Make the API explicitly use SingleThreadTaskRunner for both
SSLPlatformKeyTaskRunner 
and ThreadedSSLPrivateKey as they appear to have a hard requirement on this
being
the case: https://codereview.chromium.org/2077413009/#msg16

Explicitly document that the exposed SingleThreadTaskRunner has
CONTINUE_ON_SHUTDOWN
semantics as this is also a critical requirement.

BUG=622400
==========

to

==========
Remove dependency of SSLPlatformKeyTaskRunner on single-threaded
SequencedWorkerPool.

Use a base::Thread bound to a LazyInstance (so it's never joined) instead,
keeping CONTINUE_ON_SHUTDOWN semantics.

Make the API explicitly use SingleThreadTaskRunner for both
SSLPlatformKeyTaskRunner
and ThreadedSSLPrivateKey as they appear to have a hard requirement on this
being
the case: https://codereview.chromium.org/2077413009/#msg16

Explicitly document that the exposed SingleThreadTaskRunner has
CONTINUE_ON_SHUTDOWN
semantics as this is also a critical requirement.

BUG=622400

Committed: https://crrev.com/ed5ccb96b02293af2f9daa6f3c21b757f843304e
Cr-Commit-Position: refs/heads/master@{#409668}
==========

[commit-bot: I haz the power, 2016-08-03 23:47:44]

Patchset 6 (id:??) landed as
https://crrev.com/ed5ccb96b02293af2f9daa6f3c21b757f843304e
Cr-Commit-Position: refs/heads/master@{#409668}