Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1853753003:
Certificate Transparency: New component for obtaining fresh STHs. (Closed)
|
Created:
4 years, 8 months ago by Eran Messeri Modified:
4 years, 8 months ago Reviewers:
waffles, sky, Ryan Sleevi, ramant (doing other things), Sorin Jianu, dzhioev (left Google) CC:
chromium-reviews, cbentzel+watch_chromium.org, dzhioev+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, Ryan Sleevi, davidben, certificate-transparency-chrome_googlegroups.com, Rob Percival Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency: New component for obtaining fresh STHs.
This component is for receiving and parsing Signed Tree Heads
collected by Google from Certificate Transparency logs and distributed
to Chrome clients.
It is the first step in checking for inclusion of CT log entries
from Chrome.
Design document: https://docs.google.com/document/d/1FP5J5Sfsg0OR9P4YT0q1dM02iavhi8ix1mZlZe_z-ls/view
BUG=506227
Committed: https://crrev.com/9e1935c26148f42c6fe453cd380207c45934b944
Cr-Commit-Position: refs/heads/master@{#385903}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Ready for review #
Total comments: 29
Patch Set 3 : Addressing review comments #Patch Set 4 : Fixing compilation errors on ChromeOS #
Total comments: 32
Patch Set 5 : Ryan's comments, STHDistributor moved #
Total comments: 10
Patch Set 6 : Address comments by sorin & waffles #Patch Set 7 : Address comments by sorin & waffles #
Total comments: 16
Patch Set 8 : Addressing review comments #Messages
Total messages: 28 (11 generated)
Description was changed from ========== Certificate Transparency: New component for obtaining fresh STHs. This component is for receiving and parsing Signed Tree Heads collected by Google from Certificate Transparency logs and distributed to Chrome clients. It is the first step in checking for inclusion of CT log entries from Chrome. BUG= ========== to ========== Certificate Transparency: New component for obtaining fresh STHs. This component is for receiving and parsing Signed Tree Heads collected by Google from Certificate Transparency logs and distributed to Chrome clients. It is the first step in checking for inclusion of CT log entries from Chrome. BUG= ==========
eranm@chromium.org changed reviewers: + davidben@chromium.org, waffles@chromium.org
David/Ryan & Joshua: Soliciting early feedback from you. Design document with the broader picture: https://docs.google.com/document/d/1FP5J5Sfsg0OR9P4YT0q1dM02iavhi8ix1mZlZe_z-...
waffles@chromium.org changed reviewers: + sorin@chromium.org
Adding Sorin for a second pair of eyes. https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:38: // The extension id is: aplidfpohcjpojgnkjpkibbkcghkogef We will use ojjgnpkioondelmggbekfhllhdaimnho https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:42: 0x33, 0x90, 0x1c, 0xe9, 0x44, 0x37, 0xc4, 0xa0, 0x2e, 0x02}; We will use e996dfa8eed34bc6614a57bb7308cd7e519bcc690841e1969f7cb173ef16800a. For your convenience: {0xe9, 0x96, 0xdf, 0xa8, 0xee, 0xd3, 0x4b, 0xc6, 0x61, 0x4a, 0x57, 0xbb, 0x73, 0x08, 0xcd, 0x7e, 0x51, 0x9b, 0xcc, 0x69, 0x08, 0x41, 0xe1, 0x96, 0x9f, 0x7c, 0xb1, 0x73, 0xef, 0x16, 0x80, 0x0a}; https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:49: VLOG(1) << "XXX: STHSetComponentInstallerTraits::c'tor"; These logging statements should be cleaned up prior to check-in. (I know you said you were just looking for early feedback, but I have trouble not writing the note.) https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:87: NOTREACHED(); Not sure about this. https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:152: safe_json::SafeJsonParser::Parse( Hm, I wonder if it is a good idea to do this kind of parsing in VerifyInstallation. The benefit there is that if we do get some JSON file that doesn't parse (e.g. in the case of disk corruption), the component updater won't load that version of the component and will instead try to re-download it. The drawback (in addition to parsing twice) is that if we ever ship something that doesn't parse, the component updater will repeatedly try to update the component. Thoughts? https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sth_set_component_installer_unittest.cc (right): https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:120: //EXPECT_EQ(2u, observer_->sths.size()); I think this can be done with a BrowserThreadBundle.
Will address the rest of the comments shortly, looking feedback on a specific one. https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:152: safe_json::SafeJsonParser::Parse( On 2016/04/01 17:01:55, waffles wrote: > Hm, I wonder if it is a good idea to do this kind of parsing in > VerifyInstallation. The benefit there is that if we do get some JSON file that > doesn't parse (e.g. in the case of disk corruption), the component updater won't > load that version of the component and will instead try to re-download it. The > drawback (in addition to parsing twice) is that if we ever ship something that > doesn't parse, the component updater will repeatedly try to update the > component. Thoughts? Good point - do you have any metrics on how many clients see corrupt component downloads? Can you suggest a way to do the parsing in VerifyInstallation that would still use the SafeJsonParser? As you can see the parser invokes the success/failure callbacks asynchronously, I'm not sure blocking VerifyInstallation until these callbacks get called is a good idea. (I'm also not too concerned about it since we plan to push components fairly frequently, so a corrupted download should be remedied within 24 hours).
eranm@chromium.org changed reviewers: + dzhioev@chromium.org, rtenneti@chromium.org - davidben@chromium.org
Description was changed from ========== Certificate Transparency: New component for obtaining fresh STHs. This component is for receiving and parsing Signed Tree Heads collected by Google from Certificate Transparency logs and distributed to Chrome clients. It is the first step in checking for inclusion of CT log entries from Chrome. BUG= ========== to ========== Certificate Transparency: New component for obtaining fresh STHs. This component is for receiving and parsing Signed Tree Heads collected by Google from Certificate Transparency logs and distributed to Chrome clients. It is the first step in checking for inclusion of CT log entries from Chrome. Design document: https://docs.google.com/document/d/1FP5J5Sfsg0OR9P4YT0q1dM02iavhi8ix1mZlZe_z-... BUG=506227 ==========
I believe the code is ready for review now. Waffles/Sorin: PTAL. Raman: Adding you as reviewer for the (small) net/ changes as Davidben seems to be OOO for the next two weeks. Pasha: Adding you to review the component registration code in the ChromeOS code. https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:38: // The extension id is: aplidfpohcjpojgnkjpkibbkcghkogef On 2016/04/01 17:01:55, waffles wrote: > We will use ojjgnpkioondelmggbekfhllhdaimnho Done. https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:42: 0x33, 0x90, 0x1c, 0xe9, 0x44, 0x37, 0xc4, 0xa0, 0x2e, 0x02}; On 2016/04/01 17:01:55, waffles wrote: > We will use e996dfa8eed34bc6614a57bb7308cd7e519bcc690841e1969f7cb173ef16800a. > For your convenience: > > {0xe9, 0x96, 0xdf, 0xa8, 0xee, 0xd3, 0x4b, 0xc6, 0x61, 0x4a, 0x57, 0xbb, 0x73, > 0x08, 0xcd, 0x7e, 0x51, 0x9b, 0xcc, 0x69, 0x08, 0x41, 0xe1, 0x96, 0x9f, 0x7c, > 0xb1, 0x73, 0xef, 0x16, 0x80, 0x0a}; Done, thanks. https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:49: VLOG(1) << "XXX: STHSetComponentInstallerTraits::c'tor"; On 2016/04/01 17:01:55, waffles wrote: > These logging statements should be cleaned up prior to check-in. (I know you > said you were just looking for early feedback, but I have trouble not writing > the note.) Done - removed debugging statements, I did leave some VLOG statements behind that could be used for troubleshooting. https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.cc:87: NOTREACHED(); On 2016/04/01 17:01:55, waffles wrote: > Not sure about this. That's a pattern used in other components - will be happy to change though, should I just ignore the return value? https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sth_set_component_installer_unittest.cc (right): https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:120: //EXPECT_EQ(2u, observer_->sths.size()); On 2016/04/01 17:01:55, waffles wrote: > I think this can be done with a BrowserThreadBundle. Done, thanks! That's exactly what I was after.
Thank you Eran. Please consider the idea of moving the code around RegisterSTHSetComponent. The rest of the comments are pedantic and please accept or reject at will. https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/sth_set_component_installer.h:41: STHSetComponentInstallerTraits( explicit https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:448: scoped_ptr<net::ct::STHDistributor> sth_distributor) { Small issue. I wonder if we could avoid passing this parameter to the function and have RegisterSTHSetComponent implementation get the value of the parameter instead. This is a recurring pattern here. Every component needs some kind of run time parameter for registration. We wanted to avoid the case where we'd end up with a bunch of unrelated function parameters, which are used only for one RegisterFoo call below. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:481: if (PathService::Get(chrome::DIR_USER_DATA, &path)) { This is a comment for waffles@ and I. I find it odd that we have this condition here. For one thing, the other installers might be needing this path as well, and at the first glance, there is no good reason to special case it here. It may just be a historical artifact due to how g_browser_process->crl_set_fetcher() worked. Then other installers but not all of them used this condition and the path as a call site argument. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1723: RegisterComponentsForUpdate(std::move(distributor)); Would it be possible to handle this argument inside the STH registration? Please see the comments above. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1152: InitializeCertificateTransparencyRelatedComponents(user); Is Related affix needed? Why not say just InitializeCertificateTransparencyComponents ? https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1435: RegisterSTHSetComponent(cus, path, std::move(distributor)); Related to the other comments, if would be nice is the code is like this: // EV whitelist RegisterEVWhitelistComponent(cus, path); // STH set fetcher. RegisterSTHSetComponent(cus, path); and we deal with the |distributor| inside the RegisterSTHSetComponent. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:73: VLOG(1) << "Component ready, version " << version.GetString() << " in " At like to request a small favor. Could you please remove this line from here and add a similar log statement in default_component_installer.cc@287 ? https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:99: hash->assign(kPublicKeySHA256, we could now use std::begin and std::end, would that be more readable? https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:121: std::string log_id_hex = can this be const? https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:135: std::string log_id; I think this can be initialized at construction time, and also declared const, is that right? https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:36: // in some global object. Instead, a proxy class provided in the C'tor maybe we just say constructor. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:48: // The following methods override ComponentInstallerTraits. Please remember to rebase this change, since new members were added to ComponentInstallerTraits in the mean time. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer_unittest.cc (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:90: std::string good_sth_json = net::ct::GetSampleSTHAsJson(); const? https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:109: base::Version v("1.0"); const? https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:116: std::string first_log_id("abc"); const? https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:121: std::string second_log_id("a\00d", 3); const?
PTAL - I will try to follow up on the comment regarding the creation of STHDistributor over chat. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:448: scoped_ptr<net::ct::STHDistributor> sth_distributor) { On 2016/04/04 22:06:12, Sorin Jianu wrote: > Small issue. > > I wonder if we could avoid passing this parameter to the function and have > RegisterSTHSetComponent implementation get the value of the parameter instead. > > This is a recurring pattern here. Every component needs some kind of run time > parameter for registration. We wanted to avoid the case where we'd end up with a > bunch of unrelated function parameters, which are used only for one RegisterFoo > call below. This CL contains a bigger picture of the intended wiring: https://codereview.chromium.org/1845113003 Briefly, the sth_distributor instance passed into the component has to be shared with the IOThread. The component will invoke a callback on the sth_distributor notifying of new data and all objects that care about it (there's one per profile plus one in the IOThread) will receive it from the sth_distributor. The bigger question is how do components, particularly ones implementing ComponentInstallerTraits export the data they receive. The common pattern I've seen (cld_component_installer, ev_whitelist_component_installer) is by setting global static variable. I wanted to avoid it here because (1) globals and (2) a notification-based mechanism is more suitable for the data pushed to this component. Any advice on how to wire things differently? https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1723: RegisterComponentsForUpdate(std::move(distributor)); On 2016/04/04 22:06:13, Sorin Jianu wrote: > Would it be possible to handle this argument inside the STH registration? Please > see the comments above. See my reply above; in https://codereview.chromium.org/1845113003/diff/40001/chrome/browser/chrome_b... you can see how I intend to use the distributor. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1152: InitializeCertificateTransparencyRelatedComponents(user); On 2016/04/04 22:06:13, Sorin Jianu wrote: > Is Related affix needed? > Why not say just InitializeCertificateTransparencyComponents ? Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1435: RegisterSTHSetComponent(cus, path, std::move(distributor)); On 2016/04/04 22:06:13, Sorin Jianu wrote: > Related to the other comments, if would be nice is the code is like this: > > // EV whitelist > RegisterEVWhitelistComponent(cus, path); > > // STH set fetcher. > RegisterSTHSetComponent(cus, path); > > and we deal with the |distributor| inside the RegisterSTHSetComponent. See comment in chrome_browser_main - that distributor has to get to the IOThread somehow... https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:73: VLOG(1) << "Component ready, version " << version.GetString() << " in " On 2016/04/04 22:06:13, Sorin Jianu wrote: > At like to request a small favor. Could you please remove this line from here > and add a similar log statement in > default_component_installer.cc@287 ? Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:99: hash->assign(kPublicKeySHA256, On 2016/04/04 22:06:13, Sorin Jianu wrote: > we could now use std::begin and std::end, would that be more readable? Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:121: std::string log_id_hex = On 2016/04/04 22:06:13, Sorin Jianu wrote: > can this be const? Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:135: std::string log_id; On 2016/04/04 22:06:13, Sorin Jianu wrote: > I think this can be initialized at construction time, and also declared const, > is that right? Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:36: // in some global object. Instead, a proxy class provided in the C'tor On 2016/04/04 22:06:13, Sorin Jianu wrote: > maybe we just say constructor. Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:48: // The following methods override ComponentInstallerTraits. On 2016/04/04 22:06:13, Sorin Jianu wrote: > Please remember to rebase this change, since new members were added to > ComponentInstallerTraits in the mean time. Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer_unittest.cc (right): https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:90: std::string good_sth_json = net::ct::GetSampleSTHAsJson(); On 2016/04/04 22:06:13, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:109: base::Version v("1.0"); On 2016/04/04 22:06:13, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:116: std::string first_log_id("abc"); On 2016/04/04 22:06:13, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:121: std::string second_log_id("a\00d", 3); On 2016/04/04 22:06:13, Sorin Jianu wrote: > const? Done.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
As I've said in past reviews, I'm concerned about the cost of the VLOGs relative to their diagnostic value. So far, I'm unaware of any situation that's happened in the past that would have benefitted from VLOGing for these bits, and we've handled it so far with chrome://net-internals for the bits that have mattered. I totally get the need to understand and diagnose updates, but I err on the side of not logging until we can describe a set of problems we anticipate or expect, and if we can do that, generally we can address them in the code without logging ;) https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1432: // TreeStateTracker instances can register for STH updates. Can you file a bug? Mostly because I'm not sure what you're trying to communicate here. The bug would capture "Here's what we're doing, here's what we need to do, and why" and track the "doing the thing we need to do" part. Mostly, I'm not sure the "why" has enough context here. Plus it helps us track removing TODOs :) https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.h:166: // Starts loading EV Certificates whitelist. Comment update https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:7: #include <string> Is in header https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:9: #include <vector> Is in header https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:16: #include "base/macros.h" Is in header / unnecessary? https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:96: PathService::Get(DIR_CERT_TRANS_TREE_STATES, &result); Is it safe to ignore the return value here? Is that what others do? https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:116: FILE_PATH_LITERAL("*.sth")); DESIGN/Curious: Is there a reason to store these as individual files? Doesn't that result in worse efficiency on most modern systems? (Versus a single file, whose size is probably going to be rather small - 4K - and thus fit generally within a single read call buffer from the OS, whereas separate files means less buffering/read-ahead) If we have to change this, you'd have to rev the extension ID, hence why I bring this up now. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:120: VLOG(1) << "Reading STH from file: " << sth_file_path.value(); You asked me to review this as if it was done, but there's still a lot of chatty log spam, and you know I flag it on every review :) https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:8: #include <stdint.h> STYLE: Newline between 8/9 - https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:12: #include "base/files/file_path.h" Forward declarable https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:16: #include "base/values.h" Don't include this if you're forward declaring on 20-23 (which you should be able to, based on usage) https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:18: #include "net/cert/sth_observer.h" Forward declarable https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:36: // in some global object. Instead, a proxy class provided in the constructor I would suggest this last sentence is a bit too vague a commitment/explanation. "Instead, notifications of each of the new STHs are sent to the net::ct::STHObserver, so that it can take appropriate steps, including possible persistence." https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:79: FRIEND_TEST_ALL_PREFIXES(STHSetComponentInstallerTest, LoadSTHsFromDisk); Could you place this at the start of the "private" section? We try to group such friends together, in part, because this is a forward declaration (in effect), and we group those at the start, per https://google.github.io/styleguide/cppguide.html#Declaration_Order This would then raise the question about "Why do you friend the class, but also the specific tests" - generally, we try for one or the other (e.g. if STHSetComponentInstallerTest is friend'd, then it can provide the access to the tests as needed) https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer_unittest.cc (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:8: // 3. Does not notify of valid JSON but in a file not hex-encoded log id. Please move this sort of description to the individual tests to explain what each test is covering. It's extremely unlikely that people will remember to update the comments when they're so far separated from the code. The fact that only one test is here (CanLoadAllSTHs) makes me think that test is too overly broad, and you're really better off with individual tests covering this. https://codereview.chromium.org/1853753003/diff/60001/net/cert/sth_observer.h File net/cert/sth_observer.h (right): https://codereview.chromium.org/1853753003/diff/60001/net/cert/sth_observer.h... net/cert/sth_observer.h:29: class NET_EXPORT STHReporter { Shouldn't this be in its own header? STHDistributor exists to glue these two classes together, which suggests that they're not intended to be coupled - if they were, we could just couple them in a single class, right? I'm trying to envision the flow of dependencies here, since the logical reason to separate them out is to separate out dependencies / KISS (which is fantastic), but then they end up coupled in the header.
eranm@chromium.org changed reviewers: + sky@chromium.org
sky: please review the change to chrome/browser/chrome_browser_main.cc Sorin: As discussed offline, I've moved creation of the STHDistributor to the RegisterSTHSetComponent method implementation. I believe it's the optimal solution as it still allows me to test STHSetComponentInstallerTraits easily by injecting a listener and is not invasive in component registration code. Ryan: Addressed all your comments, PTAL. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1432: // TreeStateTracker instances can register for STH updates. On 2016/04/06 18:32:50, Ryan Sleevi wrote: > Can you file a bug? Mostly because I'm not sure what you're trying to > communicate here. > > The bug would capture "Here's what we're doing, here's what we need to do, and > why" and track the "doing the thing we need to do" part. Mostly, I'm not sure > the "why" has enough context here. Plus it helps us track removing TODOs :) Done - that TODO now lives in sth_set_component_installer.cc, where I linked to crbug.com/506227 (and the design doc). The follow-up work is in an immediate CL. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.h:166: // Starts loading EV Certificates whitelist. On 2016/04/06 18:32:50, Ryan Sleevi wrote: > Comment update Done. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:7: #include <string> On 2016/04/06 18:32:51, Ryan Sleevi wrote: > Is in header Done. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:9: #include <vector> On 2016/04/06 18:32:51, Ryan Sleevi wrote: > Is in header Done. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:16: #include "base/macros.h" On 2016/04/06 18:32:51, Ryan Sleevi wrote: > Is in header / unnecessary? Done. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:96: PathService::Get(DIR_CERT_TRANS_TREE_STATES, &result); On 2016/04/06 18:32:50, Ryan Sleevi wrote: > Is it safe to ignore the return value here? Is that what others do? I believe it's safe - it's done in all other classes that implement ComponentInstallerTraits and seems like we'd return an empty FilePath if PathService::Get fails, as it will not change the result if it fails. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:116: FILE_PATH_LITERAL("*.sth")); On 2016/04/06 18:32:50, Ryan Sleevi wrote: > DESIGN/Curious: Is there a reason to store these as individual files? Doesn't > that result in worse efficiency on most modern systems? (Versus a single file, > whose size is probably going to be rather small - 4K - and thus fit generally > within a single read call buffer from the OS, whereas separate files means less > buffering/read-ahead) > > If we have to change this, you'd have to rev the extension ID, hence why I bring > this up now. This was done mainly for simplicity of the implementation, both client and server side, as the same JSON format used by logs to return the STHs is used throughout the system for providing them. Also we already have STH JSON parsing code under net/ and the less code I have to change there, the better :) I agree it's more efficient to read it in a single call from a known filename rather than iterate over the directory, however I am not sure it is justified given the added code complexity of coming up with a new file format and the fact that loading the new component is done in the background. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.cc:120: VLOG(1) << "Reading STH from file: " << sth_file_path.value(); On 2016/04/06 18:32:51, Ryan Sleevi wrote: > You asked me to review this as if it was done, but there's still a lot of chatty > log spam, and you know I flag it on every review :) I've made a mental note to search for those before any future review :) Moved all to DVLOG, left one VLOG statement for when the provided STH json fails to parse, we'd like to be able to get reports from the field on what exactly was the JSON that failed to parse. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:8: #include <stdint.h> On 2016/04/06 18:32:51, Ryan Sleevi wrote: > STYLE: Newline between 8/9 - > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:12: #include "base/files/file_path.h" On 2016/04/06 18:32:51, Ryan Sleevi wrote: > Forward declarable Done. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:16: #include "base/values.h" On 2016/04/06 18:32:51, Ryan Sleevi wrote: > Don't include this if you're forward declaring on 20-23 (which you should be > able to, based on usage) Done. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:18: #include "net/cert/sth_observer.h" On 2016/04/06 18:32:51, Ryan Sleevi wrote: > Forward declarable Done. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:36: // in some global object. Instead, a proxy class provided in the constructor On 2016/04/06 18:32:51, Ryan Sleevi wrote: > I would suggest this last sentence is a bit too vague a commitment/explanation. > > "Instead, notifications of each of the new STHs are sent to the > net::ct::STHObserver, so that it can take appropriate steps, including possible > persistence." Done. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:79: FRIEND_TEST_ALL_PREFIXES(STHSetComponentInstallerTest, LoadSTHsFromDisk); On 2016/04/06 18:32:51, Ryan Sleevi wrote: > Could you place this at the start of the "private" section? We try to group such > friends together, in part, because this is a forward declaration (in effect), > and we group those at the start, per > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > This would then raise the question about "Why do you friend the class, but also > the specific tests" - generally, we try for one or the other (e.g. if > STHSetComponentInstallerTest is friend'd, then it can provide the access to the > tests as needed) > Done - moved that and removed the 'friend class STHSetComponentInstallerTest' declaration. https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer_unittest.cc (right): https://codereview.chromium.org/1853753003/diff/60001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:8: // 3. Does not notify of valid JSON but in a file not hex-encoded log id. On 2016/04/06 18:32:51, Ryan Sleevi wrote: > Please move this sort of description to the individual tests to explain what > each test is covering. Done > > It's extremely unlikely that people will remember to update the comments when > they're so far separated from the code. > > The fact that only one test is here (CanLoadAllSTHs) makes me think that test is > too overly broad, and you're really better off with individual tests covering > this. Done https://codereview.chromium.org/1853753003/diff/60001/net/cert/sth_observer.h File net/cert/sth_observer.h (right): https://codereview.chromium.org/1853753003/diff/60001/net/cert/sth_observer.h... net/cert/sth_observer.h:29: class NET_EXPORT STHReporter { On 2016/04/06 18:32:51, Ryan Sleevi wrote: > Shouldn't this be in its own header? STHDistributor exists to glue these two > classes together, which suggests that they're not intended to be coupled - if > they were, we could just couple them in a single class, right? I'm trying to > envision the flow of dependencies here, since the logical reason to separate > them out is to separate out dependencies / KISS (which is fantastic), but then > they end up coupled in the header. You're right, I've separated them into different headers.
lgtm https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1710: if (!parsed_command_line().HasSwitch(switches::kDisableComponentUpdate)) { Our style thus far has been to elide the braces for one-line conditionals. https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:38: // Instead, notifications of each of the new STHs are sent to the Instead of what? (Maybe just eliminate "instead".)
lgtm Thank you Eran! https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1428: // EV whitelist needs . at the end. https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:77: void OnJsonParseSuccess(std::string log_id, maybe pass be reference to const instead of by value? https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:81: void OnJsonParseError(std::string log_id, const std::string& error); same
Thanks for the prompt review, addressed all comments. https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1710: if (!parsed_command_line().HasSwitch(switches::kDisableComponentUpdate)) { On 2016/04/07 13:30:49, waffles wrote: > Our style thus far has been to elide the braces for one-line conditionals. My bad, done. https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1428: // EV whitelist On 2016/04/07 15:49:48, Sorin Jianu wrote: > needs . at the end. Done. https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:38: // Instead, notifications of each of the new STHs are sent to the On 2016/04/07 13:30:49, waffles wrote: > Instead of what? (Maybe just eliminate "instead".) Done. https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:77: void OnJsonParseSuccess(std::string log_id, On 2016/04/07 15:49:48, Sorin Jianu wrote: > maybe pass be reference to const instead of by value? Done. https://codereview.chromium.org/1853753003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/sth_set_component_installer.h:81: void OnJsonParseError(std::string log_id, const std::string& error); On 2016/04/07 15:49:49, Sorin Jianu wrote: > same Done.
At this point, it's mostly nits, so LGTM https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:158: DVLOG(0) << "STH parsing success for log: " Aren't your logging priorities inverted here? Errors should log at the default vlog level, but tracing diagnostics should be errors + 1 https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:161: LOG(WARNING) << "Failed to fill in signed tree head."; Isn't this error? https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:177: << " for log: " << base::HexEncode(log_id.data(), log_id.length()); Arguably, you don't need this either, or can/should bump the default up (to 1). If there's any errors, with a VLOG you'd say "Run Chrome with this command-line and then mail the log back to us" which, if this triggered, would then be "Now mail this file from your user data dir to us" When really, you could just short-circuit it, with "Send us all the files in this directory" And you just run locally That is, it doesn't aid diagnostics "in the field" in any meaningful way (adds a round-trip), and the result either way is you'd fire up a debugger and investigate so... drop the log here too? :) https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.h:53: DoesNotLoadValidJSONFromFileNotHexEncoded); I will push back on this if any additional prefixes grow - we shouldn't be having to enumerate all tests in the header (instead, the STHSetComponentInstallerTest should hide any logic/access). That is, longer-term, ideally only line 48 will remain. This is right at the cusp though, so I'm willing to let it slide. https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.h:55: // The following methods override ComponentInstallerTraits. The (almost canonical) form we've taken, which matches the style guide, is // ComponentInstallerTraits implementation. https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.h:69: static base::FilePath GetInstalledPath(const base::FilePath& base); A static private is almost always a wrong (exception: tests, but then the tests are generally wrong) just make it a function in the unnamed namespace in the .cc https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer_unittest.cc (right): https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:49: // created *as temp files*, they will still get cleaned up automagically. This seems to be explaining ScopedTempDir, but that doesn't seem necessary/germane? The header for ScopedTempDir should cover all of the behaviour you just described here. https://codereview.chromium.org/1853753003/diff/120001/net/cert/sth_observer.h File net/cert/sth_observer.h (right): https://codereview.chromium.org/1853753003/diff/120001/net/cert/sth_observer.... net/cert/sth_observer.h:24: // Expected to be called on the IO thread. Layering: Don't talk about "the IO thread" in //net - we stay ignorant of that. All of //net is assumed to be thread-unsafe - it's created, lives, and dies on a single thread. The only time we call out threads is when an object is taking taskrunners or when it internally has peculiar thread semantics. Really, you can just delete line 24.
LGTM
Thanks, addressed all comments. https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:158: DVLOG(0) << "STH parsing success for log: " On 2016/04/07 18:00:12, Ryan Sleevi wrote: > Aren't your logging priorities inverted here? > > Errors should log at the default vlog level, but tracing diagnostics should be > errors + 1 Good point, changed to DVLOG(1) https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:161: LOG(WARNING) << "Failed to fill in signed tree head."; On 2016/04/07 18:00:12, Ryan Sleevi wrote: > Isn't this error? yes, changed to LOG(ERROR) https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:177: << " for log: " << base::HexEncode(log_id.data(), log_id.length()); On 2016/04/07 18:00:12, Ryan Sleevi wrote: > Arguably, you don't need this either, or can/should bump the default up (to 1). > > If there's any errors, with a VLOG you'd say > "Run Chrome with this command-line and then mail the log back to us" > which, if this triggered, would then be > "Now mail this file from your user data dir to us" > > When really, you could just short-circuit it, with > "Send us all the files in this directory" > > And you just run locally > > That is, it doesn't aid diagnostics "in the field" in any meaningful way (adds a > round-trip), and the result either way is you'd fire up a debugger and > investigate so... drop the log here too? :) Good point, changed to DVLOG(1) https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer.h (right): https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.h:53: DoesNotLoadValidJSONFromFileNotHexEncoded); On 2016/04/07 18:00:13, Ryan Sleevi wrote: > I will push back on this if any additional prefixes grow - we shouldn't be > having to enumerate all tests in the header (instead, the > STHSetComponentInstallerTest should hide any logic/access). That is, > longer-term, ideally only line 48 will remain. > > This is right at the cusp though, so I'm willing to let it slide. Actually all those FRIEND_TEST declarations are no longer necessary now that helper methods in STHSetComponentInstallerTest do the job. https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.h:55: // The following methods override ComponentInstallerTraits. On 2016/04/07 18:00:13, Ryan Sleevi wrote: > The (almost canonical) form we've taken, which matches the style guide, is > > // ComponentInstallerTraits implementation. Done. https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.h:69: static base::FilePath GetInstalledPath(const base::FilePath& base); On 2016/04/07 18:00:13, Ryan Sleevi wrote: > A static private is almost always a wrong (exception: tests, but then the tests > are generally wrong) > > just make it a function in the unnamed namespace in the .cc Done. https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer_unittest.cc (right): https://codereview.chromium.org/1853753003/diff/120001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer_unittest.cc:49: // created *as temp files*, they will still get cleaned up automagically. On 2016/04/07 18:00:13, Ryan Sleevi wrote: > This seems to be explaining ScopedTempDir, but that doesn't seem > necessary/germane? The header for ScopedTempDir should cover all of the > behaviour you just described here. Done - removed. https://codereview.chromium.org/1853753003/diff/120001/net/cert/sth_observer.h File net/cert/sth_observer.h (right): https://codereview.chromium.org/1853753003/diff/120001/net/cert/sth_observer.... net/cert/sth_observer.h:24: // Expected to be called on the IO thread. On 2016/04/07 18:00:13, Ryan Sleevi wrote: > Layering: Don't talk about "the IO thread" in //net - we stay ignorant of that. > > All of //net is assumed to be thread-unsafe - it's created, lives, and dies on a > single thread. The only time we call out threads is when an object is taking > taskrunners or when it internally has peculiar thread semantics. > > Really, you can just delete line 24. Done.
The CQ bit was checked by eranm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org, sorin@chromium.org, sky@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1853753003/#ps140001 (title: "Addressing review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1853753003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1853753003/140001
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency: New component for obtaining fresh STHs. This component is for receiving and parsing Signed Tree Heads collected by Google from Certificate Transparency logs and distributed to Chrome clients. It is the first step in checking for inclusion of CT log entries from Chrome. Design document: https://docs.google.com/document/d/1FP5J5Sfsg0OR9P4YT0q1dM02iavhi8ix1mZlZe_z-... BUG=506227 ========== to ========== Certificate Transparency: New component for obtaining fresh STHs. This component is for receiving and parsing Signed Tree Heads collected by Google from Certificate Transparency logs and distributed to Chrome clients. It is the first step in checking for inclusion of CT log entries from Chrome. Design document: https://docs.google.com/document/d/1FP5J5Sfsg0OR9P4YT0q1dM02iavhi8ix1mZlZe_z-... BUG=506227 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency: New component for obtaining fresh STHs. This component is for receiving and parsing Signed Tree Heads collected by Google from Certificate Transparency logs and distributed to Chrome clients. It is the first step in checking for inclusion of CT log entries from Chrome. Design document: https://docs.google.com/document/d/1FP5J5Sfsg0OR9P4YT0q1dM02iavhi8ix1mZlZe_z-... BUG=506227 ========== to ========== Certificate Transparency: New component for obtaining fresh STHs. This component is for receiving and parsing Signed Tree Heads collected by Google from Certificate Transparency logs and distributed to Chrome clients. It is the first step in checking for inclusion of CT log entries from Chrome. Design document: https://docs.google.com/document/d/1FP5J5Sfsg0OR9P4YT0q1dM02iavhi8ix1mZlZe_z-... BUG=506227 Committed: https://crrev.com/9e1935c26148f42c6fe453cd380207c45934b944 Cr-Commit-Position: refs/heads/master@{#385903} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9e1935c26148f42c6fe453cd380207c45934b944 Cr-Commit-Position: refs/heads/master@{#385903} |
