Remove dependency of SSLPlatformKeyTaskRunner on single-threaded SequencedWorkerPool.

net/ssl/ssl_platform_key_task_runner.h

Index: net/ssl/ssl_platform_key_task_runner.h
diff --git a/net/ssl/ssl_platform_key_task_runner.h b/net/ssl/ssl_platform_key_task_runner.h
index 0c9b124f1fa3f891ae15f6f0c38d30aee0e45b85..7abf3241cb3fcceb2f4fdfdd373cfe4969fa6bca 100644
--- a/net/ssl/ssl_platform_key_task_runner.h
+++ b/net/ssl/ssl_platform_key_task_runner.h
@@ -6,28 +6,33 @@
 #define NET_SSL_SSL_PLATFORM_KEY_TASK_RUNNER_H_
 
 #include "base/macros.h"
-#include "base/threading/sequenced_worker_pool.h"
+#include "base/memory/ref_counted.h"
+#include "base/threading/thread.h"
+
+namespace {
+class SequencedTaskRunner;
+}
 
 namespace net {
 
-// Serialize all the private key operations on a single background
-// thread to avoid problems with buggy smartcards.
+// Serialize all the private key operations on a single background thread to
+// avoid problems with buggy smartcards. The exposed SingleThreadTaskRunner has
+// CONTINUE_ON_SHUTDOWN semantics.

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

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?

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

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).
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.

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

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.

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

Makes sense, thanks

 class SSLPlatformKeyTaskRunner {
  public:
   SSLPlatformKeyTaskRunner();
-  ~SSLPlatformKeyTaskRunner();
 
-  scoped_refptr<base::SequencedTaskRunner> task_runner();
+  // Ensure the instance is leaked to enforce CONTINUE_ON_SHUTDOWN semantics.
+  ~SSLPlatformKeyTaskRunner() = delete;
+
+  static scoped_refptr<base::SingleThreadTaskRunner> Get();

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

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.

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

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).
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.

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

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.
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)
"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.
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.
Hopefully explained the reasoning why, but yes, let's keep it the same.

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

Done.

 
  private:
-  scoped_refptr<base::SequencedWorkerPool> worker_pool_;
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  base::Thread worker_thread_;

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

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]

Yep, runs fine in a Debug build on Win10 corp:
 - startup
 - navigated https pages
 - net speed test for stress
 - clean shutdown
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).
Got it, manual test is clean and all bots are also happy.

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

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]

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]

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.

 
   DISALLOW_COPY_AND_ASSIGN(SSLPlatformKeyTaskRunner);
 };
 
-scoped_refptr<base::SequencedTaskRunner> GetSSLPlatformKeyTaskRunner();
-
 }  // namespace net
 
 #endif  // NET_SSL_SSL_PLATFORM_KEY_TASK_RUNNER_H_