Certificate Transparency: Start tracking logs' state

chrome/browser/component_updater/sth_set_component_installer.cc

Index: chrome/browser/component_updater/sth_set_component_installer.cc
diff --git a/chrome/browser/component_updater/sth_set_component_installer.cc b/chrome/browser/component_updater/sth_set_component_installer.cc
index d26c8784b5498cdeacd1ec042a967b6a31e8b055..604b5303576eec9c265677c930ead722eaa9dc13 100644
--- a/chrome/browser/component_updater/sth_set_component_installer.cc
+++ b/chrome/browser/component_updater/sth_set_component_installer.cc
@@ -15,6 +15,8 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "base/version.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/io_thread.h"
 #include "components/component_updater/component_updater_paths.h"
 #include "components/safe_json/safe_json_parser.h"
 #include "content/public/browser/browser_thread.h"
@@ -185,11 +187,16 @@ void RegisterSTHSetComponent(ComponentUpdateService* cus,
                              const base::FilePath& user_data_dir) {
   DVLOG(1) << "Registering STH Set fetcher component.";
 
-  // TODO(eranm): The next step in auditing CT logs (crbug.com/506227) is to
-  // pass the distributor to the IOThread so it can be used in a per-profile
-  // context for checking inclusion of SCTs.
   scoped_ptr<net::ct::STHDistributor> distributor(
       new net::ct::STHDistributor());
+  // Register the distributor as the STHReporter for the IOThread (see
+  // crbug.com/506227 for details).
+  DCHECK(g_browser_process->io_thread());

[Ryan Sleevi, 2016/04/21 14:05:15]

This defeats the abstraction of lines 195-196 (content::BrowserThread::IO) and
inappropriately couples it.

If you want to DCHECK, then use GetMessageLoopProxyForThread and DCHECK. But
that still doesn't guarantee correct results - because you can get the object
even if there's no underlying backing thread.

If you want correct results, look at the return result of the PostTask.

[Eran Messeri, 2016/04/25 14:50:59]

Apologies, but I am not sure I understand this comment. It reads to me as if I
was (wrongly) checking that the function is called on the IO thread, but what I
was attempting to do is check that the IOThread object from the
g_browser_process global is not null.

Are you suggesting to completely remove this DCHECK, call PostTask (below) and
DCHECK on the result?

[Ryan Sleevi, 2016/04/27 23:05:58]

Yes.

The point of using content::BrowserThread::PostTask(content::BrowserThread::IO
... ) is that you're supposed to be decoupled from g_browser_process. The fact
that it exists is one of those necessary evils, but you should never be assuming
that, say, the content::BrowserThread::IO == g_browser_process->io_thread(),
because that's not the contract of content::BrowserThread::IO

If you want to know if the thread still exists (more aptly, if the object was
posted correctly), then just look at the result - which will return false if the
thread is gone/shutting down.

But it doesn't strike me as clear why you'd ever DCHECK() on this, or what the
"developer error" scenario would be (but here, perhaps, I'm not understanding)

+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&IOThread::RegisterSTHReporter,
+                 base::Unretained(g_browser_process->io_thread()),

[Ryan Sleevi, 2016/04/21 14:05:15]

g_browser_process has an incredibly high bar to justify its usage. What are you
trying to do?

[Eran Messeri, 2016/04/25 14:50:59]

I'm trying to share an STHDistributor instance between the class implementing
ComponentInstallerTraits and receiving the STH updates and the STHObserver
(which lives in the IOThread / ProfileIOData) that needs these STHs.

My original plan was to pass this instance into RegisterSTHSetComponent and
share it by passing it into the IOThread via BrowserProcessImpl, but sorin@
asked for a less invasive change (see
https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b...)
and suggested the use of the g_browser_process global to pass the object rather
than pass it into RegisterSTHSetComponent.

Since I'm creating the STHDistributor instance here, I have no way of getting it
to the IOThread/STHObserver other than through globals.

How do you suggest I get STHs out from this class to objects created on the
IOThread/ProfileIOData?

[Ryan Sleevi, 2016/04/27 23:05:58]

Having objects live on multiple threads - whether born on one and used on
another or, worse, used on both - is usually an anti-pattern.
Right, I'm not sure I understand sorin's objection, or that I'd agree with it.
Using globals is, as you note, not desirable. Especially g_browser_process
stuff.

Sorin, could you explain more your comments? Did I miss a reply to Eran's
request for more details?

[Sorin Jianu, 2016/05/05 00:32:03]

I have a minor concern regarding a coupling issue that occurs in the function
RegisterComponentsForUpdate. This is really minor in the big scheme of things
but we'd like to have it resolved favorably, within reason.

To understand the concern, let's take a look at the patchset 1, as this change
has been broken off into many pieces by now.

https://codereview.chromium.org/1845113003/diff/1/chrome/browser/chrome_brows...
line 1710: we can see that what was a nice, cohesive piece of code, now has some
dependencies on a STHDistributor. Also, note that something gets registered at
line 1721, and that registration is not encapsulated anymore inside the actual
registration of the STHSet component with the component updater, which happens
under conditional and runtime branches at line 499 in the same file. Please
noticed how the instance of STHDistributor is moved from call site to call site.
These are small kinks but they raise some concerns about coupling,
encapsulation, cohesion, and ownership of objects.

Then, moving down the execution path, at line 447, we see how that function is
given a parameter only to be used by the RegisterSTHSetComponent call below. We
also don't like the coupling of this function with net:: artifacts. We'd prefer
these implementation details to be encapsulated by the RegisterSTHSetComponent.

The function RegisterComponentsForUpdate needs some further refactoring. It's
not in the scope of this CL to clean this function up but if possible, we want
to avoid additional dependencies, to make it easier for us to move the code in
the future.

In my opinion, further refactoring of the code in this CL could consider the
following:
* is the ownership of sth_distributor correct. Currently, that object is owned
by the STHSetComponentInstallerTraits instance, but is that the right owner?
Could g_browser_process own that, like it already owns other component
dependencies? 
* can we use a singleton in the net::, as the CRLSet is using?

As a closing thought, we are hitting some sharp edge in the design of the Chrome
browser process instance. On one hand, we have components, that are related to
how files are installed on the disk, but these files need to come alive, be
instantiated, and hook with the rest of
the browser at the right time, so that can be functionally useful. Each
component has hacked its way in, either by using a global or a singleton to
connect with the rest of the browser at runtime.

We don't want to hold up this CL for pedantic reasons, especially since the code
that deals with installers and updaters is already messy to start with. If these
concerns are valid and they could be addressed reasonably, it would help the
sanity of the code.

+                 distributor.get()));
 
   scoped_ptr<ComponentInstallerTraits> traits(
       new STHSetComponentInstallerTraits(std::move(distributor)));