Certificate Transparency: Wire the TreeStateTracker up.

Certificate Transparency: Wire the TreeStateTracker up.

This change puts into use the TreeStateTracker class by wiring it to:
* A global STHDistributor for receiving fresh STHs via the component
  updater.
* The per-profile CTVerifier for receiving notification of verified
  SCTs that should be checked for inclusion.

As discussed with sorin and rsleevi, the wiring of the STHSetComponentInstaller
is via a globally-accessible STHDistributor, created by the IOThread.

BUG=613501
Committed: https://crrev.com/3c2d643cded7d3c68510aa69b08a19b2ac465740
Cr-Commit-Position: refs/heads/master@{#397096}

[Eran Messeri, 2016-05-25 15:01:11]

Description was changed from

==========
Certificate Transparency: Wire the TreeStateTracker up.

This change puts into use the TreeStateTracker class by wiring it to:
* A global STHDistributor for receiving fresh STHs via the component
  updater.
* The per-profile CTVerifier for receiving notification of verified
  SCTs that should be checked for inclusion.

As discussed with sorin and rsleevi, the wiring of the STHSetComponentInstaller
is via a globally-accessible STHDistributor, created by the IOThread.

BUG=613501
==========

to

==========
Certificate Transparency: Wire the TreeStateTracker up.

This change puts into use the TreeStateTracker class by wiring it to:
* A global STHDistributor for receiving fresh STHs via the component
  updater.
* The per-profile CTVerifier for receiving notification of verified
  SCTs that should be checked for inclusion.

As discussed with sorin and rsleevi, the wiring of the STHSetComponentInstaller
is via a globally-accessible STHDistributor, created by the IOThread.

BUG=613501
==========

[Eran Messeri, 2016-05-25 15:01:12]

eranm@chromium.org changed reviewers:
+ rsleevi@chromium.org, sorin@chromium.org

[Eran Messeri, 2016-05-25 15:01:42]

Ryan, Sorin, PTAL.

[Sorin Jianu, 2016-05-25 16:17:48]

Thank you Eran. I sent a couple of questions to start with.

https://codereview.chromium.org/2014573002/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/sth_set_component_installer.h (right):

https://codereview.chromium.org/2014573002/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/sth_set_component_installer.h:75:
net::ct::STHObserver* sth_observer_;
Would it be possible to use a smart pointer? Same question for the constructor
parameter.

https://codereview.chromium.org/2014573002/diff/1/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/2014573002/diff/1/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:438: void RegisterSTHObserver(net::ct::STHObserver*
observer);
Can we do more smart pointers instead of raw pointers?

[Eran Messeri, 2016-05-25 16:26:18]

Sorin, PTAL.

https://codereview.chromium.org/2014573002/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/sth_set_component_installer.h (right):

https://codereview.chromium.org/2014573002/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/sth_set_component_installer.h:75:
net::ct::STHObserver* sth_observer_;
On 2016/05/25 16:17:48, Sorin Jianu wrote:
> Would it be possible to use a smart pointer? Same question for the constructor
> parameter.

The two smart pointers I could use here are:
unique_ptr: Means passing ownership into STHSetComponentInstallerTraits, which,
as we discussed earlier, would complicate wiring and lifecycle management of
everything that's registered on it.
scoped_refptr: Means reference-counting the STHObserver. But because a global
object is now used to access it I don't see much value.

https://codereview.chromium.org/2014573002/diff/1/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/2014573002/diff/1/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:438: void RegisterSTHObserver(net::ct::STHObserver*
observer);
On 2016/05/25 16:17:48, Sorin Jianu wrote:
> Can we do more smart pointers instead of raw pointers?

I couldn't see a way.
I don't think it is necessary because the ownership of the observer is not
passed into RegisterSTHObserver - it remains with whichever object created the
observer.
Per
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/TlL1D-Djta0...,
since ownership is not passed, I am passing a raw pointer.

In this particular case, each entity that registers an STH Observer (the
IOThread or ProfileIOData) is responsible for un-registering it before
destroying it, as the same object would be registered in a local context with
the CTVerifier object held by the same entity.

[Sorin Jianu, 2016-05-25 17:54:45]

lgtm

thank you Eran!

https://codereview.chromium.org/2014573002/diff/20001/chrome/browser/componen...
File chrome/browser/component_updater/sth_set_component_installer.h (right):

https://codereview.chromium.org/2014573002/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/sth_set_component_installer.h:75:
net::ct::STHObserver* sth_observer_;
A comment would be helpful to document the ownership of this object.

https://codereview.chromium.org/2014573002/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2014573002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:415: // empty.
As a casual code reader, I wonder how does the code enforces that the observer
list is empty. And what happens if the list is not empty when the execution flow
reaches this point.

[Eran Messeri, 2016-05-26 09:51:22]

https://codereview.chromium.org/2014573002/diff/20001/chrome/browser/componen...
File chrome/browser/component_updater/sth_set_component_installer.h (right):

https://codereview.chromium.org/2014573002/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/sth_set_component_installer.h:75:
net::ct::STHObserver* sth_observer_;
On 2016/05/25 17:54:45, Sorin Jianu wrote:
> A comment would be helpful to document the ownership of this object.

Done.

https://codereview.chromium.org/2014573002/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2014573002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:415: // empty.
On 2016/05/25 17:54:45, Sorin Jianu wrote:
> As a casual code reader, I wonder how does the code enforces that the observer
> list is empty. And what happens if the list is not empty when the execution
flow
> reaches this point.

My bad, this was broken off to https://codereview.chromium.org/2015503003/.
The STHDistributor has an ObserverList and one of the template parameters is a
boolean indicating whether to check if the list of observers is empty:
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...

In the other CL I'll add a comment indicating that.

[Ryan Sleevi, 2016-05-27 04:59:53]

LGTM

https://codereview.chromium.org/2014573002/diff/40001/chrome/browser/componen...
File chrome/browser/component_updater/sth_set_component_installer.cc (right):

https://codereview.chromium.org/2014573002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/sth_set_component_installer.cc:189:
DCHECK(distributor != nullptr);
DCHECK(distributor)

(Rely on boolean conversion)

https://codereview.chromium.org/2014573002/diff/40001/chrome/browser/net/sth_...
File chrome/browser/net/sth_distributor_provider.cc (right):

https://codereview.chromium.org/2014573002/diff/40001/chrome/browser/net/sth_...
chrome/browser/net/sth_distributor_provider.cc:23:
CHECK(global_sth_distributor.Get() != nullptr);
Remove the != nullptr

(Plus, it'd be .Get().get() if you kept it, but you shouldn't)

[Eran Messeri, 2016-05-27 08:43:54]

Thanks for the quick review, sending to a chrome/ OWNER for review.

https://codereview.chromium.org/2014573002/diff/40001/chrome/browser/componen...
File chrome/browser/component_updater/sth_set_component_installer.cc (right):

https://codereview.chromium.org/2014573002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/sth_set_component_installer.cc:189:
DCHECK(distributor != nullptr);
On 2016/05/27 04:59:53, Ryan Sleevi (OOO til 6-6) wrote:
> DCHECK(distributor)
> 
> (Rely on boolean conversion)

Done.

https://codereview.chromium.org/2014573002/diff/40001/chrome/browser/net/sth_...
File chrome/browser/net/sth_distributor_provider.cc (right):

https://codereview.chromium.org/2014573002/diff/40001/chrome/browser/net/sth_...
chrome/browser/net/sth_distributor_provider.cc:23:
CHECK(global_sth_distributor.Get() != nullptr);
On 2016/05/27 04:59:53, Ryan Sleevi (OOO til 6-6) wrote:
> Remove the != nullptr
> 
> (Plus, it'd be .Get().get() if you kept it, but you shouldn't)

Done.

[Eran Messeri, 2016-05-27 08:44:23]

eranm@chromium.org changed reviewers:
+ thestig@chromium.org

[Eran Messeri, 2016-05-27 08:45:58]

Lei, please review this change for wiring an IOThread code to Component Updater
code. 
Reasoning behind the chosen wiring method is in the bug
(https://bugs.chromium.org/p/chromium/issues/detail?id=613501).

[Lei Zhang, 2016-05-27 18:24:32]

lgtm

https://codereview.chromium.org/2014573002/diff/60001/chrome/browser/componen...
File chrome/browser/component_updater/sth_set_component_installer.cc (right):

https://codereview.chromium.org/2014573002/diff/60001/chrome/browser/componen...
chrome/browser/component_updater/sth_set_component_installer.cc:186:
net::ct::STHDistributor* distributor(
nit: Can you assign pointers with an equal sign? It just looks odd.

https://codereview.chromium.org/2014573002/diff/60001/chrome/browser/componen...
File chrome/browser/component_updater/sth_set_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2014573002/diff/60001/chrome/browser/componen...
chrome/browser/component_updater/sth_set_component_installer_unittest.cc:85: //
traits_ should be destroyed  before the observer_ as it holds a pointer
nits: Refer to variables as |foo|, is that a double space in the sentence?

[Eran Messeri, 2016-05-31 11:30:56]

Thanks all for the quick review, addressed Lei's comments, will submit assuming
the trybots are green.

https://codereview.chromium.org/2014573002/diff/60001/chrome/browser/componen...
File chrome/browser/component_updater/sth_set_component_installer.cc (right):

https://codereview.chromium.org/2014573002/diff/60001/chrome/browser/componen...
chrome/browser/component_updater/sth_set_component_installer.cc:186:
net::ct::STHDistributor* distributor(
On 2016/05/27 18:24:32, Lei Zhang wrote:
> nit: Can you assign pointers with an equal sign? It just looks odd.

Done.

https://codereview.chromium.org/2014573002/diff/60001/chrome/browser/componen...
File chrome/browser/component_updater/sth_set_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2014573002/diff/60001/chrome/browser/componen...
chrome/browser/component_updater/sth_set_component_installer_unittest.cc:85: //
traits_ should be destroyed  before the observer_ as it holds a pointer
On 2016/05/27 18:24:32, Lei Zhang wrote:
> nits: Refer to variables as |foo|, is that a double space in the sentence?

Done.

[Eran Messeri, 2016-05-31 14:04:37]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2016-05-31 14:04:37]

The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org,
rsleevi@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2014573002/#ps80001
(title: "Addressing Lei's comments")

[commit-bot: I haz the power, 2016-05-31 14:04:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2014573002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2014573002/80001

[commit-bot: I haz the power, 2016-05-31 16:59:35]

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

[commit-bot: I haz the power, 2016-05-31 16:59:36]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Eran Messeri, 2016-06-01 09:17:05]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2016-06-01 09:17:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2014573002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2014573002/80001

[commit-bot: I haz the power, 2016-06-01 10:17:40]

Description was changed from

==========
Certificate Transparency: Wire the TreeStateTracker up.

This change puts into use the TreeStateTracker class by wiring it to:
* A global STHDistributor for receiving fresh STHs via the component
  updater.
* The per-profile CTVerifier for receiving notification of verified
  SCTs that should be checked for inclusion.

As discussed with sorin and rsleevi, the wiring of the STHSetComponentInstaller
is via a globally-accessible STHDistributor, created by the IOThread.

BUG=613501
==========

to

==========
Certificate Transparency: Wire the TreeStateTracker up.

This change puts into use the TreeStateTracker class by wiring it to:
* A global STHDistributor for receiving fresh STHs via the component
  updater.
* The per-profile CTVerifier for receiving notification of verified
  SCTs that should be checked for inclusion.

As discussed with sorin and rsleevi, the wiring of the STHSetComponentInstaller
is via a globally-accessible STHDistributor, created by the IOThread.

BUG=613501
==========

[commit-bot: I haz the power, 2016-06-01 10:17:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-01 10:19:00]

Description was changed from

==========
Certificate Transparency: Wire the TreeStateTracker up.

This change puts into use the TreeStateTracker class by wiring it to:
* A global STHDistributor for receiving fresh STHs via the component
  updater.
* The per-profile CTVerifier for receiving notification of verified
  SCTs that should be checked for inclusion.

As discussed with sorin and rsleevi, the wiring of the STHSetComponentInstaller
is via a globally-accessible STHDistributor, created by the IOThread.

BUG=613501
==========

to

==========
Certificate Transparency: Wire the TreeStateTracker up.

This change puts into use the TreeStateTracker class by wiring it to:
* A global STHDistributor for receiving fresh STHs via the component
  updater.
* The per-profile CTVerifier for receiving notification of verified
  SCTs that should be checked for inclusion.

As discussed with sorin and rsleevi, the wiring of the STHSetComponentInstaller
is via a globally-accessible STHDistributor, created by the IOThread.

BUG=613501

Committed: https://crrev.com/3c2d643cded7d3c68510aa69b08a19b2ac465740
Cr-Commit-Position: refs/heads/master@{#397096}
==========

[commit-bot: I haz the power, 2016-06-01 10:19:01]

Patchset 5 (id:??) landed as
https://crrev.com/3c2d643cded7d3c68510aa69b08a19b2ac465740
Cr-Commit-Position: refs/heads/master@{#397096}