Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1405293009:
Certificate Transparency: Fetching consistency proofs. (Closed)
|
Created:
5 years, 1 month ago by Eran Messeri Modified:
4 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, davidben, Rob Percival, certificate-transparency-chrome_googlegroups.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency: Fetching consistency proofs.
This adds the ability for Chrome clients to fetch consistency proofs between two tree heads.
It will be used in case a Chrome client has an old STH stored for which it was not provided with a consistency proof to a new STH pushed in a component update.
BUG=506227
Committed: https://crrev.com/6851c0c562cb3f711f0fa03fff18a0750ac221e9
Cr-Commit-Position: refs/heads/master@{#374865}
Patch Set 1 #Patch Set 2 : Post merge with master #
Total comments: 14
Patch Set 3 : Addressing review comments. #
Total comments: 12
Patch Set 4 : Addressing review comments and compilation issues #Patch Set 5 : Fix compilation on Android #
Total comments: 32
Patch Set 6 : Addressing review comments #
Total comments: 21
Patch Set 7 : Class hierarchy for various requests #Patch Set 8 : Switched to using RunLoop #
Total comments: 6
Patch Set 9 : Added LogFetcher for handling a single request #Patch Set 10 : Addressing forgotten review comment. #
Total comments: 32
Patch Set 11 : Addressing review comments #
Total comments: 14
Patch Set 12 : Addressing comments #
Total comments: 36
Patch Set 13 : Switching to simpler callbacks model #
Total comments: 54
Patch Set 14 : Design, documentation fixes #
Total comments: 4
Patch Set 15 : Addressing mmenke's comments #
Total comments: 2
Patch Set 16 : Small documentation fix #Patch Set 17 : Catching up with master #
Messages
Total messages: 57 (12 generated)
Description was changed from ========== DRAFT: Certificate Transparency: Fetching consistency proofs. A part of this patch is already out for review. BUG= ========== to ========== DRAFT: Certificate Transparency: Fetching consistency proofs. A part of this patch is already out for review. BUG= ==========
eranm@chromium.org changed reviewers: + mmenke@chromium.org
Description was changed from ========== DRAFT: Certificate Transparency: Fetching consistency proofs. A part of this patch is already out for review. BUG= ========== to ========== Certificate Transparency: Fetching consistency proofs. This adds the ability for Chrome clients to fetch consistency proofs between two tree heads. It will be used in case a Chrome client has an old STH stored for which it was not provided with a consistency proof to a new STH pushed in a component update. BUG=506227 ==========
Matt, this change adds the ability to fetch consistency proofs, implementing one step of the plan outlined in https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/l6k0FWeLsx4
mmenke@chromium.org changed reviewers: + svaldez@chromium.org
[+svaldez]: Mind doing an initial pass on this CL?
Did a quick initial pass. I'll try doing a more thorough pass in a bit. https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:75: FetchFailedCallback failed_callback; Can't you just add: SignedTreeHeadFetchedCallback sth_fetch_callback; ConsistencyProofFetchedCallback proof_fetch_callback; And then remove the fetch_callbacks_ maps and related methods? The FetchState lifetime seems to be identical to the elements of the callbacks map https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:133: .ReplaceComponents(replacements); Can't you just do: std::string relative = StringPrintf("ct/v1/get-sth-consistency?first=%lu&second=%lu", ...) GURL fetch_url = base_log_url.Resolve(relative); https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:44: return std::string(32, 'a' + node_id); Consider hashing the node_id or some alternative dummy values so that kDummyConsistencyProofLength doesn't have to be constrained by this implementation. https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:51: LogFetchTestJob(const std::string& get_sth_data, nit: Rename to something more general. get_log_data/get_log_headers https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:258: const int kNewTree = 8; nit: size_t https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:370: EXPECT_EQ(net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED, callback.net_error()); Check the callback response code? https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:373: TEST_F(LogProofFetcherTest, TestInvalidGetConsistencyValidReply) { Should this be TestValidGetConsistency...? Might also want to move it above the Invalid test, to match ordering of the previous tests.
Addressed all comments, PTAL. https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:75: FetchFailedCallback failed_callback; On 2015/11/12 20:11:03, svaldez wrote: > Can't you just add: > > SignedTreeHeadFetchedCallback sth_fetch_callback; > ConsistencyProofFetchedCallback proof_fetch_callback; > > And then remove the fetch_callbacks_ maps and related methods? > > The FetchState lifetime seems to be identical to the elements of the callbacks > map Done - it does simplify the design. https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:133: .ReplaceComponents(replacements); On 2015/11/12 20:11:03, svaldez wrote: > Can't you just do: > > std::string relative = > StringPrintf("ct/v1/get-sth-consistency?first=%lu&second=%lu", ...) > GURL fetch_url = base_log_url.Resolve(relative); Done. https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:44: return std::string(32, 'a' + node_id); On 2015/11/12 20:11:03, svaldez wrote: > Consider hashing the node_id or some alternative dummy values so that > kDummyConsistencyProofLength doesn't have to be constrained by this > implementation. Done. https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:51: LogFetchTestJob(const std::string& get_sth_data, On 2015/11/12 20:11:03, svaldez wrote: > nit: Rename to something more general. get_log_data/get_log_headers Done. https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:258: const int kNewTree = 8; On 2015/11/12 20:11:03, svaldez wrote: > nit: size_t Done. https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:370: EXPECT_EQ(net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED, callback.net_error()); On 2015/11/12 20:11:03, svaldez wrote: > Check the callback response code? Done. https://codereview.chromium.org/1405293009/diff/20001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:373: TEST_F(LogProofFetcherTest, TestInvalidGetConsistencyValidReply) { On 2015/11/12 20:11:03, svaldez wrote: > Should this be TestValidGetConsistency...? > > Might also want to move it above the Invalid test, to match ordering of the > previous tests. Done.
Few more nits. https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:52: ConsistencyProofFetchedCallback proof_fetch_callback, const ...FetchedCallback& https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:103: ConsistencyProofFetchedCallback(), failed_callback); Might be able to just pass 'nullptr'? https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:126: SignedTreeHeadFetchedCallback(), fetched_callback, failed_callback); ditto. https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:42: // so forth. Fix comment. https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:92: response_headers_( Consider setting default expected_*_tree_size_ and removing set_expect_...(0, 0) https://codereview.chromium.org/1405293009/diff/40001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1405293009/diff/40001/net/base/net_error_list... net/base/net_error_list.h:366: // Certificate Transparency: Received a consistency proof that failed to parse nit: failed to parse.
Thanks for the quick re-review, PTAL. https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:52: ConsistencyProofFetchedCallback proof_fetch_callback, On 2015/11/16 17:33:07, svaldez wrote: > const ...FetchedCallback& Done. https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:103: ConsistencyProofFetchedCallback(), failed_callback); On 2015/11/16 17:33:07, svaldez wrote: > Might be able to just pass 'nullptr'? Can't - Callback doesn't have a single argument c'tor (so the compiler spits out: no known conversion from 'nullptr_t' to 'const ConsistencyProofFetchedCallback). https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:126: SignedTreeHeadFetchedCallback(), fetched_callback, failed_callback); On 2015/11/16 17:33:07, svaldez wrote: > ditto. See comment above. https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:42: // so forth. On 2015/11/16 17:33:07, svaldez wrote: > Fix comment. D'oh, done. https://codereview.chromium.org/1405293009/diff/40001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:92: response_headers_( On 2015/11/16 17:33:08, svaldez wrote: > Consider setting default expected_*_tree_size_ and removing set_expect_...(0, 0) Done. https://codereview.chromium.org/1405293009/diff/40001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1405293009/diff/40001/net/base/net_error_list... net/base/net_error_list.h:366: // Certificate Transparency: Received a consistency proof that failed to parse On 2015/11/16 17:33:08, svaldez wrote: > nit: failed to parse. Done.
lgtm, though mmenke will need to take a look as an actual owner.
On 2015/11/17 21:49:11, svaldez wrote: > lgtm, though mmenke will need to take a look as an actual owner. Thanks for the thorough review, Steven! I'll try to get to this tomorrow.
https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:96: const FetchFailedCallback& failed_callback) { Suggest putting everything in this method except the line to get the fetch_url into a subroutine, shared with FetchConsistencyProof. Not a fan of duplicated code. You could just inline CreateRequest in that method, too, unless you think you'll need to add another method where it will be useful. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:120: static_cast<unsigned long>(new_tree_size)); include format_macros.h and use PRIuS to use printf methods with size_t's. Or if these should really be unsigned ints, make the arguments to this method unsigned ints. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:272: if (net::ct::FillSignedTreeHead(*parsed_json.get(), &signed_tree_head)) { nit: While you're here, .get() isn't needed. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:273: DCHECK(!(fetch_state->sth_fetch_callback.is_null())); This DCHECK makes much more sense in FetchSignedTreeHead. You should also check the failed_callback there, too (Or NULL-check it before calling it). You can keep a DCHECK here for documentation purposes, if you want. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:294: if (net::ct::FillConsistencyProof(*parsed_json.get(), &consistency_proof)) { See comment in OnSTHJsonParseSuccess. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:295: DCHECK(!(fetch_state->proof_fetch_callback.is_null())); See comment in OnSTHJsonParseSuccess. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:62: const std::vector<std::string>& consistency_proof)>; include <vector> https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:91: const GURL& base_log_url, include url/gurl.h https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:139: net::URLRequest* CreateRequest(const GURL& url); make this return a scoped_ptr, to make ownership semantics clear? https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:90: response_body_(""), nit: Not needed (Know it was like that before) https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:102: "%s://%s/%s/ct/v1/", kLogSchema, kLogHost, kLogPathPrefix); I don't think we want logic in here that matches the logic in production code to deduce the expected URL. Suggest adding a setter, and explicitly calling it in tests. Alternatively, could make an accessor for the URL we end up fetching, and put the EXPECT in the test body (Or in RunGetConsistencyFetcherWithCallback / RunFetcherWithCallback) - think that makes for clearer tests. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:185: invoked_ = true; Record which method was invoked? https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:189: << " node: " << i; Again, think it's cleaner to store these in the test fixture, and then make the test body check them. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:253: message_loop_.RunUntilIdle(); optional: I did sign off on this before...But think it would be cleaner to use a RunLoop invoked by RecordFetchCallbackInvocations, rather than rely on everything happening without any intervening idle time. The disadvantage is on breakage, the test hangs. The advantage is it's more flexible, and doesn't break if someone changes how the test works (Switches to a real test server, or whatever).
Matt, thanks for the thorough review - hopefully you can have another look before the US holiday. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:96: const FetchFailedCallback& failed_callback) { On 2015/11/18 19:25:15, mmenke wrote: > Suggest putting everything in this method except the line to get the fetch_url > into a subroutine, shared with FetchConsistencyProof. Not a fan of duplicated > code. You could just inline CreateRequest in that method, too, unless you think > you'll need to add another method where it will be useful. Done. I note that the use of two callbacks is a smell of poor man's inheritance pattern - I can fix it to have a delegate class that will do the json parsing and invoke the callback for each case. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:120: static_cast<unsigned long>(new_tree_size)); On 2015/11/18 19:25:15, mmenke wrote: > include format_macros.h and use PRIuS to use printf methods with size_t's. Or > if these should really be unsigned ints, make the arguments to this method > unsigned ints. Done. These are size_t - they indicate size of a Merkle tree (which is always non-negative). https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:272: if (net::ct::FillSignedTreeHead(*parsed_json.get(), &signed_tree_head)) { On 2015/11/18 19:25:15, mmenke wrote: > nit: While you're here, .get() isn't needed. Done. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:273: DCHECK(!(fetch_state->sth_fetch_callback.is_null())); On 2015/11/18 19:25:15, mmenke wrote: > This DCHECK makes much more sense in FetchSignedTreeHead. You should also check > the failed_callback there, too (Or NULL-check it before calling it). > > You can keep a DCHECK here for documentation purposes, if you want. Good point - DCHECK made much earlier on, removed from here. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:294: if (net::ct::FillConsistencyProof(*parsed_json.get(), &consistency_proof)) { On 2015/11/18 19:25:15, mmenke wrote: > See comment in OnSTHJsonParseSuccess. Done. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:295: DCHECK(!(fetch_state->proof_fetch_callback.is_null())); On 2015/11/18 19:25:15, mmenke wrote: > See comment in OnSTHJsonParseSuccess. Done (removed unnecessary DCHECKs here as they are performed before the FetchState instance is created). https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:62: const std::vector<std::string>& consistency_proof)>; On 2015/11/18 19:25:15, mmenke wrote: > include <vector> Done. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:91: const GURL& base_log_url, On 2015/11/18 19:25:15, mmenke wrote: > include url/gurl.h Done. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:139: net::URLRequest* CreateRequest(const GURL& url); On 2015/11/18 19:25:15, mmenke wrote: > make this return a scoped_ptr, to make ownership semantics clear? Obsolete - to avoid code duplication the code in this method was placed in a method that creates the request, FetchState, etc, per comment in .cc file. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:90: response_body_(""), On 2015/11/18 19:25:16, mmenke wrote: > nit: Not needed (Know it was like that before) Done. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:102: "%s://%s/%s/ct/v1/", kLogSchema, kLogHost, kLogPathPrefix); On 2015/11/18 19:25:16, mmenke wrote: > I don't think we want logic in here that matches the logic in production code to > deduce the expected URL. Suggest adding a setter, and explicitly calling it in > tests. > > Alternatively, could make an accessor for the URL we end up fetching, and put > the EXPECT in the test body (Or in RunGetConsistencyFetcherWithCallback / > RunFetcherWithCallback) - think that makes for clearer tests. Done - by adding a setter, as the MaybeInterceptRequest method is const, I can't change any fields. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:185: invoked_ = true; On 2015/11/18 19:25:15, mmenke wrote: > Record which method was invoked? Done - that made the invoked_ field obsolete so I've removed it. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:189: << " node: " << i; On 2015/11/18 19:25:16, mmenke wrote: > Again, think it's cleaner to store these in the test fixture, and then make the > test body check them. Done - both for the consistency proof and the STH fetching. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:253: message_loop_.RunUntilIdle(); On 2015/11/18 19:25:16, mmenke wrote: > optional: I did sign off on this before...But think it would be cleaner to use > a RunLoop invoked by RecordFetchCallbackInvocations, rather than rely on > everything happening without any intervening idle time. The disadvantage is on > breakage, the test hangs. The advantage is it's more flexible, and doesn't > break if someone changes how the test works (Switches to a real test server, or > whatever). IIRC there was a problem with the async_io case - I'll dig up the bug number shortly.
https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:96: const FetchFailedCallback& failed_callback) { On 2015/11/24 22:53:38, Eran Messeri wrote: > On 2015/11/18 19:25:15, mmenke wrote: > > Suggest putting everything in this method except the line to get the fetch_url > > into a subroutine, shared with FetchConsistencyProof. Not a fan of duplicated > > code. You could just inline CreateRequest in that method, too, unless you > think > > you'll need to add another method where it will be useful. > > Done. I note that the use of two callbacks is a smell of poor man's inheritance > pattern - I can fix it to have a delegate class that will do the json parsing > and invoke the callback for each case. Could also make a single callback that takes a value indicating success or failure. I don't have strong feelings about it, though the prototype of FetchFromLog is looking pretty ugly now. :( https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:102: "%s://%s/%s/ct/v1/", kLogSchema, kLogHost, kLogPathPrefix); On 2015/11/24 22:53:38, Eran Messeri wrote: > On 2015/11/18 19:25:16, mmenke wrote: > > I don't think we want logic in here that matches the logic in production code > to > > deduce the expected URL. Suggest adding a setter, and explicitly calling it > in > > tests. > > > > Alternatively, could make an accessor for the URL we end up fetching, and put > > the EXPECT in the test body (Or in RunGetConsistencyFetcherWithCallback / > > RunFetcherWithCallback) - think that makes for clearer tests. > > Done - by adding a setter, as the MaybeInterceptRequest method is const, I can't > change any fields. You can always use mutable. Anyhow, fine as-is. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:137: } optional: May be cleaner as an argument...Or maybe not. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:141: enum InterceptedRequestType { RequestType -> ResultType? I don't think a failure or nothing is really a type of request. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:189: InterceptedRequestType intercepted_request_type() { return request_type_; } intercepted_request_type() const https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:195: const net::ct::SignedTreeHead& intercepted_sth() { return sth_; } const https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:197: const std::string& intercepted_log_id() { return log_id_; } const https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:199: const std::vector<std::string>& intercepted_proof() { const https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:294: } On 2015/11/24 22:53:38, Eran Messeri wrote: > On 2015/11/18 19:25:16, mmenke wrote: > > optional: I did sign off on this before...But think it would be cleaner to > use > > a RunLoop invoked by RecordFetchCallbackInvocations, rather than rely on > > everything happening without any intervening idle time. The disadvantage is > on > > breakage, the test hangs. The advantage is it's more flexible, and doesn't > > break if someone changes how the test works (Switches to a real test server, > or > > whatever). > > IIRC there was a problem with the async_io case - I'll dig up the bug number > shortly. I tossed in a RunLoop locally, and all tests passed, at least on Windows.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1405293009/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:45: enum LogRequestType { SIGNED_TREE_HEAD, CONSISTENCY_PROOF }; Document https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:78: DCHECK(!(sth_fetch_callback.is_null() && proof_fetch_callback.is_null())); Past experience has shown mixing state objects like this is generally a design smell and a recipe for bugs. I imagine there will be further, future requests to make of the log - either to handle V1 responses or for more V2 queries, right? This makes me wonder whether this should be re-structured somewhat: a) Does LogProofFetcher need to be a URLRequestDelegate itself? Could it defer the actual requesting to an inner class (e.g. FetchState) to be the Delegate? b) It seems like FetchState is really FetchAndParseState. Would it make more sense for FetchState to have a singular callback that it calls, with the fully assembled response, and then that callback can take appropriate action? Currying could help here. I'm a little concerned about the size and amount of logic growing in the class here - there's probably a few things we can do to simplify, but perhaps in a separate CL. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:107: size_t old_tree_size, BUG: I'm fairly sure these should be uint64_t types, and not size_t. 32-bit clients shouldn't be limited to 32-bit entries from logs, right? https://codereview.chromium.org/1405293009/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:140: // Creates a new request with the right flags for log requests. "with the right flags" is not very clear/helpful.
Happy Thanksgiving! I've addressed all comments except the RunLoop in the tests, which I'm now looking into. Note I've rewrote the way different log requests are handled - there's now a class hierarchy abstracting away the particulars of each request. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:96: const FetchFailedCallback& failed_callback) { On 2015/11/25 17:40:28, mmenke wrote: > On 2015/11/24 22:53:38, Eran Messeri wrote: > > On 2015/11/18 19:25:15, mmenke wrote: > > > Suggest putting everything in this method except the line to get the > fetch_url > > > into a subroutine, shared with FetchConsistencyProof. Not a fan of > duplicated > > > code. You could just inline CreateRequest in that method, too, unless you > > think > > > you'll need to add another method where it will be useful. > > > > Done. I note that the use of two callbacks is a smell of poor man's > inheritance > > pattern - I can fix it to have a delegate class that will do the json parsing > > and invoke the callback for each case. > > Could also make a single callback that takes a value indicating success or > failure. I don't have strong feelings about it, though the prototype of > FetchFromLog is looking pretty ugly now. :( Should be fixed now that I've switched to using a proper class hierarchy for handling the various log requests. https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher_unittest.cc:102: "%s://%s/%s/ct/v1/", kLogSchema, kLogHost, kLogPathPrefix); On 2015/11/25 17:40:28, mmenke wrote: > On 2015/11/24 22:53:38, Eran Messeri wrote: > > On 2015/11/18 19:25:16, mmenke wrote: > > > I don't think we want logic in here that matches the logic in production > code > > to > > > deduce the expected URL. Suggest adding a setter, and explicitly calling it > > in > > > tests. > > > > > > Alternatively, could make an accessor for the URL we end up fetching, and > put > > > the EXPECT in the test body (Or in RunGetConsistencyFetcherWithCallback / > > > RunFetcherWithCallback) - think that makes for clearer tests. > > > > Done - by adding a setter, as the MaybeInterceptRequest method is const, I > can't > > change any fields. > > You can always use mutable. Anyhow, fine as-is. Acknowledged. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:45: enum LogRequestType { SIGNED_TREE_HEAD, CONSISTENCY_PROOF }; On 2015/11/26 00:50:09, Ryan Sleevi wrote: > Document Obsolete. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:78: DCHECK(!(sth_fetch_callback.is_null() && proof_fetch_callback.is_null())); On 2015/11/26 00:50:09, Ryan Sleevi wrote: > Past experience has shown mixing state objects like this is generally a design > smell and a recipe for bugs. > > I imagine there will be further, future requests to make of the log - either to > handle V1 responses or for more V2 queries, right? > > This makes me wonder whether this should be re-structured somewhat: > a) Does LogProofFetcher need to be a URLRequestDelegate itself? Could it defer > the actual requesting to an inner class (e.g. FetchState) to be the Delegate? > b) It seems like FetchState is really FetchAndParseState. Would it make more > sense for FetchState to have a singular callback that it calls, with the fully > assembled response, and then that callback can take appropriate action? Currying > could help here. > > I'm a little concerned about the size and amount of logic growing in the class > here - there's probably a few things we can do to simplify, but perhaps in a > separate CL. Agreed - this is indeed a smell. I've completely changed the FetchState and added a LogRequest class hierarchy, where subclasses provide the URL to fetch from and interpret the parsed JSON. Should be much simpler to read now. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:107: size_t old_tree_size, On 2015/11/26 00:50:09, Ryan Sleevi wrote: > BUG: I'm fairly sure these should be uint64_t types, and not size_t. 32-bit > clients shouldn't be limited to 32-bit entries from logs, right? Correct, fixed. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:137: } On 2015/11/25 17:40:28, mmenke wrote: > optional: May be cleaner as an argument...Or maybe not. Obsolete - see restructuring of the code. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:140: // Creates a new request with the right flags for log requests. On 2015/11/26 00:50:09, Ryan Sleevi wrote: > "with the right flags" is not very clear/helpful. Done - completely changed the documentation as this method does more than just create the request now. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:141: enum InterceptedRequestType { On 2015/11/25 17:40:29, mmenke wrote: > RequestType -> ResultType? I don't think a failure or nothing is really a type > of request. Done. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:189: InterceptedRequestType intercepted_request_type() { return request_type_; } On 2015/11/25 17:40:29, mmenke wrote: > intercepted_request_type() const Done. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:195: const net::ct::SignedTreeHead& intercepted_sth() { return sth_; } On 2015/11/25 17:40:29, mmenke wrote: > const Done. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:197: const std::string& intercepted_log_id() { return log_id_; } On 2015/11/25 17:40:29, mmenke wrote: > const Done. https://codereview.chromium.org/1405293009/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:199: const std::vector<std::string>& intercepted_proof() { On 2015/11/25 17:40:29, mmenke wrote: > const Done.
Switched to using a RunLoop - it's now ready for another look.
Suggestion: Get rid of FetchState. Make LogRequest own everything (Including the request itself) and drive requests. Think this makes for a cleaner architecture. |inflight_requests_| could also then be a std::vector<scoped_ptr<FetchState>> (Or a std::list), which makes ownership a bit clearer. https://codereview.chromium.org/1405293009/diff/140001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:47: LogRequest(const LogProofFetcher::FetchFailedCallback& failure_callback); explicit https://codereview.chromium.org/1405293009/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:60: LogProofFetcher::FetchFailedCallback failure_callback_; const? https://codereview.chromium.org/1405293009/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:139: FetchState(const std::string& log_id, LogRequest* request_handler); Do we really need both a FetchState and a LogRequest? Seems like the request itself could store everything.
Matt, PTAL. In patchset 9 I've introduced the LogFetcher, which implements URLRequest::Delegate and abstracts interaction with URLRequest. Once the request is fulfilled successfully (or errors), it calls back to the LogProofFetcher which will invoke the SafeJsonParser and hand out the parsing result (whether parsed json or error) to the LogResponseHandler. I ended up with this design for several reasons: (1) Keeps conceptual separation between handling the URLRequest and CT-specific response handling. For example, a network error surfaced from URLRequest can potentially translate to different NET errors depending on the request made to the log. (2) Makes reasoning about object lifetime easier: The LogProofFetcher owns the LogFetcher, LogResponseHandler and manager their lifetime. (3) Each log request type is handled by a different subclass; avoiding the "poor man's inheritance" pattern that this CL originally had. https://codereview.chromium.org/1405293009/diff/140001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:47: LogRequest(const LogProofFetcher::FetchFailedCallback& failure_callback); On 2015/12/01 16:23:49, mmenke wrote: > explicit N/A anymore. https://codereview.chromium.org/1405293009/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:60: LogProofFetcher::FetchFailedCallback failure_callback_; On 2015/12/01 16:23:49, mmenke wrote: > const? Done. https://codereview.chromium.org/1405293009/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:139: FetchState(const std::string& log_id, LogRequest* request_handler); On 2015/12/01 16:23:49, mmenke wrote: > Do we really need both a FetchState and a LogRequest? Seems like the request > itself could store everything. As discussed offline, I think the separation between handling the interaction with URLRequest and dealing with the fully-assembled response should be kept for clarity. Now LogFetcher (which replaces FetchState) implements URLRequest::Delegate and handles assembling the response, the LogResponseHandler handles request-specific parsing.
Just a few rounds of drive-by design comments. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:71: virtual void HandleNetFailure(int net_error, int http_response_code); nit: s/Net/Network/ https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:108: if (net::ct::FillSignedTreeHead(parsed_json, &signed_tree_head)) { nit: restructure this so errors first? if (!net::ct::....) { failure_callback_.Run(...); return; } sth_fetched_.Run(...) Note: In general, invoking callback_.Run(), where |callback_| is a member, is a highly error-prone pattern, particularly for cases of potential re-entrency. For this reason, I generally encourage people to use something like either PostTask or, alternatively, base::ResetAndReturn(&callback_).Run(...) https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:141: if (net::ct::FillConsistencyProof(parsed_json, &consistency_proof)) { same comments re: error handling https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:173: // automatically. I'm having trouble trying to figure out what you're saying here. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:196: // Invokes the failure callback with the supplied error information. newline between 195 and 196 https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:219: LogProofFetcher::kMaxLogResponseSizeInBytes)) { Is this really sufficient? I didn't see any comments explaining how this value was decided or when to grow it. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:237: if (!status.is_success() || request->GetResponseCode() != net::HTTP_OK) { mmenke: Is this right? What about situations of cached responses or any other 2XX series. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:244: << " http response code: " << http_response_code; Seems unnecessary; the NetLog will already have this https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:267: if (bytes_read == -1 || !url_request_->status().is_success()) { Shouldn't this be bytes_read < 0 ? https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:287: DVLOG(1) << "Have " << bytes_read << " bytes to assemble."; Unnecessary? https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:289: assembled_response_.append(response_buffer_->data(), bytes_read); Suggestion: Alternatively write this as if (bytes_read > LogProofFetcher::kMaxLogResponseSizeInBytes || (LogProofFetcher::kMaxLogResponseSizeInBytes - bytes_read) < assembled_response_.size()) { ... } The reason for doing this is that it prevents you from doing .append() with the 'attacker' controlled size, potentially growing beyond what you're desiring before you do the check. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:304: url_request_->Read(response_buffer_.get(), response_buffer_->size(), 1) Why aren't you checking this return code? 2) Why aren't you decreasing what you present as the max_bytes to read? https://codereview.chromium.org/1405293009/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:17: #include "url/gurl.h" You don't need this; Alternatively, delete line 33, but pretty sure you don't need this :)
https://codereview.chromium.org/1405293009/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:71: virtual void HandleNetFailure(int net_error, int http_response_code); On 2015/12/08 01:13:52, Ryan Sleevi wrote: > nit: s/Net/Network/ net may actually be more accurate - net::kErrorDomain is "net". https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:237: if (!status.is_success() || request->GetResponseCode() != net::HTTP_OK) { On 2015/12/08 01:13:52, Ryan Sleevi wrote: > mmenke: Is this right? What about situations of cached responses or any other > 2XX series. Redirects don't result in calling this method - they're automatically followed (Should they be, for these requests). 304s are replaced with the 200 or 3xx redirect when we're reading from cache. So this should be fine, unless there's some reason we don't want a cached response, in which case we need a load flag to prevent that. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:304: url_request_->Read(response_buffer_.get(), response_buffer_->size(), On 2015/12/08 01:13:52, Ryan Sleevi wrote: > 1) Why aren't you checking this return code? url_request_->Read response codes are really weird and confusing. ResourceLoader, the primary consumer of the class, does the same thing. Patches welcome! I'd really like to have it return an error code, like net does internally everywhere (And maybe take a separate pointer to populate with the number of bytes read, if any, rather than using positive numbers to indicate success), and get rid of URLRequest::status(). > 2) Why aren't you decreasing what you present as the max_bytes to read? We should have a test for that, if we don't already. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:129: std::map<LogFetcher*, LogResponseHandler*> inflight_fetches_; I still think you should have the handler own the fetcher. Having this class deal with transferring data from one to the other seems weird. It also requires 2 or 3 extra methods, and using a map rather than a vector (Maybe it's just my own bias, but a map that owns the objects pointed to be the keys seems to have a bit of code smell).
Addressed all comments, PTAL. Regarding changes tho URLRequest usage code, I suggest any issues be addressed in a follow-up change as (1) the URLRequest-related code was simply moved and this CL and (2) this CL has seen quite a few revisions, I'd like it to converge so as to not block other CLs. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:71: virtual void HandleNetFailure(int net_error, int http_response_code); On 2015/12/08 17:10:12, mmenke wrote: > On 2015/12/08 01:13:52, Ryan Sleevi wrote: > > nit: s/Net/Network/ > > net may actually be more accurate - net::kErrorDomain is "net". Leaving as mmenke suggested. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:108: if (net::ct::FillSignedTreeHead(parsed_json, &signed_tree_head)) { On 2015/12/08 01:13:52, Ryan Sleevi wrote: > nit: restructure this so errors first? > > if (!net::ct::....) { > failure_callback_.Run(...); > return; > } > > sth_fetched_.Run(...) > > > Note: In general, invoking callback_.Run(), where |callback_| is a member, is a > highly error-prone pattern, particularly for cases of potential re-entrency. For > this reason, I generally encourage people to use something like either PostTask > or, alternatively, base::ResetAndReturn(&callback_).Run(...) > Done - switched to using base::ResetAndReturn and switched error handling order. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:141: if (net::ct::FillConsistencyProof(parsed_json, &consistency_proof)) { On 2015/12/08 01:13:52, Ryan Sleevi wrote: > same comments re: error handling Done. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:173: // automatically. On 2015/12/08 01:13:52, Ryan Sleevi wrote: > I'm having trouble trying to figure out what you're saying here. Removed the comment ; I wanted to say there's no need to manually/explicitly delete anything in the d'tor, but it's obvious from the empty body. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:196: // Invokes the failure callback with the supplied error information. On 2015/12/08 01:13:52, Ryan Sleevi wrote: > newline between 195 and 196 N/A anymore. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:219: LogProofFetcher::kMaxLogResponseSizeInBytes)) { On 2015/12/08 01:13:52, Ryan Sleevi wrote: > Is this really sufficient? I didn't see any comments explaining how this value > was decided or when to grow it. Good point - added documentation to the constant where it's defined. Turns out more bytes are needed to accommodate consistency proofs between large trees. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:237: if (!status.is_success() || request->GetResponseCode() != net::HTTP_OK) { On 2015/12/08 17:10:12, mmenke wrote: > On 2015/12/08 01:13:52, Ryan Sleevi wrote: > > mmenke: Is this right? What about situations of cached responses or any other > > 2XX series. > > Redirects don't result in calling this method - they're automatically followed > (Should they be, for these requests). 304s are replaced with the 200 or 3xx > redirect when we're reading from cache. So this should be fine, unless there's > some reason we don't want a cached response, in which case we need a load flag > to prevent that. Cached responses are fine for all replies from the log: * For get-sth, cached responses are fine as long as cache expiration dates are honoured. The log may use HTTP caching headers to tell clients there's no point in asking for a new STH until the cache expires. * For get-consistency-proof the reply for a particular request (where the old and new tree sizes are specified) is never going to change. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:244: << " http response code: " << http_response_code; On 2015/12/08 01:13:52, Ryan Sleevi wrote: > Seems unnecessary; the NetLog will already have this Done. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:267: if (bytes_read == -1 || !url_request_->status().is_success()) { On 2015/12/08 01:13:52, Ryan Sleevi wrote: > Shouldn't this be bytes_read < 0 ? Done. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:287: DVLOG(1) << "Have " << bytes_read << " bytes to assemble."; On 2015/12/08 01:13:52, Ryan Sleevi wrote: > Unnecessary? Removed, although I originally added it to identify the code has reached a particular flow to handle partial response. Arguably this is not necessary anymore because the code works :) https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:289: assembled_response_.append(response_buffer_->data(), bytes_read); On 2015/12/08 01:13:52, Ryan Sleevi wrote: > Suggestion: Alternatively write this as > > if (bytes_read > LogProofFetcher::kMaxLogResponseSizeInBytes || > (LogProofFetcher::kMaxLogResponseSizeInBytes - bytes_read) < > assembled_response_.size()) { > ... > } > > The reason for doing this is that it prevents you from doing .append() with the > 'attacker' controlled size, potentially growing beyond what you're desiring > before you do the check. Good point, done. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:304: url_request_->Read(response_buffer_.get(), response_buffer_->size(), On 2015/12/08 17:10:12, mmenke wrote: > On 2015/12/08 01:13:52, Ryan Sleevi wrote: > > 1) Why aren't you checking this return code? > > url_request_->Read response codes are really weird and confusing. > ResourceLoader, the primary consumer of the class, does the same thing. Patches > welcome! > > I'd really like to have it return an error code, like net does internally > everywhere (And maybe take a separate pointer to populate with the number of > bytes read, if any, rather than using positive numbers to indicate success), and > get rid of URLRequest::status(). > > > 2) Why aren't you decreasing what you present as the max_bytes to read? > > We should have a test for that, if we don't already. There is a test case covering a response that's too big: TEST_F(LogProofFetcherTest, TestLogReplyIsTooLong I did not decrease the value presented as max_bytes for code simplicity: - Right now handling of a response that's bigger than kMaxLogResponseSizeInBytes is done in HandleReadResult. - If I were to do decrease max_bytes presented here I'd have to check if the assembled_response_ size is exactly kMaxLogResponseSizeInBytes, otherwise I'd be passing 0 as max_bytes to read. - That will happen when the response is too big: The first kMaxLogResponseSizeInBytes bytes will be read and put into the assembled_response_. Another Read() must be performed anyway to determine if the response was *exactly* kMaxLogResponseSizeInBytes or more. Two options: - Leaving the code as-is: The extra Read is performed the same way all other reads do, with the same buffer size. Advantages: Same code path, easier to follow. Disadvantages: We may consume at most 2 * kMaxLogResponseSizeInBytes when the response is too big. - Special-case handling here to resize the response_buffer_ to 1 bytes just to detect if the response is beyond kMaxLogResponesSizeInBytes. I argue that for code simplicity it's OK not to do special-case handling here for the unlikely event that we'll use 2*kMaxLogResponseSizeInBytes bytes before disposing of everything and invoking the error callback. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:17: #include "url/gurl.h" On 2015/12/08 01:13:52, Ryan Sleevi wrote: > You don't need this; > > Alternatively, delete line 33, but pretty sure you don't need this :) Done. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:129: std::map<LogFetcher*, LogResponseHandler*> inflight_fetches_; On 2015/12/08 17:10:12, mmenke wrote: > I still think you should have the handler own the fetcher. Having this class > deal with transferring data from one to the other seems weird. > > It also requires 2 or 3 extra methods, and using a map rather than a vector > (Maybe it's just my own bias, but a map that owns the objects pointed to be the > keys seems to have a bit of code smell). Done - now the LogProofFetcher has a set of LogResponseHandlers and only gets a callback once everything is done, to dispose of the LogResponseHandler.
Also FYI the asan failure is likely fixed by https://codereview.chromium.org/1527063002/
A couple nitty comments I hadn't yet published. https://codereview.chromium.org/1405293009/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:304: url_request_->Read(response_buffer_.get(), response_buffer_->size(), On 2015/12/14 13:01:42, Eran Messeri wrote: > On 2015/12/08 17:10:12, mmenke wrote: > > On 2015/12/08 01:13:52, Ryan Sleevi wrote: > > > 1) Why aren't you checking this return code? > > > > url_request_->Read response codes are really weird and confusing. > > ResourceLoader, the primary consumer of the class, does the same thing. > Patches > > welcome! > > > > I'd really like to have it return an error code, like net does internally > > everywhere (And maybe take a separate pointer to populate with the number of > > bytes read, if any, rather than using positive numbers to indicate success), > and > > get rid of URLRequest::status(). > > > > > 2) Why aren't you decreasing what you present as the max_bytes to read? > > > > We should have a test for that, if we don't already. > > There is a test case covering a response that's too big: > TEST_F(LogProofFetcherTest, TestLogReplyIsTooLong > > I did not decrease the value presented as max_bytes for code simplicity: > - Right now handling of a response that's bigger than kMaxLogResponseSizeInBytes > is done in HandleReadResult. > - If I were to do decrease max_bytes presented here I'd have to check if the > assembled_response_ size is exactly kMaxLogResponseSizeInBytes, otherwise I'd be > passing 0 as max_bytes to read. > - That will happen when the response is too big: The first > kMaxLogResponseSizeInBytes bytes will be read and put into the > assembled_response_. Another Read() must be performed anyway to determine if the > response was *exactly* kMaxLogResponseSizeInBytes or more. > > Two options: > - Leaving the code as-is: The extra Read is performed the same way all other > reads do, with the same buffer size. Advantages: Same code path, easier to > follow. Disadvantages: We may consume at most 2 * kMaxLogResponseSizeInBytes > when the response is too big. > - Special-case handling here to resize the response_buffer_ to 1 bytes just to > detect if the response is beyond kMaxLogResponesSizeInBytes. > > I argue that for code simplicity it's OK not to do special-case handling here > for the unlikely event that we'll use 2*kMaxLogResponseSizeInBytes bytes before > disposing of everything and invoking the error callback. Ah, right...we append to a string after each read, instead of continuing to read into the buffer where the last read ended. I'm not too concerned about the extra memory + CPU cost of those appends. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:67: const std::string& assembled_response() { return assembled_response_; } assembled_response() const { https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:170: // bytes_read is non-negative at this point, casting to size_t should be We generally use |bytes_read| to refer to arguments. Google style guide no longer requires it, though. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:400: const GURL request_url = base_log_url.Resolve("ct/v1/get-sth"); nit: const is typically not used in these situations. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:412: const GURL request_url = base_log_url.Resolve(base::StringPrintf( nit: const is typically not used in these situations. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:16: #include "net/url_request/url_request.h" Can move this into the CC file. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:98: uint64_t old_tree_size, include <stdint.h> https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:123: DISALLOW_COPY_AND_ASSIGN(LogProofFetcher); nit: Blank line before DISALLOW_COPY_AND_ASSIGN
Addressed all comments. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:67: const std::string& assembled_response() { return assembled_response_; } On 2015/12/16 15:45:28, mmenke wrote: > assembled_response() const { Done. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:170: // bytes_read is non-negative at this point, casting to size_t should be On 2015/12/16 15:45:28, mmenke wrote: > We generally use |bytes_read| to refer to arguments. Google style guide no > longer requires it, though. Done. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:400: const GURL request_url = base_log_url.Resolve("ct/v1/get-sth"); On 2015/12/16 15:45:28, mmenke wrote: > nit: const is typically not used in these situations. Done. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:412: const GURL request_url = base_log_url.Resolve(base::StringPrintf( On 2015/12/16 15:45:28, mmenke wrote: > nit: const is typically not used in these situations. Done. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:16: #include "net/url_request/url_request.h" On 2015/12/16 15:45:28, mmenke wrote: > Can move this into the CC file. Done. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:98: uint64_t old_tree_size, On 2015/12/16 15:45:28, mmenke wrote: > include <stdint.h> Done. https://codereview.chromium.org/1405293009/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:123: DISALLOW_COPY_AND_ASSIGN(LogProofFetcher); On 2015/12/16 15:45:28, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN Done.
mmenke/rsleevi: Pre-holiday ping (one can always hope).
Post-holidays, Post Real World Crypto ping.
https://codereview.chromium.org/1405293009/diff/220001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:58: LogFetcher(net::URLRequestContext* request_context, nit: newline between 57 & 58 https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:84: // Invokes the failure callback with the supplied error information. nit: newline between 83 and 84? https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:85: // After this method the LogFetcher instance may be deleted and |this| After this method what? (I believe you mean "After this method is called, the LogFetcher instance may be deleted") However, that's a sort of weak API contract - it implies there are cases where it won't be deleted. And that makes it ambiguous on how to safely call this. // After this method is called, the LogFetcher is deleted and no longer safe to call But if that's the case, then Line 62 should likely be moved into private and we should have a stronger contract on what the API is. (Hopefully it's clearer the ambiguity of lifetime; I realize this is all localized to a .cc, just hoping we can provide strong guarantees one way or another as to what the lifetime and validity of one of these objects is) https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:111: // This request should not send any cookies or otherwise identifying data grammar: s/data/data, / https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:123: net::URLRequestStatus status(request->status()); Do you actually need to clone status? It seems like you're really just doing bool had_error = !request->status().is_success() if (had_error || ...) { .. if (had_error) net_error = ... } StartNextRead(); https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:152: net::URLRequestStatus status(url_request_->status()); This seems unnecessarily verbose - do you really need the temporary? https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:170: DCHECK_GE(bytes_read, 0); Why DCHECK here, given line 173? https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:300: base::ResetAndReturn(&done_callback_).Run(this); DESIGN: This strikes me weird that we would invoke both failure_callback_ and done_callback_, and is usually a source of bugs (invoking multiple callbacks simultaneously, where one callback may lead to the deletion of |this|) I think it'd be easier to reason about if you only had one callback being invoked out from the edge of the graph. You could also stack-copy the callback (in this case, done_callback_), but I've also seen errors come from that (double frees), so I'm loathe to encourage it. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:312: base::ResetAndReturn(&done_callback_).Run(this); Both of these methods also seem to suffer from the same double-callback exit (error & done) It's further compounded that you're using ResetAndReturn in those methods, except you should be *guaranteed* that's unnecessary because you access the done_callback_ in these methods, so you know the |this| cannot be invalidated in HandleJsonParseFailure nor HandleParsedJson, since neither of those callbacks is permitted to affect |this| in any way (by virtue of reasoning about these methods) https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:436: STLDeleteContainerPointers(it, next); This (line 433-436) just seems... needlessly complex? What's it trying to defend against? Why not just auto it = .. DCHECK(it != ) delete *it; inflight_fetches_.erase(it); https://codereview.chromium.org/1405293009/diff/220001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:108: class LogFetcher; You don't need the declarations on line 106-108, AFAICT. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:110: // Starts the fetch (by delegating to the LogResponseHandler) typo: by delegating/by delegating/ https://codereview.chromium.org/1405293009/diff/220001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:199: void SetRunLoop(base::RunLoop* run_loop) { run_loop_ = run_loop; } Suggestion: You could just make your fetch callbacks (ConsistencyProofFetched, FetchingFailed, etc) take a parameter, which is a base::Closure quit_closure. When binding (e.g. line 263, 264), just curry the QuitClosure from the RunLoop e.g. base::Bind(&RecordFetchCallbackInvocations::STHFetched, base::Unretained(callback), RunLoop.QuitClosure()) https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:253: RecordFetchCallbackInvocations* callback) { NAMING: This is more a remark about the test in general, but I would say naming it "callback" seems a bit overloaded, because this isn't a base::Callback but a helper class. This is a remark for a separate CL though - I would say enough pre-existing code exists that it's not worth trying to fix all in *this* CL, but something that stood out to me when I was trying to figure out why base::Unretained(callback) existed, which surprised me.
I just reviewed the use of URLRequest / implementation of URLRequest::Delegate, and that LGTM. Be sure to wait for a full signoff from sleevi before landing. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:39: return net::ERR_ABORTED; This is fine, but it's probably overkill. The only things that can cancel the request are this class and the ChromeNetworkDelegate...which can do weird things, but I think it's fine to just do: if (status.success()) return net::OK; return status.error(); https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:107: LogProofFetcher::kMaxLogResponseSizeInBytes)) { Optional: Could initialize this only once we receive the headers. Reduce the length of time we use the memory for. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:130: net_error = GetNetErrorFromURLRequestStatus(status); Get rid of this, and use GetNetErrorFromURLRequestStatus(...)? https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:186: void LogProofFetcher::LogFetcher::StartNextRead() { ReadLoop(), maybe?
Addressed all your comments, PTAL. Also Valgrind is happy with my code so hopefully the simplified callbacks model is not leaky. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:39: return net::ERR_ABORTED; On 2016/01/12 22:43:20, mmenke wrote: > This is fine, but it's probably overkill. The only things that can cancel the > request are this class and the ChromeNetworkDelegate...which can do weird > things, but I think it's fine to just do: > > if (status.success()) > return net::OK; > return status.error(); Done. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:58: LogFetcher(net::URLRequestContext* request_context, On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > nit: newline between 57 & 58 Done. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:84: // Invokes the failure callback with the supplied error information. On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > nit: newline between 83 and 84? Done. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:85: // After this method the LogFetcher instance may be deleted and |this| On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > After this method what? > > (I believe you mean "After this method is called, the LogFetcher instance may be > deleted") > > However, that's a sort of weak API contract - it implies there are cases where > it won't be deleted. And that makes it ambiguous on how to safely call this. > > // After this method is called, the LogFetcher is deleted and no longer safe to > call > > But if that's the case, then Line 62 should likely be moved into private and we > should have a stronger contract on what the API is. > > (Hopefully it's clearer the ambiguity of lifetime; I realize this is all > localized to a .cc, just hoping we can provide strong guarantees one way or > another as to what the lifetime and validity of one of these objects is) Done - the LogFetcher instance is deleted either by the success or failure callbacks. I've updated the documentation and code to reflect that. I did not, however, make the d'tor private as it the LogFetcher will be deleted by the LogResponseHandler (which owns it). https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:107: LogProofFetcher::kMaxLogResponseSizeInBytes)) { On 2016/01/12 22:43:20, mmenke wrote: > Optional: Could initialize this only once we receive the headers. Reduce the > length of time we use the memory for. Done - I've moved it to OnResponseStarted, please let me know if that's not a sensible choice. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:111: // This request should not send any cookies or otherwise identifying data On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > grammar: s/data/data, / Done. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:123: net::URLRequestStatus status(request->status()); On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > Do you actually need to clone status? It seems like you're really just doing > > bool had_error = !request->status().is_success() > > if (had_error || ...) { > .. > if (had_error) > net_error = ... > } > > StartNextRead(); I've simplified the code a bit to always calculate net_error and check net_error against net::OK - which GetNetErrorFromURLRequestStatus will only return if request->status().is_success() is true. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:130: net_error = GetNetErrorFromURLRequestStatus(status); On 2016/01/12 22:43:20, mmenke wrote: > Get rid of this, and use GetNetErrorFromURLRequestStatus(...)? Done. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:152: net::URLRequestStatus status(url_request_->status()); On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > This seems unnecessarily verbose - do you really need the temporary? Done - removed temporary. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:170: DCHECK_GE(bytes_read, 0); On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > Why DCHECK here, given line 173? The checks later are to make sure we haven't read too much, not just that bytes_read is non-negative. This DCHECK enforces the contract for calling HandleReadResult - assuming bytes_read >= 0 implying that there's a valid read output to handle. Does that make sense? If it seems superfluous, I could remove this DCHECK. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:186: void LogProofFetcher::LogFetcher::StartNextRead() { On 2016/01/12 22:43:20, mmenke wrote: > ReadLoop(), maybe? Done. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:300: base::ResetAndReturn(&done_callback_).Run(this); On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > DESIGN: This strikes me weird that we would invoke both failure_callback_ and > done_callback_, and is usually a source of bugs (invoking multiple callbacks > simultaneously, where one callback may lead to the deletion of |this|) > > I think it'd be easier to reason about if you only had one callback being > invoked out from the edge of the graph. > > You could also stack-copy the callback (in this case, done_callback_), but I've > also seen errors come from that (double frees), so I'm loathe to encourage it. Per your suggestion I now have one callback being invoked out from the edge of the graph: The LogResponseHandler prepares/passes on a base::Closure that is binding the output to the success/failure callbacks. This closure is then passed into the DoneCallback as a parameter. The DoneCallback is implemented by the LogProofFetcher itself: It will delete the LogResponseHandler it owns, only then invoke the base::Closure instance prepared by the LogResponseHandler. Does this design address your concerns? https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:312: base::ResetAndReturn(&done_callback_).Run(this); On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > Both of these methods also seem to suffer from the same double-callback exit > (error & done) > > It's further compounded that you're using ResetAndReturn in those methods, > except you should be *guaranteed* that's unnecessary because you access the > done_callback_ in these methods, so you know the |this| cannot be invalidated in > HandleJsonParseFailure nor HandleParsedJson, since neither of those callbacks is > permitted to affect |this| in any way (by virtue of reasoning about these > methods) See design changes I've highlighted above. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:436: STLDeleteContainerPointers(it, next); On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > This (line 433-436) just seems... needlessly complex? > > What's it trying to defend against? Why not just > > auto it = .. > DCHECK(it != ) > delete *it; > inflight_fetches_.erase(it); Done. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:108: class LogFetcher; On 2016/01/12 22:15:01, Ryan Sleevi (OOO til 1-19) wrote: > You don't need the declarations on line 106-108, AFAICT. Where should I declare these classes, then? These are inner classes I define in the .cc file. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:110: // Starts the fetch (by delegating to the LogResponseHandler) On 2016/01/12 22:15:01, Ryan Sleevi wrote: > typo: by delegating/by delegating/ Done. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:199: void SetRunLoop(base::RunLoop* run_loop) { run_loop_ = run_loop; } On 2016/01/12 22:15:01, Ryan Sleevi wrote: > Suggestion: You could just make your fetch callbacks (ConsistencyProofFetched, > FetchingFailed, etc) take a parameter, which is a base::Closure quit_closure. > When binding (e.g. line 263, 264), just curry the QuitClosure from the RunLoop > > e.g. > > base::Bind(&RecordFetchCallbackInvocations::STHFetched, > base::Unretained(callback), RunLoop.QuitClosure()) Done. https://codereview.chromium.org/1405293009/diff/220001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:253: RecordFetchCallbackInvocations* callback) { On 2016/01/12 22:15:01, Ryan Sleevi wrote: > NAMING: This is more a remark about the test in general, but I would say naming > it "callback" seems a bit overloaded, because this isn't a base::Callback but a > helper class. > > This is a remark for a separate CL though - I would say enough pre-existing code > exists that it's not worth trying to fix all in *this* CL, but something that > stood out to me when I was trying to figure out why base::Unretained(callback) > existed, which surprised me. Acknowledged - Do I understand correctly that you suggest renaming the |callback| parameter? is |callback_recorder| more appropriate?
Friendly pre-weekend ping.
https://codereview.chromium.org/1405293009/diff/240001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:116: int net_error = GetNetErrorFromURLRequestStatus(request->status()); if status.is_success() is false, will status.error() ever be net::OK? If status.error() is set, is request->GetResponseCode() guaranteed to be anything meaningful? I feel like this helper is actually hiding logic/side-behaviours, and I wonder whether it might make more sense to write out explicitly if (!request->status.is_success()) { InvokeFailureCallback(request->status.error(), request->GetResponseCode() /* is this ever filled if request->status.is_success() is false? */); return; } if (request->GetResponseCode() != net::HTTP_OK) { InvokeFailureCallback(net::OK /*is this really what you want? */, request->GetResponseCode()); return; } Yes, it's more duplication, but it also makes it slightly clearer the exit cases. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:127: if (!response_buffer_) braces https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:146: if (bytes_read < 0 || !url_request_->status().is_success()) { This order of conditions, while likely irrelevant to the function, seems non-obvious from a self-documenting sense. Shouldn't you check for errors _before_ you then check to see if data was screwed up? It seems like the (latter condition) implies the former, but not vice-versa. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:148: GetNetErrorFromURLRequestStatus(url_request_->status()), net::OK); This is the other case where GetNetError() resulted in surprising behaviour, IMO. If bytes_read <0, and is_success() is true, this calls InvokeFailureCallback(net::OK, net::OK), which... doesn't make sense. Unless it does? That's why I'm not sure whether it makes sense to first handle the !is_success() case (where a real .error() is available), and then handle the bytes_read < 0 (if that's ever actually possible w/ is_success()), with a more meaningful explicit error code. This would also get rid of the helper entirely. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:196: // instance will be delected by the callback. typo: deleted grammar: |this| is not valid grammar: s/callback as/callback, as/ https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:203: // NOTE: |this| not valid after this callback as the LogFetcher instance grammar: |this| is not valid grammar: s/callback as/callback, as/ https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:208: // requests. grammar: Is the plurality agreement correct? Isn't this handling the response from a CT log for _a_ particular request_. ? https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:210: // to each request should be parsed and validated diffenetly. typo: differently. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:212: // LogProofFetcher instances are owned by the LogProofFetcher and are This comment seems confusingly worded. Is this a typo about LogResponseHandler's are owned by the LogProofFetcher? Or that LogProofFetchers own themselves? (which doesn't seem like it belongs here) The comment as a whole seems to be documenting the particular implementation of LogProofFetcher, and doesn't seem relevant to the LogResponseHandler (especially since the public dtor on line 226 means it's not really guaranteed that the comment isn't a lie) https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:217: const base::Closure&)>; DESIGN: While this is a .cc file, and so I'm not going to press hard on it, it does seem an odd design to pass the |this| as the first member - you're effectively binding the API contract to this specific implementation, which seems a very tight coupling. That is, an alternative example (based solely on reading this class) would seem to be that DoneCallback is just a base::Closure() (that is, remove the typedef in line 232), and the caller's responsible for lifetime management - such as using the form void Caller::MyMethod(LogResponseHandler* handler); LogResponseHandler* handler = new LogResponseHandler(...); handler->StartFetch(..., base::Bind(&Caller::MyMethod, caller_as_a_weak_ptr, base::Owned(handler))); handler = nullptr; The difference in this API contract is that it separates out the lifetime management to being the responsibility of the creator entirely, whereas here, this API contract places some restrictions (which are equally hard to discern with respect to failure_callback) Now, if you didn't go the Owned approach (perfectly reasonable), you could and should still focus on what *your* class needs rather than what *callers* need. You can still leave the ownership up to the invoker, but rather than smuggling the |this| pointer back in the callback, leave it up to the invoker to curry this classes pointer themselves, since they're responsible for lifetime management. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:220: // this failure pretains to. Which callback? failure_callback? https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:222: // request types. I have trouble parsing this, such that I'm not sure how to suggest rewording. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:229: // invocation when fetching and parsing of the request finished. It sounds like it's safe to delete this object in the |done_callback|, which seems like it should be documented as such. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:246: // Handle failure to parse the JSON by invoking HandleJsonParseFailure, then newline between 245 and 246 https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:286: LogFetcher::FailureCallback failure_callback = I'm not used to see temporaries here like this. That's not to say it's wrong, I'm just... not used to it :) Normally we base::Bind() chain directly, AFAIK. For example, you do this on lines 298-301, which is why the internal inconsistency stood out to me. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:303: // the assembled_response string is copied into the SafeJsonParser so it grammar: s/the/The/ https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:316: base::Bind(failure_callback, log_id_, net_error, http_response_code); same remark re: temporaries as arguments (bound_failure) https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:318: // NOTE: |this| is not valid after the |done_callback_| was invoked. grammar: tense agreement (is vs was) |this| is no longer valid after |done_callback_| is invoked. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:323: base::Closure bound_success_callback = HandleParsedJson(*parsed_json); same remark re: temporary https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:325: // NOTE: |this| is not valid after the |done_callback_| was invoked. same nit https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:332: // NOTE: |this| is not valid after the |done_callback_| was invoked. Same remarks to both https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:346: // successful, invoke the success callback with this STH. I originally had a grammar nit about "this STH" vs "the STH" (the this implies there are multiple instances, hence the need to specify, but there aren't)... But more generally, this seems to go against the declarative vs imperative comment guidance of https://google.github.io/styleguide/cppguide.html#Function_Comments // Parses the JSON into a net::ct::SignedTreeHead and, if successful, invokes the // success callback with it. Otherwise, invokes the failure callback indicating // the error that occurred. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:352: base::ResetAndReturn(&failure_callback_); Unnecessary temporaries (similarly throughout) https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:387: // Otherwise, invoke the failure callback indicating proof parsing has failed. Similar comment-level remarks. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:109: class LogFetcher; So LogFetcher definitely doesn't need to be pre-declared, but my suggestion was to move these from being class-level definitions to being a single forward declaration at namespace scope (since you need to have LogResponseHandler*'s as an implementation) I mean, strictly speaking, you could do the type erasure with base::Closure()'s, fully hiding the implementation, but that feels like overkill. So concrete suggestion: "class LogResponseHandler" at line 37 (with appropriate whitespace), which is solely implemented in the .cc. Remove lines 106-109. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:48: } This comment/function is not really clear. Why the static_cast to char? What makes that safe? Should it just be the low 8 bits, which at least is easier to reason about/ // Gets a dummy consistency proof for the given |node_id|. std::string ... { // Take the low 8 bits and repeat them as a string. This // has no special meaning, other than making it easier to // debug which consistency proof was used. return std::string(32, static_cast<char>(node_id & 0xFF)); } By documenting as you have on line 45, it's unclear whether or not that's an API contract you're intending or is just incidental (the Dummy implies it's incidental) https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:50: const size_t kDummyConsistencyProofLength = 4; This needs documentation. In looking how it's used, it makes me wonder whether the documentation on 45 should be expanded to note the distinction between node and proof here, since until you see how it's used (proof is kDummy...Length nodes), it's ... non-obvious?
PTAL - hopefully lifetime management is clearer and simpler now. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:116: int net_error = GetNetErrorFromURLRequestStatus(request->status()); On 2016/01/23 00:34:23, Ryan Sleevi wrote: > if status.is_success() is false, will status.error() ever be net::OK? > > If status.error() is set, is request->GetResponseCode() guaranteed to be > anything meaningful? > > I feel like this helper is actually hiding logic/side-behaviours, and I wonder > whether it might make more sense to write out explicitly > > if (!request->status.is_success()) { > InvokeFailureCallback(request->status.error(), request->GetResponseCode() /* > is this ever filled if request->status.is_success() is false? */); > return; > } > > if (request->GetResponseCode() != net::HTTP_OK) { > InvokeFailureCallback(net::OK /*is this really what you want? */, > request->GetResponseCode()); > return; > } > > Yes, it's more duplication, but it also makes it slightly clearer the exit > cases. Done - implemented two separate checks as you suggested. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:127: if (!response_buffer_) On 2016/01/23 00:34:23, Ryan Sleevi wrote: > braces Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:146: if (bytes_read < 0 || !url_request_->status().is_success()) { On 2016/01/23 00:34:23, Ryan Sleevi wrote: > This order of conditions, while likely irrelevant to the function, seems > non-obvious from a self-documenting sense. Shouldn't you check for errors > _before_ you then check to see if data was screwed up? It seems like the (latter > condition) implies the former, but not vice-versa. Swapped ordering. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:148: GetNetErrorFromURLRequestStatus(url_request_->status()), net::OK); On 2016/01/23 00:34:23, Ryan Sleevi wrote: > This is the other case where GetNetError() resulted in surprising behaviour, > IMO. > > If bytes_read <0, and is_success() is true, this calls > InvokeFailureCallback(net::OK, net::OK), which... doesn't make sense. Unless it > does? > > That's why I'm not sure whether it makes sense to first handle the !is_success() > case (where a real .error() is available), and then handle the bytes_read < 0 > (if that's ever actually possible w/ is_success()), with a more meaningful > explicit error code. > > This would also get rid of the helper entirely. Fair point. I've checked (in this if clause) if read_bytes < 0 and if so changed the error passed to InvokeFailureCallback to be net::URLRequestStatus::FAILED (because a more detailed failure information is not available). https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:196: // instance will be delected by the callback. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > typo: deleted > grammar: |this| is not valid > grammar: s/callback as/callback, as/ Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:203: // NOTE: |this| not valid after this callback as the LogFetcher instance On 2016/01/23 00:34:22, Ryan Sleevi wrote: > grammar: |this| is not valid > grammar: s/callback as/callback, as/ Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:208: // requests. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > grammar: Is the plurality agreement correct? Isn't this handling the response > from a CT log for _a_ particular request_. > > ? Good point, fixed to be explicitly singular. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:210: // to each request should be parsed and validated diffenetly. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > typo: differently. Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:212: // LogProofFetcher instances are owned by the LogProofFetcher and are On 2016/01/23 00:34:22, Ryan Sleevi wrote: > This comment seems confusingly worded. > > Is this a typo about LogResponseHandler's are owned by the LogProofFetcher? Or > that LogProofFetchers own themselves? (which doesn't seem like it belongs here) > > The comment as a whole seems to be documenting the particular implementation of > LogProofFetcher, and doesn't seem relevant to the LogResponseHandler (especially > since the public dtor on line 226 means it's not really guaranteed that the > comment isn't a lie) (1) Yes, there's a typo. (2) I've changed the documentation to clarify that LogResponseHandler instances are expected to be deleted by the DoneCallback. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:217: const base::Closure&)>; On 2016/01/23 00:34:23, Ryan Sleevi wrote: > DESIGN: While this is a .cc file, and so I'm not going to press hard on it, it > does seem an odd design to pass the |this| as the first member - you're > effectively binding the API contract to this specific implementation, which > seems a very tight coupling. > > That is, an alternative example (based solely on reading this class) would seem > to be that DoneCallback is just a base::Closure() (that is, remove the typedef > in line 232), and the caller's responsible for lifetime management - such as > using the form > > void Caller::MyMethod(LogResponseHandler* handler); > > LogResponseHandler* handler = new LogResponseHandler(...); > handler->StartFetch(..., base::Bind(&Caller::MyMethod, caller_as_a_weak_ptr, > base::Owned(handler))); > handler = nullptr; > > The difference in this API contract is that it separates out the lifetime > management to being the responsibility of the creator entirely, whereas here, > this API contract places some restrictions (which are equally hard to discern > with respect to failure_callback) > > Now, if you didn't go the Owned approach (perfectly reasonable), you could and > should still focus on what *your* class needs rather than what *callers* need. > You can still leave the ownership up to the invoker, but rather than smuggling > the |this| pointer back in the callback, leave it up to the invoker to curry > this classes pointer themselves, since they're responsible for lifetime > management. I've dropped the LogResponseHandler* from the DoneCallback. I've still left a typedef for it as it receives one parameter - the callback to invoke after deleting the LogProofFetcher instance (that callback is prepared by the LogResponseHandler, with the result of the fetch, so cannot be another bound parameter to DoneCallback, thus preventing it from being a base::Closure). https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:220: // this failure pretains to. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > Which callback? failure_callback? Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:222: // request types. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > I have trouble parsing this, such that I'm not sure how to suggest rewording. This was meant to be a documentation of a design decision - that the |failure_callback| is in the base class because all types of requests have to handle failure. I've simply removed this comment. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:229: // invocation when fetching and parsing of the request finished. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > It sounds like it's safe to delete this object in the |done_callback|, which > seems like it should be documented as such. Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:246: // Handle failure to parse the JSON by invoking HandleJsonParseFailure, then On 2016/01/23 00:34:23, Ryan Sleevi wrote: > newline between 245 and 246 Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:286: LogFetcher::FailureCallback failure_callback = On 2016/01/23 00:34:22, Ryan Sleevi wrote: > I'm not used to see temporaries here like this. That's not to say it's wrong, > I'm just... not used to it :) Normally we base::Bind() chain directly, AFAIK. > > For example, you do this on lines 298-301, which is why the internal > inconsistency stood out to me. Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:303: // the assembled_response string is copied into the SafeJsonParser so it On 2016/01/23 00:34:23, Ryan Sleevi wrote: > grammar: s/the/The/ Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:316: base::Bind(failure_callback, log_id_, net_error, http_response_code); On 2016/01/23 00:34:23, Ryan Sleevi wrote: > same remark re: temporaries as arguments (bound_failure) Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:318: // NOTE: |this| is not valid after the |done_callback_| was invoked. On 2016/01/23 00:34:22, Ryan Sleevi wrote: > grammar: tense agreement (is vs was) > > |this| is no longer valid after |done_callback_| is invoked. Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:323: base::Closure bound_success_callback = HandleParsedJson(*parsed_json); On 2016/01/23 00:34:23, Ryan Sleevi wrote: > same remark re: temporary Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:325: // NOTE: |this| is not valid after the |done_callback_| was invoked. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > same nit Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:332: // NOTE: |this| is not valid after the |done_callback_| was invoked. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > Same remarks to both Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:346: // successful, invoke the success callback with this STH. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > I originally had a grammar nit about "this STH" vs "the STH" (the this implies > there are multiple instances, hence the need to specify, but there aren't)... > > But more generally, this seems to go against the declarative vs imperative > comment guidance of > https://google.github.io/styleguide/cppguide.html#Function_Comments > > // Parses the JSON into a net::ct::SignedTreeHead and, if successful, invokes > the > // success callback with it. Otherwise, invokes the failure callback indicating > // the error that occurred. Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:352: base::ResetAndReturn(&failure_callback_); On 2016/01/23 00:34:22, Ryan Sleevi wrote: > Unnecessary temporaries (similarly throughout) Done throughout. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:387: // Otherwise, invoke the failure callback indicating proof parsing has failed. On 2016/01/23 00:34:23, Ryan Sleevi wrote: > Similar comment-level remarks. Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:109: class LogFetcher; On 2016/01/23 00:34:23, Ryan Sleevi wrote: > So LogFetcher definitely doesn't need to be pre-declared, but my suggestion was > to move these from being class-level definitions to being a single forward > declaration at namespace scope (since you need to have LogResponseHandler*'s as > an implementation) > > I mean, strictly speaking, you could do the type erasure with base::Closure()'s, > fully hiding the implementation, but that feels like overkill. > > So concrete suggestion: > > "class LogResponseHandler" at line 37 (with appropriate whitespace), which is > solely implemented in the .cc. Remove lines 106-109. Done (here and in the cc, obviously) https://codereview.chromium.org/1405293009/diff/240001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:48: } On 2016/01/23 00:34:23, Ryan Sleevi wrote: > This comment/function is not really clear. Why the static_cast to char? What > makes that safe? Should it just be the low 8 bits, which at least is easier to > reason about/ > > // Gets a dummy consistency proof for the given |node_id|. > std::string ... { > // Take the low 8 bits and repeat them as a string. This > // has no special meaning, other than making it easier to > // debug which consistency proof was used. > return std::string(32, static_cast<char>(node_id & 0xFF)); > } > > By documenting as you have on line 45, it's unclear whether or not that's an API > contract you're intending or is just incidental (the Dummy implies it's > incidental) Done. https://codereview.chromium.org/1405293009/diff/240001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:50: const size_t kDummyConsistencyProofLength = 4; On 2016/01/23 00:34:23, Ryan Sleevi wrote: > This needs documentation. In looking how it's used, it makes me wonder whether > the documentation on 45 should be expanded to note the distinction between node > and proof here, since until you see how it's used (proof is kDummy...Length > nodes), it's ... non-obvious? I agree it's non-obvious what this value is for in particular, until you see how it's used. Renamed to kDummyConsistencyProofNumNodes - is it clearer?
https://codereview.chromium.org/1405293009/diff/260001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/260001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:142: net_error = net::URLRequestStatus::FAILED; If bytes_read is < 0, net_error really should indicate an error. I don't trust the API to force URLRequestJob subclasses to currently get that right (grumble...grumble...Working on it...) Anyhow, I'd suggest: if (net_error == net::OK) net_error = ... https://codereview.chromium.org/1405293009/diff/260001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:144: InvokeFailureCallback(net_error, net::OK); BUG: net::OK -> net::HTTP_OK. Should have a test for this case that catches that.
https://codereview.chromium.org/1405293009/diff/260001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/260001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:142: net_error = net::URLRequestStatus::FAILED; On 2016/01/25 17:41:39, mmenke wrote: > If bytes_read is < 0, net_error really should indicate an error. I don't trust > the API to force URLRequestJob subclasses to currently get that right > (grumble...grumble...Working on it...) > > Anyhow, I'd suggest: > > if (net_error == net::OK) > net_error = ... Done. https://codereview.chromium.org/1405293009/diff/260001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:144: InvokeFailureCallback(net_error, net::OK); On 2016/01/25 17:41:39, mmenke wrote: > BUG: net::OK -> net::HTTP_OK. Should have a test for this case that catches > that. I've added assertions in the test that the http_response_code is HTTP_OK. However I documented my way out of this because when net_error != net::OK, the second parameter (http_response_code) is not meaningful. I've made a note of that in log_proof_fetcher.h.
Ping.
I've got one nit. To be honest, I still struggle a bit with the lifetime management, perhaps because there's so many ways for the object to end up in a terminal state where the object is deleted. I'm torn on this. I can't make sense of it, having read it twice today, and while I want to believe that's my own stupidity, it concerns me for the code health if I have so much trouble with it. Because Matt and Steven are happy with it (so I'm assuming PEBKAC), and because I don't have concrete suggestions on how to resolve this, I'll LG it... but I won't lie, I feel guilty doing so, because I don't know how maintainable this code abstraction is. https://codereview.chromium.org/1405293009/diff/280001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/280001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:159: // We have data, collect it and indicate another read is needed. Pedantry: Avoid pronouns in comments (who is "we" here) // Data is available; collect it and indicate another read is needed.
On 2016/02/05 02:28:32, Ryan Sleevi wrote: > I've got one nit. > > To be honest, I still struggle a bit with the lifetime management, perhaps > because there's so many ways for the object to end up in a terminal state where > the object is deleted. > > I'm torn on this. I can't make sense of it, having read it twice today, and > while I want to believe that's my own stupidity, it concerns me for the code > health if I have so much trouble with it. Because Matt and Steven are happy with > it (so I'm assuming PEBKAC), and because I don't have concrete suggestions on > how to resolve this, I'll LG it... but I won't lie, I feel guilty doing so, > because I don't know how maintainable this code abstraction is. Hrm...We could switch to a DoLoop. > https://codereview.chromium.org/1405293009/diff/280001/components/certificate... > File components/certificate_transparency/log_proof_fetcher.cc (right): > > https://codereview.chromium.org/1405293009/diff/280001/components/certificate... > components/certificate_transparency/log_proof_fetcher.cc:159: // We have data, > collect it and indicate another read is needed. > Pedantry: Avoid pronouns in comments (who is "we" here) > > // Data is available; collect it and indicate another read is needed.
On 2016/02/05 02:43:06, mmenke wrote: > On 2016/02/05 02:28:32, Ryan Sleevi wrote: > > I've got one nit. > > > > To be honest, I still struggle a bit with the lifetime management, perhaps > > because there's so many ways for the object to end up in a terminal state > where > > the object is deleted. > > > > I'm torn on this. I can't make sense of it, having read it twice today, and > > while I want to believe that's my own stupidity, it concerns me for the code > > health if I have so much trouble with it. Because Matt and Steven are happy > with > > it (so I'm assuming PEBKAC), and because I don't have concrete suggestions on > > how to resolve this, I'll LG it... but I won't lie, I feel guilty doing so, > > because I don't know how maintainable this code abstraction is. > > Hrm...We could switch to a DoLoop. And yea, I think it's a valid point that the state machine here isn't easy to follow.
> And yea, I think it's a valid point that the state machine here isn't easy to > follow. I completely agree that the state machine in this class is not easy to follow and that lifetime management is hard to reason about. I've tried, but couldn't come up with a simpler approach :/ It is not clear to me if Matt suggests DoLoop to resolve lifetime management issues or complexity arising from interacting with URLRequest. If the latter, we would still be left with non-trivial lifetime management. If the former, then we'll end up with a single class that holds all the state for all requests, which is the design we moved away from. I hope I can somewhat alleviate Ryan's concerns about code health by pointing out that the complexity is limited to a single compilation unit - invoking callbacks created outside of this compilation unit is done in a safe manner and the "worst" a callback can do is submit a new request, which will create separate objects (log fetching requests are not re-used so no risk of re-entring code in an object that's deleted). My proposal is: - For now we stay with the existing design, despite the complexity, to unblock further work. - We watch crash reports closely when this code is launched to see if any crashes originate from it. - I will add UMA to measure how often this path in the code is used (it should not be frequent as in most cases clients should not need to fetch consistency proofs themselves). - If we find out that there's a significant number of crashes compared to the frequency this code is used, we'll switch to a DoLoop or another design that will simplify lifestyle management. How does that sound?
https://codereview.chromium.org/1405293009/diff/280001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1405293009/diff/280001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:159: // We have data, collect it and indicate another read is needed. On 2016/02/05 02:28:32, Ryan Sleevi wrote: > Pedantry: Avoid pronouns in comments (who is "we" here) > > // Data is available; collect it and indicate another read is needed. Done.
On 2016/02/05 11:04:14, Eran Messeri wrote: > My proposal is: > - For now we stay with the existing design, despite the complexity, to unblock > further work. This is the unfortunate path I was suggesting, given the lack of being able to suggest a good alternative. > - We watch crash reports closely when this code is launched to see if any > crashes originate from it. > - I will add UMA to measure how often this path in the code is used (it should > not be frequent as in most cases clients should not need to fetch consistency > proofs themselves). > - If we find out that there's a significant number of crashes compared to the > frequency this code is used, we'll switch to a DoLoop or another design that > will simplify lifestyle management. > > How does that sound? None of these address the concern I was raising, so it sounds like we're miscommunicating. I'm suggesting that the ability to understand and reason about the code, and the complexity there in, is itself an intrinsic feature. That is, even if there are zero crashes, if the code is too difficult to understand, then it's too difficult to maintain, and thus it slows down the ability to clean up, refactor, or understand for future feature additions. I think the DoLoop likely represents a reasonable path, but I also know that priorities and such mean it's unlikely to be done this quarter. And I agree that the value of making progress on CT likely outweighs the costs here, for now, although it's hard to evaluate what those full long-term costs will be. The state machine itself seems simple - Build request - Send request - Gather request body - Assemble as JSON - Parse JSON into native type With the exit nodes, at any point, being "failure callback" or "success callback" (That is, it seems there are never more than two terminal states). Is that a fair summary? If so, that's a rather trivial state machine, but I suspect the timezone and codebase differences will make it too long to progress. I realize I forgot to give the "LGTM", but it's with extreme reservations, for the reasons explained above. Even if the code never crashes, if we have to make *any* changes whatsoever, I'm not confident in the ability to thoroughly and safely review and extend :)
On 2016/02/07 22:15:45, Ryan Sleevi wrote: > On 2016/02/05 11:04:14, Eran Messeri wrote: > > My proposal is: > > - For now we stay with the existing design, despite the complexity, to unblock > > further work. > > This is the unfortunate path I was suggesting, given the lack of being able to > suggest a good alternative. > > > - We watch crash reports closely when this code is launched to see if any > > crashes originate from it. > > - I will add UMA to measure how often this path in the code is used (it should > > not be frequent as in most cases clients should not need to fetch consistency > > proofs themselves). > > - If we find out that there's a significant number of crashes compared to the > > frequency this code is used, we'll switch to a DoLoop or another design that > > will simplify lifestyle management. > > > > How does that sound? > > None of these address the concern I was raising, so it sounds like we're > miscommunicating. I'm suggesting that the ability to understand and reason about > the code, and the complexity there in, is itself an intrinsic feature. That is, > even if there are zero crashes, if the code is too difficult to understand, then > it's too difficult to maintain, and thus it slows down the ability to clean up, > refactor, or understand for future feature additions. > > I think the DoLoop likely represents a reasonable path, but I also know that > priorities and such mean it's unlikely to be done this quarter. And I agree that > the value of making progress on CT likely outweighs the costs here, for now, > although it's hard to evaluate what those full long-term costs will be. > > The state machine itself seems simple > > - Build request > - Send request > - Gather request body > - Assemble as JSON > - Parse JSON into native type > > With the exit nodes, at any point, being "failure callback" or "success > callback" (That is, it seems there are never more than two terminal states). Is > that a fair summary? If so, that's a rather trivial state machine, but I suspect > the timezone and codebase differences will make it too long to progress. > > I realize I forgot to give the "LGTM", but it's with extreme reservations, for > the reasons explained above. Even if the code never crashes, if we have to make > *any* changes whatsoever, I'm not confident in the ability to thoroughly and > safely review and extend :) Thanks, I fully understand the concerns. I will submit and follow-up offline on ways to mitigate the complexity (if I understand correctly the first thing to try would be to re-write this as a state machine with transitions using the DoLoop model). FWIW I will likely adopt the DoLoop approach in follow-up code, particularly the code that tracks state of CT logs.
The CQ bit was checked by eranm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svaldez@chromium.org, mmenke@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1405293009/#ps320001 (title: "Catching up with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405293009/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405293009/320001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. Transient error: No JSON object could be decoded
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by eranm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405293009/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405293009/320001
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency: Fetching consistency proofs. This adds the ability for Chrome clients to fetch consistency proofs between two tree heads. It will be used in case a Chrome client has an old STH stored for which it was not provided with a consistency proof to a new STH pushed in a component update. BUG=506227 ========== to ========== Certificate Transparency: Fetching consistency proofs. This adds the ability for Chrome clients to fetch consistency proofs between two tree heads. It will be used in case a Chrome client has an old STH stored for which it was not provided with a consistency proof to a new STH pushed in a component update. BUG=506227 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency: Fetching consistency proofs. This adds the ability for Chrome clients to fetch consistency proofs between two tree heads. It will be used in case a Chrome client has an old STH stored for which it was not provided with a consistency proof to a new STH pushed in a component update. BUG=506227 ========== to ========== Certificate Transparency: Fetching consistency proofs. This adds the ability for Chrome clients to fetch consistency proofs between two tree heads. It will be used in case a Chrome client has an old STH stored for which it was not provided with a consistency proof to a new STH pushed in a component update. BUG=506227 Committed: https://crrev.com/6851c0c562cb3f711f0fa03fff18a0750ac221e9 Cr-Commit-Position: refs/heads/master@{#374865} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/6851c0c562cb3f711f0fa03fff18a0750ac221e9 Cr-Commit-Position: refs/heads/master@{#374865} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
