Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2066553002:
Certificate Transparency DNS log client (Closed)
|
Created:
4 years, 6 months ago by Rob Percival Modified:
4 years, 5 months ago CC:
chromium-reviews, certificate-transparency-chrome_googlegroups.com, sdefresne+watchlist_chromium.org, Eran Messeri, droger+watchlist_chromium.org, blundell+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@mock_dns_responses Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency DNS log client
This can query CT logs over DNS, as defined by:
https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft-ct-over-dns.md
This is required for obtaining audit proofs, which will allow Chrome to verify
that SCTs it receives are trustworthy and that logs are behaving correctly.
BUG=612439
Committed: https://crrev.com/59b6ea2217dbc10400b6a9d433ad13c91bb6b7c2
Committed: https://crrev.com/2c7cd56d182ae1a14191170c47154641a24633fa
Cr-Original-Commit-Position: refs/heads/master@{#403798}
Cr-Commit-Position: refs/heads/master@{#404199}
Patch Set 1 #Patch Set 2 : Adds base32 to dependencies #Patch Set 3 : Do not exclude dns/record_* from builds if mdns is disabled #Patch Set 4 : Improves documentation #Patch Set 5 : Simplifies handling of DnsTransactions #Patch Set 6 : Use GMock to test callback results #Patch Set 7 : Report invalid arguments in more cases #Patch Set 8 : Use mock sockets instead of mock DNS for tests #Patch Set 9 : Removes changes to mock DNS framework #Patch Set 10 : Rebase #
Total comments: 23
Patch Set 11 : Addresses some of mmenke's comments #Patch Set 12 : Test with both async and sync socket reads #
Total comments: 2
Patch Set 13 : Replaces GMock callback with hand-rolled callback #Patch Set 14 : Addresses almost all remaining comments by mmenke #Patch Set 15 : Tests that socket read errors are handled #Patch Set 16 : More tests and a comment describing a performance improvement that could be made #Patch Set 17 : Removes unneeded #include #
Total comments: 1
Patch Set 18 : Rebase #Patch Set 19 : Removes clock from LogDnsClient #Patch Set 20 : Removes use of content::TestBrowserThreadBundle #Patch Set 21 : Rebase #
Total comments: 12
Patch Set 22 : Rebase #Patch Set 23 : Addresses Eran's comments and tests timeouts #Patch Set 24 : Use net::Error matchers from net/test/gtest_util.h #Patch Set 25 : Extra precondition checks (also downgrades some CHECKs to DCHECKs) #Patch Set 26 : Revert using MerkleAuditProof::tree_size #
Total comments: 2
Patch Set 27 : Move parsing functions into unnamed namespace #Patch Set 28 : Addresses Eran's final comment #Patch Set 29 : Rebase and lint fixes #
Total comments: 4
Messages
Total messages: 91 (33 generated)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066553002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066553002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066553002/40001
robpercival@chromium.org changed reviewers: + eranm@chromium.org
PTAL. More tests will be added, but first I'm evaluating whether reworking the tests to use mock sockets is worthwhile (see comments by mmenke on https://codereview.chromium.org/2030783003). I've left out the tests I wrote that query real DNS resolvers.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066553002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
robpercival@chromium.org changed reviewers: + mmenke@chromium.org
Hi Matt, mind reviewing my use of mock sockets? This is the result of your review comments on https://codereview.chromium.org/2030783003/. Thanks, Rob
On 2016/06/23 16:28:51, Rob Percival wrote: > Hi Matt, mind reviewing my use of mock sockets? This is the result of your > review comments on https://codereview.chromium.org/2030783003/. > > Thanks, > Rob Very cool! I'm happy to. I'll try to do a pass today or tomorrow, but if it takes too long, maybe have to punt until next week - I'm on network bug triage, which is a full time responsibility.
On 2016/06/23 16:33:02, mmenke wrote: > On 2016/06/23 16:28:51, Rob Percival wrote: > > Hi Matt, mind reviewing my use of mock sockets? This is the result of your > > review comments on https://codereview.chromium.org/2030783003/. > > > > Thanks, > > Rob > > Very cool! I'm happy to. I'll try to do a pass today or tomorrow, but if it > takes too long, maybe have to punt until next week - I'm on network bug triage, > which is a full time responsibility. Thanks, much appreciated.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066553002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Patchset #11 (id:200001) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066553002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A number of suggestions. I just focused on the tests and the use of SequencedSocketData (Which looks fine, most comments are a little tangential) https://codereview.chromium.org/2066553002/diff/180001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:169: class MockSocket { These aren't sockets. Call them MockSocketData or somesuch? https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:175: expected_write_(net::SYNCHRONOUS, You should test net::ASYNC as well. For reads, especially, it's the more common result. https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:182: 1), You can also simulate network errors for both reads and writes - I think you should test both cases. Not as likely as with TCP, but should be sure it works. https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:208: mock_sockets_.clear(); Not needed. For each test, the test fixture is created from scratch, so this does nothing. Given that, I'd recommend you remove this line, and move all this stuff to the constructor, in an initializer list (And make them no longer unique_ptrs). https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:287: base::RunLoop().RunUntilIdle(); I don't think it's a great idea, rather than waiting until the callback function is invoked. I suggest using RunLoop::Run / RunLoop::Quit (Can bind the RunLoop as an argument to MockLeafIndexCallback::Run). https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:318: MockLeafIndexCallback callback; The chrome wide preference is not to use gmock, to make hacking on Chrome easier for new developers. Looks like it's fairly straightforward to just switch to something else for all of these.
https://codereview.chromium.org/2066553002/diff/180001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:307: std::unique_ptr<net::BoundTestNetLog> net_log_; Also, you don't need this - just pass in a net::BoundNetLog(), and everything will work fine. BoundTestNetLog is just needed if you want to peer at logged network events.
https://codereview.chromium.org/2066553002/diff/180001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:307: std::unique_ptr<net::BoundTestNetLog> net_log_; On 2016/06/27 17:04:09, mmenke wrote: > Also, you don't need this - just pass in a net::BoundNetLog(), and everything > will work fine. > > BoundTestNetLog is just needed if you want to peer at logged network events. (Also, pass in nullptr when you need a NetLog instead of a BoundNetLog)
Patchset #11 (id:220001) has been deleted
https://codereview.chromium.org/2066553002/diff/180001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:169: class MockSocket { On 2016/06/27 17:01:08, mmenke wrote: > These aren't sockets. Call them MockSocketData or somesuch? Done. https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:175: expected_write_(net::SYNCHRONOUS, On 2016/06/27 17:01:08, mmenke wrote: > You should test net::ASYNC as well. For reads, especially, it's the more common > result. Isn't that beyond the concern of LogDnsClient? It's using a DnsClient to handle all the socket interaction, so the DnsClient tests (and tests of classes it uses) should have that covered, shouldn't they? DnsClient shouldn't change its external behaviour based on the whether the reads/writes are async or not. https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:208: mock_sockets_.clear(); On 2016/06/27 17:01:07, mmenke wrote: > Not needed. For each test, the test fixture is created from scratch, so this > does nothing. > > Given that, I'd recommend you remove this line, and move all this stuff to the > constructor, in an initializer list (And make them no longer unique_ptrs). Done. https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:287: base::RunLoop().RunUntilIdle(); On 2016/06/27 17:01:08, mmenke wrote: > I don't think it's a great idea, rather than waiting until the callback function > is invoked. I suggest using RunLoop::Run / RunLoop::Quit (Can bind the RunLoop > as an argument to MockLeafIndexCallback::Run). Why's that? With your approach, if a bug causes the callback to not be invoked, each test case will hang until it times out. This approach should be reliable (this is all run on a single thread). It also seems to be in line with what a lot of other tests do (at least the ones I've come across). https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:307: std::unique_ptr<net::BoundTestNetLog> net_log_; On 2016/06/27 17:04:09, mmenke wrote: > Also, you don't need this - just pass in a net::BoundNetLog(), and everything > will work fine. > > BoundTestNetLog is just needed if you want to peer at logged network events. Done. https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:318: MockLeafIndexCallback callback; On 2016/06/27 17:01:08, mmenke wrote: > The chrome wide preference is not to use gmock, to make hacking on Chrome easier > for new developers. Looks like it's fairly straightforward to just switch to > something else for all of these. As it happens, I used to have some hand-rolled mocks until patch set 6 (https://codereview.chromium.org/2066553002/diff2/80001:100001/components/cert...). GMock is definitely more concise in this case though and I don't think it's unreasonable to expect new developers to learn the basics. Are GMock matchers included in that preference, or is it just mocks themselves?
https://codereview.chromium.org/2066553002/diff/180001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:175: expected_write_(net::SYNCHRONOUS, On 2016/06/27 19:01:18, Rob Percival wrote: > On 2016/06/27 17:01:08, mmenke wrote: > > You should test net::ASYNC as well. For reads, especially, it's the more > common > > result. > > Isn't that beyond the concern of LogDnsClient? It's using a DnsClient to handle > all the socket interaction, so the DnsClient tests (and tests of classes it > uses) should have that covered, shouldn't they? DnsClient shouldn't change its > external behaviour based on the whether the reads/writes are async or not. If DnsClient always returns asynchronously, regardless of whether reads/writes return sync/async, you're completely correct, it doesn't matter. https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:287: base::RunLoop().RunUntilIdle(); On 2016/06/27 19:01:18, Rob Percival wrote: > On 2016/06/27 17:01:08, mmenke wrote: > > I don't think it's a great idea, rather than waiting until the callback > function > > is invoked. I suggest using RunLoop::Run / RunLoop::Quit (Can bind the > RunLoop > > as an argument to MockLeafIndexCallback::Run). > > Why's that? With your approach, if a bug causes the callback to not be invoked, > each test case will hang until it times out. This approach should be reliable > (this is all run on a single thread). It also seems to be in line with what a > lot of other tests do (at least the ones I've come across). Just because tests do it, doesn't mean they should. RunUntilIdle often adds implicit dependencies on behaviors that are not part of the public API. DnsClient makes no promises about doing all its work on the current thread, for instance. Changing the code to do stuff on another thread would cause issues. Unlikely here, but I've run into the issue when refactoring other net/ APIs that also had seemed unlikely to switch to doing stuff on another thread. Also, code is often unable to fully validate APIs when it uses RunUntilIdle - it can only check the state of objects after the callback is invoked, and then all pending tasks are executed. net/ has a ton of cache tests, for instance, that only pass because they spin the message loop after the callback the test is actually using is complete. Is the state of the world consistent during the time the callback is actually invoked? The tests can't actually validate that, because of the extra spinning. It looks like gmock helps you get around some of this, in these tests, but...see other comment on gmock. :) Also worth noting that RunUntilIdle can also never terminate, if the message loop never becomes idle (Less likely than Run hanging, but it doesn't eliminate the issue). Note that if a number of tests hang, a tryjob will be terminated early, so you don't have all bots hung for the next 2 days (This didn't used to be the case, and it caused a lot of issues at one point). https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:318: MockLeafIndexCallback callback; On 2016/06/27 19:01:18, Rob Percival wrote: > On 2016/06/27 17:01:08, mmenke wrote: > > The chrome wide preference is not to use gmock, to make hacking on Chrome > easier > > for new developers. Looks like it's fairly straightforward to just switch to > > something else for all of these. > > As it happens, I used to have some hand-rolled mocks until patch set 6 > (https://codereview.chromium.org/2066553002/diff2/80001:100001/components/cert...). > GMock is definitely more concise in this case though and I don't think it's > unreasonable to expect new developers to learn the basics. Are GMock matchers > included in that preference, or is it just mocks themselves? Clarity is generally better than being concise. Here's the discussion on the topic: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/-KH_IP0rIWQ Consensus may be a bit weaker than I indicated, but I've continued to see a bunch of reviewers call it out, and I also find it much less readable. The lack of support for unique_ptrs also makes things look leaky.
LGTM, as-is. Looks like DnsTransactions can complete synchronously on Start, or at least the API technically allows it, even, so should test SYNC/SYNC, and ASYNC/ASYNC. Anything else is up to you. https://codereview.chromium.org/2066553002/diff/260001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2066553002/diff/260001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:302: net::BoundNetLog net_log_; Can remove this entirely - just inline net::BoundNetLog where you use this. Note that BoundNetLogs are copyable, they aren't passed as pointer.
Whether you want to follow my suggestions on Gmock and RunUntilIdle are up to you. I gave you my opinion, and my understanding of the consensus, but not going to fight over them, as I'm not an owner (I would probably fight over both if this were net/ code).
Actually, net/ Not LGTM. What's up with the net/ changes? You're removing stuff from the build files, but not deleting the files?
On 2016/06/27 19:33:53, mmenke wrote: > Actually, net/ Not LGTM. What's up with the net/ changes? You're removing > stuff from the build files, but not deleting the files? That's something that definitely needs attention from a net OWNER such as yourself, glad you brought it up! Those lines selectively remove some classes from compilation based on whether enable_mdns is turned on, but I depend on them regardless so I'd like them compiled either way. If you have a look at those classes, it appears that there is no reason we can't compile them all of the time.
On 2016/06/27 20:02:13, Rob Percival wrote: > On 2016/06/27 19:33:53, mmenke wrote: > > Actually, net/ Not LGTM. What's up with the net/ changes? You're removing > > stuff from the build files, but not deleting the files? > > That's something that definitely needs attention from a net OWNER such as > yourself, glad you brought it up! Those lines selectively remove some classes > from compilation based on whether enable_mdns is turned on, but I depend on them > regardless so I'd like them compiled either way. If you have a look at those > classes, it appears that there is no reason we can't compile them all of the > time. Ah, right, that all looks reasonable. net/ LGTM.
On 2016/06/27 19:30:32, mmenke wrote: > LGTM, as-is. Looks like DnsTransactions can complete synchronously on Start, or > at least the API technically allows it, even, so should test SYNC/SYNC, and > ASYNC/ASYNC. Anything else is up to you. > > https://codereview.chromium.org/2066553002/diff/260001/components/certificate... > File components/certificate_transparency/log_dns_client_unittest.cc (right): > > https://codereview.chromium.org/2066553002/diff/260001/components/certificate... > components/certificate_transparency/log_dns_client_unittest.cc:302: > net::BoundNetLog net_log_; > Can remove this entirely - just inline net::BoundNetLog where you use this. > Note that BoundNetLogs are copyable, they aren't passed as pointer. Based on see https://cs.chromium.org/chromium/src/net/dns/dns_transaction.h?sq=package:chr... and https://cs.chromium.org/chromium/src/net/dns/dns_transaction.cc?sq=package:ch..., it appears to state that the DnsTransactions will always complete asynchronously. Start() doesn't have any way of synchronously returning a result from what I can see. Nevertheless, I've added a patch set that tests with both sync and async socket reads. Are you saying it should test just the two combinations of synchronous reads and writes and asynchronous reads and writes, but not a combination of the two?
https://codereview.chromium.org/2066553002/diff/180001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:287: base::RunLoop().RunUntilIdle(); On 2016/06/27 19:26:20, mmenke wrote: > On 2016/06/27 19:01:18, Rob Percival wrote: > > On 2016/06/27 17:01:08, mmenke wrote: > > > I don't think it's a great idea, rather than waiting until the callback > > function > > > is invoked. I suggest using RunLoop::Run / RunLoop::Quit (Can bind the > > RunLoop > > > as an argument to MockLeafIndexCallback::Run). > > > > Why's that? With your approach, if a bug causes the callback to not be > invoked, > > each test case will hang until it times out. This approach should be reliable > > (this is all run on a single thread). It also seems to be in line with what a > > lot of other tests do (at least the ones I've come across). > > Just because tests do it, doesn't mean they should. > > RunUntilIdle often adds implicit dependencies on behaviors that are not part of > the public API. DnsClient makes no promises about doing all its work on the > current thread, for instance. Changing the code to do stuff on another thread > would cause issues. Unlikely here, but I've run into the issue when refactoring > other net/ APIs that also had seemed unlikely to switch to doing stuff on > another thread. > > Also, code is often unable to fully validate APIs when it uses RunUntilIdle - it > can only check the state of objects after the callback is invoked, and then all > pending tasks are executed. net/ has a ton of cache tests, for instance, that > only pass because they spin the message loop after the callback the test is > actually using is complete. Is the state of the world consistent during the > time the callback is actually invoked? The tests can't actually validate that, > because of the extra spinning. It looks like gmock helps you get around some of > this, in these tests, but...see other comment on gmock. :) > > Also worth noting that RunUntilIdle can also never terminate, if the message > loop never becomes idle (Less likely than Run hanging, but it doesn't eliminate > the issue). Note that if a number of tests hang, a tryjob will be terminated > early, so you don't have all bots hung for the next 2 days (This didn't used to > be the case, and it caused a lot of issues at one point). It's also the way TestBrowserThreadBundle gives as an example (https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_...). Is it possible for DnsClient to do anything on another thread? There are no other threads in existence. TestBrowserThreadBundle runs all tasks, regardless of what browser thread they were posted to, on the thread that calls RunLoop().RunUntilIdle(). I can't imagine DnsClient ever creating its own threads either, but maybe I just lack imagination ;) It seems like this is good advice for cases where there are multiple threads and/or sub-optimal callback testing. However, in this single-threaded case with GMock, it should be redundant and just unnecessarily complicate the tests. I suppose there's an argument for doing it here just so that others won't copy this technique and use it in those situations where it isn't as suitable though.. https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:318: MockLeafIndexCallback callback; On 2016/06/27 19:26:20, mmenke wrote: > On 2016/06/27 19:01:18, Rob Percival wrote: > > On 2016/06/27 17:01:08, mmenke wrote: > > > The chrome wide preference is not to use gmock, to make hacking on Chrome > > easier > > > for new developers. Looks like it's fairly straightforward to just switch > to > > > something else for all of these. > > > > As it happens, I used to have some hand-rolled mocks until patch set 6 > > > (https://codereview.chromium.org/2066553002/diff2/80001:100001/components/cert...). > > GMock is definitely more concise in this case though and I don't think it's > > unreasonable to expect new developers to learn the basics. Are GMock matchers > > included in that preference, or is it just mocks themselves? > > Clarity is generally better than being concise. Here's the discussion on the > topic: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/-KH_IP0rIWQ > > Consensus may be a bit weaker than I indicated, but I've continued to see a > bunch of reviewers call it out, and I also find it much less readable. The lack > of support for unique_ptrs also makes things look leaky. That lack of support for unique_ptrs is indeed unfortunate. GMock does support them but some of the build bots are using versions of libc++ with incomplete C++11 support (missing some type traits), so GMock with C++11 support enabled fails to compile :( Very well, if you dislike GMock that much then I'll revert to using the hand-rolled mocks I had before.
On 2016/06/27 20:10:51, Rob Percival wrote: > On 2016/06/27 19:30:32, mmenke wrote: > > LGTM, as-is. Looks like DnsTransactions can complete synchronously on Start, > or > > at least the API technically allows it, even, so should test SYNC/SYNC, and > > ASYNC/ASYNC. Anything else is up to you. > > > > > https://codereview.chromium.org/2066553002/diff/260001/components/certificate... > > File components/certificate_transparency/log_dns_client_unittest.cc (right): > > > > > https://codereview.chromium.org/2066553002/diff/260001/components/certificate... > > components/certificate_transparency/log_dns_client_unittest.cc:302: > > net::BoundNetLog net_log_; > > Can remove this entirely - just inline net::BoundNetLog where you use this. > > Note that BoundNetLogs are copyable, they aren't passed as pointer. > > Based on see > https://cs.chromium.org/chromium/src/net/dns/dns_transaction.h?sq=package:chr... > and > https://cs.chromium.org/chromium/src/net/dns/dns_transaction.cc?sq=package:ch..., > it appears to state that the DnsTransactions will always complete > asynchronously. Start() doesn't have any way of synchronously returning a result > from what I can see. Nevertheless, I've added a patch set that tests with both > sync and async socket reads. Are you saying it should test just the two > combinations of synchronous reads and writes and asynchronous reads and writes, > but not a combination of the two? Oops, you're right. I was looking at DnsAttempt::Start, and say the sync return path. Since DnsTransaction::Start can't complete synchronously, you're fine making everything sync or async. Sorry about that!
https://codereview.chromium.org/2066553002/diff/180001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2066553002/diff/180001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:287: base::RunLoop().RunUntilIdle(); On 2016/06/27 20:12:11, Rob Percival wrote: > On 2016/06/27 19:26:20, mmenke wrote: > > On 2016/06/27 19:01:18, Rob Percival wrote: > > > On 2016/06/27 17:01:08, mmenke wrote: > > > > I don't think it's a great idea, rather than waiting until the callback > > > function > > > > is invoked. I suggest using RunLoop::Run / RunLoop::Quit (Can bind the > > > RunLoop > > > > as an argument to MockLeafIndexCallback::Run). > > > > > > Why's that? With your approach, if a bug causes the callback to not be > > invoked, > > > each test case will hang until it times out. This approach should be > reliable > > > (this is all run on a single thread). It also seems to be in line with what > a > > > lot of other tests do (at least the ones I've come across). > > > > Just because tests do it, doesn't mean they should. > > > > RunUntilIdle often adds implicit dependencies on behaviors that are not part > of > > the public API. DnsClient makes no promises about doing all its work on the > > current thread, for instance. Changing the code to do stuff on another thread > > would cause issues. Unlikely here, but I've run into the issue when > refactoring > > other net/ APIs that also had seemed unlikely to switch to doing stuff on > > another thread. > > > > Also, code is often unable to fully validate APIs when it uses RunUntilIdle - > it > > can only check the state of objects after the callback is invoked, and then > all > > pending tasks are executed. net/ has a ton of cache tests, for instance, that > > only pass because they spin the message loop after the callback the test is > > actually using is complete. Is the state of the world consistent during the > > time the callback is actually invoked? The tests can't actually validate > that, > > because of the extra spinning. It looks like gmock helps you get around some > of > > this, in these tests, but...see other comment on gmock. :) > > > > Also worth noting that RunUntilIdle can also never terminate, if the message > > loop never becomes idle (Less likely than Run hanging, but it doesn't > eliminate > > the issue). Note that if a number of tests hang, a tryjob will be terminated > > early, so you don't have all bots hung for the next 2 days (This didn't used > to > > be the case, and it caused a lot of issues at one point). > > It's also the way TestBrowserThreadBundle gives as an example > (https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_...). > Is it possible for DnsClient to do anything on another thread? There are no > other threads in existence. TestBrowserThreadBundle runs all tasks, regardless > of what browser thread they were posted to, on the thread that calls > RunLoop().RunUntilIdle(). I can't imagine DnsClient ever creating its own > threads either, but maybe I just lack imagination ;) I think it's unlikely, but remember that the WorkerPool is a global thread pool. SimpleUrlRequestJob used to be a single-threaded class, but then it turned out consumers were using its "memcpy" to copy data from memory mapped files, which caused it to do file I/O on the IO thread, so we switched to using WorkerPools...breaking unittests that really didn't need to care if it was done synchronously or not. So modifying the API to behave in a way such that it still adheres to spec would break this code. Meaning the hapless network stack team member doing that would have to dig into this test and fix it. > It seems like this is good advice for cases where there are multiple threads > and/or sub-optimal callback testing. However, in this single-threaded case with > GMock, it should be redundant and just unnecessarily complicate the tests. I > suppose there's an argument for doing it here just so that others won't copy > this technique and use it in those situations where it isn't as suitable > though.. If you want to use this pattern, feel free - I've said why I think it's a bad pattern, but not going to push back any more on it.
On 2016/06/27 20:24:45, mmenke wrote: > https://codereview.chromium.org/2066553002/diff/180001/components/certificate... > File components/certificate_transparency/log_dns_client_unittest.cc (right): > > https://codereview.chromium.org/2066553002/diff/180001/components/certificate... > components/certificate_transparency/log_dns_client_unittest.cc:287: > base::RunLoop().RunUntilIdle(); > On 2016/06/27 20:12:11, Rob Percival wrote: > > On 2016/06/27 19:26:20, mmenke wrote: > > > On 2016/06/27 19:01:18, Rob Percival wrote: > > > > On 2016/06/27 17:01:08, mmenke wrote: > > > > > I don't think it's a great idea, rather than waiting until the callback > > > > function > > > > > is invoked. I suggest using RunLoop::Run / RunLoop::Quit (Can bind the > > > > RunLoop > > > > > as an argument to MockLeafIndexCallback::Run). > > > > > > > > Why's that? With your approach, if a bug causes the callback to not be > > > invoked, > > > > each test case will hang until it times out. This approach should be > > reliable > > > > (this is all run on a single thread). It also seems to be in line with > what > > a > > > > lot of other tests do (at least the ones I've come across). > > > > > > Just because tests do it, doesn't mean they should. > > > > > > RunUntilIdle often adds implicit dependencies on behaviors that are not part > > of > > > the public API. DnsClient makes no promises about doing all its work on the > > > current thread, for instance. Changing the code to do stuff on another > thread > > > would cause issues. Unlikely here, but I've run into the issue when > > refactoring > > > other net/ APIs that also had seemed unlikely to switch to doing stuff on > > > another thread. > > > > > > Also, code is often unable to fully validate APIs when it uses RunUntilIdle > - > > it > > > can only check the state of objects after the callback is invoked, and then > > all > > > pending tasks are executed. net/ has a ton of cache tests, for instance, > that > > > only pass because they spin the message loop after the callback the test is > > > actually using is complete. Is the state of the world consistent during the > > > time the callback is actually invoked? The tests can't actually validate > > that, > > > because of the extra spinning. It looks like gmock helps you get around > some > > of > > > this, in these tests, but...see other comment on gmock. :) > > > > > > Also worth noting that RunUntilIdle can also never terminate, if the message > > > loop never becomes idle (Less likely than Run hanging, but it doesn't > > eliminate > > > the issue). Note that if a number of tests hang, a tryjob will be > terminated > > > early, so you don't have all bots hung for the next 2 days (This didn't used > > to > > > be the case, and it caused a lot of issues at one point). > > > > It's also the way TestBrowserThreadBundle gives as an example > > > (https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_...). > > Is it possible for DnsClient to do anything on another thread? There are no > > other threads in existence. TestBrowserThreadBundle runs all tasks, regardless > > of what browser thread they were posted to, on the thread that calls > > RunLoop().RunUntilIdle(). I can't imagine DnsClient ever creating its own > > threads either, but maybe I just lack imagination ;) > > I think it's unlikely, but remember that the WorkerPool is a global thread pool. > SimpleUrlRequestJob used to be a single-threaded class, but then it turned out > consumers were using its "memcpy" to copy data from memory mapped files, which > caused it to do file I/O on the IO thread, so we switched to using > WorkerPools...breaking unittests that really didn't need to care if it was done > synchronously or not. > > So modifying the API to behave in a way such that it still adheres to spec would > break this code. Meaning the hapless network stack team member doing that would > have to dig into this test and fix it. > > > It seems like this is good advice for cases where there are multiple threads > > and/or sub-optimal callback testing. However, in this single-threaded case > with > > GMock, it should be redundant and just unnecessarily complicate the tests. I > > suppose there's an argument for doing it here just so that others won't copy > > this technique and use it in those situations where it isn't as suitable > > though.. > > If you want to use this pattern, feel free - I've said why I think it's a bad > pattern, but not going to push back any more on it. Ah I hadn't seen WorkerPool. Since I'm re-writing the callbacks to not use GMock, I might as well change the way I use RunLoop in the process and save that hypothetical hapless engineer a potential headache, though I expect they've got bigger problems if WorkerPool worms its way into the DNS stack! I'll add some tests for read/write errors too, as suggested. Thanks for the feedback!
On 2016/06/27 20:50:05, Rob Percival wrote: > On 2016/06/27 20:24:45, mmenke wrote: > > > https://codereview.chromium.org/2066553002/diff/180001/components/certificate... > > File components/certificate_transparency/log_dns_client_unittest.cc (right): > > > > > https://codereview.chromium.org/2066553002/diff/180001/components/certificate... > > components/certificate_transparency/log_dns_client_unittest.cc:287: > > base::RunLoop().RunUntilIdle(); > > On 2016/06/27 20:12:11, Rob Percival wrote: > > > On 2016/06/27 19:26:20, mmenke wrote: > > > > On 2016/06/27 19:01:18, Rob Percival wrote: > > > > > On 2016/06/27 17:01:08, mmenke wrote: > > > > > > I don't think it's a great idea, rather than waiting until the > callback > > > > > function > > > > > > is invoked. I suggest using RunLoop::Run / RunLoop::Quit (Can bind > the > > > > > RunLoop > > > > > > as an argument to MockLeafIndexCallback::Run). > > > > > > > > > > Why's that? With your approach, if a bug causes the callback to not be > > > > invoked, > > > > > each test case will hang until it times out. This approach should be > > > reliable > > > > > (this is all run on a single thread). It also seems to be in line with > > what > > > a > > > > > lot of other tests do (at least the ones I've come across). > > > > > > > > Just because tests do it, doesn't mean they should. > > > > > > > > RunUntilIdle often adds implicit dependencies on behaviors that are not > part > > > of > > > > the public API. DnsClient makes no promises about doing all its work on > the > > > > current thread, for instance. Changing the code to do stuff on another > > thread > > > > would cause issues. Unlikely here, but I've run into the issue when > > > refactoring > > > > other net/ APIs that also had seemed unlikely to switch to doing stuff on > > > > another thread. > > > > > > > > Also, code is often unable to fully validate APIs when it uses > RunUntilIdle > > - > > > it > > > > can only check the state of objects after the callback is invoked, and > then > > > all > > > > pending tasks are executed. net/ has a ton of cache tests, for instance, > > that > > > > only pass because they spin the message loop after the callback the test > is > > > > actually using is complete. Is the state of the world consistent during > the > > > > time the callback is actually invoked? The tests can't actually validate > > > that, > > > > because of the extra spinning. It looks like gmock helps you get around > > some > > > of > > > > this, in these tests, but...see other comment on gmock. :) > > > > > > > > Also worth noting that RunUntilIdle can also never terminate, if the > message > > > > loop never becomes idle (Less likely than Run hanging, but it doesn't > > > eliminate > > > > the issue). Note that if a number of tests hang, a tryjob will be > > terminated > > > > early, so you don't have all bots hung for the next 2 days (This didn't > used > > > to > > > > be the case, and it caused a lot of issues at one point). > > > > > > It's also the way TestBrowserThreadBundle gives as an example > > > > > > (https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_...). > > > Is it possible for DnsClient to do anything on another thread? There are no > > > other threads in existence. TestBrowserThreadBundle runs all tasks, > regardless > > > of what browser thread they were posted to, on the thread that calls > > > RunLoop().RunUntilIdle(). I can't imagine DnsClient ever creating its own > > > threads either, but maybe I just lack imagination ;) > > > > I think it's unlikely, but remember that the WorkerPool is a global thread > pool. > > SimpleUrlRequestJob used to be a single-threaded class, but then it turned > out > > consumers were using its "memcpy" to copy data from memory mapped files, which > > caused it to do file I/O on the IO thread, so we switched to using > > WorkerPools...breaking unittests that really didn't need to care if it was > done > > synchronously or not. > > > > So modifying the API to behave in a way such that it still adheres to spec > would > > break this code. Meaning the hapless network stack team member doing that > would > > have to dig into this test and fix it. > > > > > It seems like this is good advice for cases where there are multiple threads > > > and/or sub-optimal callback testing. However, in this single-threaded case > > with > > > GMock, it should be redundant and just unnecessarily complicate the tests. I > > > suppose there's an argument for doing it here just so that others won't copy > > > this technique and use it in those situations where it isn't as suitable > > > though.. > > > > If you want to use this pattern, feel free - I've said why I think it's a bad > > pattern, but not going to push back any more on it. > > Ah I hadn't seen WorkerPool. Since I'm re-writing the callbacks to not use > GMock, I might as well change the way I use RunLoop in the process and save that > hypothetical hapless engineer a potential headache, though I expect they've got > bigger problems if WorkerPool worms its way into the DNS stack! I'll add some > tests for read/write errors too, as suggested. Thanks for the feedback! Think you actually just need one network error case, since read and write errors should use the same path through your code.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
All comments have been addressed. https://chromiumcodereview-hr.appspot.com/2066553002/diff/180001/components/c... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://chromiumcodereview-hr.appspot.com/2066553002/diff/180001/components/c... components/certificate_transparency/log_dns_client_unittest.cc:182: 1), On 2016/06/27 17:01:07, mmenke wrote: > You can also simulate network errors for both reads and writes - I think you > should test both cases. Not as likely as with TCP, but should be sure it works. Done. https://chromiumcodereview-hr.appspot.com/2066553002/diff/180001/components/c... components/certificate_transparency/log_dns_client_unittest.cc:287: base::RunLoop().RunUntilIdle(); On 2016/06/27 20:24:45, mmenke wrote: > On 2016/06/27 20:12:11, Rob Percival wrote: > > On 2016/06/27 19:26:20, mmenke wrote: > > > On 2016/06/27 19:01:18, Rob Percival wrote: > > > > On 2016/06/27 17:01:08, mmenke wrote: > > > > > I don't think it's a great idea, rather than waiting until the callback > > > > function > > > > > is invoked. I suggest using RunLoop::Run / RunLoop::Quit (Can bind the > > > > RunLoop > > > > > as an argument to MockLeafIndexCallback::Run). > > > > > > > > Why's that? With your approach, if a bug causes the callback to not be > > > invoked, > > > > each test case will hang until it times out. This approach should be > > reliable > > > > (this is all run on a single thread). It also seems to be in line with > what > > a > > > > lot of other tests do (at least the ones I've come across). > > > > > > Just because tests do it, doesn't mean they should. > > > > > > RunUntilIdle often adds implicit dependencies on behaviors that are not part > > of > > > the public API. DnsClient makes no promises about doing all its work on the > > > current thread, for instance. Changing the code to do stuff on another > thread > > > would cause issues. Unlikely here, but I've run into the issue when > > refactoring > > > other net/ APIs that also had seemed unlikely to switch to doing stuff on > > > another thread. > > > > > > Also, code is often unable to fully validate APIs when it uses RunUntilIdle > - > > it > > > can only check the state of objects after the callback is invoked, and then > > all > > > pending tasks are executed. net/ has a ton of cache tests, for instance, > that > > > only pass because they spin the message loop after the callback the test is > > > actually using is complete. Is the state of the world consistent during the > > > time the callback is actually invoked? The tests can't actually validate > > that, > > > because of the extra spinning. It looks like gmock helps you get around > some > > of > > > this, in these tests, but...see other comment on gmock. :) > > > > > > Also worth noting that RunUntilIdle can also never terminate, if the message > > > loop never becomes idle (Less likely than Run hanging, but it doesn't > > eliminate > > > the issue). Note that if a number of tests hang, a tryjob will be > terminated > > > early, so you don't have all bots hung for the next 2 days (This didn't used > > to > > > be the case, and it caused a lot of issues at one point). > > > > It's also the way TestBrowserThreadBundle gives as an example > > > (https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_...). > > Is it possible for DnsClient to do anything on another thread? There are no > > other threads in existence. TestBrowserThreadBundle runs all tasks, regardless > > of what browser thread they were posted to, on the thread that calls > > RunLoop().RunUntilIdle(). I can't imagine DnsClient ever creating its own > > threads either, but maybe I just lack imagination ;) > > I think it's unlikely, but remember that the WorkerPool is a global thread pool. > SimpleUrlRequestJob used to be a single-threaded class, but then it turned out > consumers were using its "memcpy" to copy data from memory mapped files, which > caused it to do file I/O on the IO thread, so we switched to using > WorkerPools...breaking unittests that really didn't need to care if it was done > synchronously or not. > > So modifying the API to behave in a way such that it still adheres to spec would > break this code. Meaning the hapless network stack team member doing that would > have to dig into this test and fix it. > > > It seems like this is good advice for cases where there are multiple threads > > and/or sub-optimal callback testing. However, in this single-threaded case > with > > GMock, it should be redundant and just unnecessarily complicate the tests. I > > suppose there's an argument for doing it here just so that others won't copy > > this technique and use it in those situations where it isn't as suitable > > though.. > > If you want to use this pattern, feel free - I've said why I think it's a bad > pattern, but not going to push back any more on it. Done. https://chromiumcodereview-hr.appspot.com/2066553002/diff/180001/components/c... components/certificate_transparency/log_dns_client_unittest.cc:318: MockLeafIndexCallback callback; On 2016/06/27 20:12:11, Rob Percival wrote: > On 2016/06/27 19:26:20, mmenke wrote: > > On 2016/06/27 19:01:18, Rob Percival wrote: > > > On 2016/06/27 17:01:08, mmenke wrote: > > > > The chrome wide preference is not to use gmock, to make hacking on Chrome > > > easier > > > > for new developers. Looks like it's fairly straightforward to just switch > > to > > > > something else for all of these. > > > > > > As it happens, I used to have some hand-rolled mocks until patch set 6 > > > > > > (https://codereview.chromium.org/2066553002/diff2/80001:100001/components/cert...). > > > GMock is definitely more concise in this case though and I don't think it's > > > unreasonable to expect new developers to learn the basics. Are GMock > matchers > > > included in that preference, or is it just mocks themselves? > > > > Clarity is generally better than being concise. Here's the discussion on the > > topic: > > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/-KH_IP0rIWQ > > > > Consensus may be a bit weaker than I indicated, but I've continued to see a > > bunch of reviewers call it out, and I also find it much less readable. The > lack > > of support for unique_ptrs also makes things look leaky. > > That lack of support for unique_ptrs is indeed unfortunate. GMock does support > them but some of the build bots are using versions of libc++ with incomplete > C++11 support (missing some type traits), so GMock with C++11 support enabled > fails to compile :( > > Very well, if you dislike GMock that much then I'll revert to using the > hand-rolled mocks I had before. Done. https://chromiumcodereview-hr.appspot.com/2066553002/diff/260001/components/c... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://chromiumcodereview-hr.appspot.com/2066553002/diff/260001/components/c... components/certificate_transparency/log_dns_client_unittest.cc:302: net::BoundNetLog net_log_; On 2016/06/27 19:30:32, mmenke wrote: > Can remove this entirely - just inline net::BoundNetLog where you use this. > Note that BoundNetLogs are copyable, they aren't passed as pointer. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jam@chromium.org changed reviewers: + jam@chromium.org
https://chromiumcodereview-hr.appspot.com/2066553002/diff/360001/components/c... File components/certificate_transparency/DEPS (right): https://chromiumcodereview-hr.appspot.com/2066553002/diff/360001/components/c... components/certificate_transparency/DEPS:4: "+content/public/test/test_browser_thread_bundle.h", content/ is pretty large, so we avoid depending on it just to get small helpers like this. the unittest can just create an IO thread manually like other tests in net/
On 2016/06/30 06:32:47, jam wrote: > https://chromiumcodereview-hr.appspot.com/2066553002/diff/360001/components/c... > File components/certificate_transparency/DEPS (right): > > https://chromiumcodereview-hr.appspot.com/2066553002/diff/360001/components/c... > components/certificate_transparency/DEPS:4: > "+content/public/test/test_browser_thread_bundle.h", > content/ is pretty large, so we avoid depending on it just to get small helpers > like this. > > the unittest can just create an IO thread manually like other tests in net/ Done.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was unchecked by commit-bot@chromium.org
lgtm
overall looks good, the only major comment is handling of an empty response from the log (i.e. no nodes). https://codereview.chromium.org/2066553002/diff/440001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:49: std::string qname = encoded_leaf_hash; Nit: Use StringPrintf https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:97: QueryAuditProofNodes(std::move(proof), domain_for_log, leaf_index, tree_size, You could drop the leaf_index from the parameters list as it's in |proof|. Also, add a TODO to add the tree size to the MerkleAuditProof, because a proof that does not indicate which tree size it's for is useless. https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:98: 0, callback); comment what the 0 stands for, e.g.: 0 /* first proof node */ https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:122: if (response == nullptr) { Nit: You could move this condition up (to line 115) and change the net_error to be ERR_INVALID_RESPONSE if response is null, then the check in lines 116-120 will catch that and invoke the callback with error. https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:270: if (!ParseTxtResponse(response, &audit_path)) Also return false if the audit_path is of length 0, meaning the server has not returned any nodes (which could lead to the code performing the same query again and again). https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:276: if (node.size() == crypto::kSHA256Length) You could check beforehand: if (audit_path.size() % crypto::kSHA256Length != 0) return false And avoid checking each and every node.
PTAL. The latest patch set is dependent on https://chromiumcodereview-hr.appspot.com/2107423004/. https://codereview.chromium.org/2066553002/diff/440001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:49: std::string qname = encoded_leaf_hash; On 2016/07/01 09:08:00, Eran Messeri wrote: > Nit: Use StringPrintf I've opted for using std::ostringstream instead, as that gives me the same behaviour. StringPrintf with StringPiece works differently (StringPrintf prints "(null)" for null StringPieces, rather than an empty string). https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:97: QueryAuditProofNodes(std::move(proof), domain_for_log, leaf_index, tree_size, On 2016/07/01 09:07:59, Eran Messeri wrote: > You could drop the leaf_index from the parameters list as it's in |proof|. > Also, add a TODO to add the tree size to the MerkleAuditProof, because a proof > that does not indicate which tree size it's for is useless. Done. https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:98: 0, callback); On 2016/07/01 09:08:00, Eran Messeri wrote: > comment what the 0 stands for, e.g.: > 0 /* first proof node */ Done. https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:122: if (response == nullptr) { On 2016/07/01 09:08:00, Eran Messeri wrote: > Nit: You could move this condition up (to line 115) and change the net_error to > be ERR_INVALID_RESPONSE if response is null, then the check in lines 116-120 > will catch that and invoke the callback with error. That could hide a more specific net_error though. I can do "if (response == nullptr && net_error == net::OK) net_error = net::ERR_INVALID_RESPONSE;" back at line 115 I suppose. I guess it simplifies the flow a little. https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:270: if (!ParseTxtResponse(response, &audit_path)) On 2016/07/01 09:07:59, Eran Messeri wrote: > Also return false if the audit_path is of length 0, meaning the server has not > returned any nodes (which could lead to the code performing the same query again > and again). Done. https://codereview.chromium.org/2066553002/diff/440001/components/certificate... components/certificate_transparency/log_dns_client.cc:276: if (node.size() == crypto::kSHA256Length) On 2016/07/01 09:08:00, Eran Messeri wrote: > You could check beforehand: > if (audit_path.size() % crypto::kSHA256Length != 0) > return false > > And avoid checking each and every node. Done.
lgtm with one minor comment. On a side note, the tests having to create a DNS response at the byte level is very cumbersome. We should work something out so that other tests which need a fake LogDnsClient do not have to perform such a complex set-up. https://codereview.chromium.org/2066553002/diff/530001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2066553002/diff/530001/components/certificate... components/certificate_transparency/log_dns_client.cc:52: std::ostringstream qname; Nit (here and below): You could define and fill qname later on, after getting the DnsTransactionFactory and binding it.
On 2016/07/05 15:28:12, Eran Messeri wrote: > lgtm with one minor comment. > > On a side note, the tests having to create a DNS response at the byte level is > very cumbersome. > We should work something out so that other tests which need a fake LogDnsClient > do not have to perform such a complex set-up. > > https://codereview.chromium.org/2066553002/diff/530001/components/certificate... > File components/certificate_transparency/log_dns_client.cc (right): > > https://codereview.chromium.org/2066553002/diff/530001/components/certificate... > components/certificate_transparency/log_dns_client.cc:52: std::ostringstream > qname; > Nit (here and below): You could define and fill qname later on, after getting > the DnsTransactionFactory and binding it. I agree about the testing being cumbersome. If you look at patch set 8 (https://codereview.chromium.org/2066553002/#ps140001), you can see the more concise tests that are possible using MockDnsClient and https://chromiumcodereview-hr.appspot.com/2030783003. I think that would be the best way to test things that use LogDnsClient.
https://codereview.chromium.org/2066553002/diff/530001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2066553002/diff/530001/components/certificate... components/certificate_transparency/log_dns_client.cc:52: std::ostringstream qname; On 2016/07/05 15:28:12, Eran Messeri wrote: > Nit (here and below): You could define and fill qname later on, after getting > the DnsTransactionFactory and binding it. Done.
The CQ bit was checked by robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, jam@chromium.org, eranm@chromium.org Link to the patchset: https://codereview.chromium.org/2066553002/#ps570001 (title: "Addresses Eran's final comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #28 (id:570001)
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency DNS log client This can query CT logs over DNS, as defined by: https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft... This is required for obtaining audit proofs, which will allow Chrome to verify that SCTs it receives are trustworthy and that logs are behaving correctly. BUG=612439 ========== to ========== Certificate Transparency DNS log client This can query CT logs over DNS, as defined by: https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft... This is required for obtaining audit proofs, which will allow Chrome to verify that SCTs it receives are trustworthy and that logs are behaving correctly. BUG=612439 Committed: https://crrev.com/59b6ea2217dbc10400b6a9d433ad13c91bb6b7c2 Cr-Commit-Position: refs/heads/master@{#403798} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/59b6ea2217dbc10400b6a9d433ad13c91bb6b7c2 Cr-Commit-Position: refs/heads/master@{#403798}
Message was sent while issue was closed.
A revert of this CL (patchset #28 id:570001) has been created in https://codereview.chromium.org/2124873002/ by jbroman@chromium.org. The reason for reverting is: Causing Win build failure: https://build.chromium.org/p/chromium/builders/Win%20x64/builds/2184.
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency DNS log client This can query CT logs over DNS, as defined by: https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft... This is required for obtaining audit proofs, which will allow Chrome to verify that SCTs it receives are trustworthy and that logs are behaving correctly. BUG=612439 Committed: https://crrev.com/59b6ea2217dbc10400b6a9d433ad13c91bb6b7c2 Cr-Commit-Position: refs/heads/master@{#403798} ========== to ========== Certificate Transparency DNS log client This can query CT logs over DNS, as defined by: https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft... This is required for obtaining audit proofs, which will allow Chrome to verify that SCTs it receives are trustworthy and that logs are behaving correctly. BUG=612439 Committed: https://crrev.com/59b6ea2217dbc10400b6a9d433ad13c91bb6b7c2 Cr-Commit-Position: refs/heads/master@{#403798} ==========
On 2016/07/05 17:40:11, jbroman wrote: > A revert of this CL (patchset #28 id:570001) has been created in > https://codereview.chromium.org/2124873002/ by mailto:jbroman@chromium.org. > > The reason for reverting is: Causing Win build failure: > > https://build.chromium.org/p/chromium/builders/Win%20x64/builds/2184. Need to land https://codereview.chromium.org/2118383006 in order to fix the build failure.
The CQ bit was checked by robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, eranm@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2066553002/#ps590001 (title: "Rebase and lint fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency DNS log client This can query CT logs over DNS, as defined by: https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft... This is required for obtaining audit proofs, which will allow Chrome to verify that SCTs it receives are trustworthy and that logs are behaving correctly. BUG=612439 Committed: https://crrev.com/59b6ea2217dbc10400b6a9d433ad13c91bb6b7c2 Cr-Commit-Position: refs/heads/master@{#403798} ========== to ========== Certificate Transparency DNS log client This can query CT logs over DNS, as defined by: https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft... This is required for obtaining audit proofs, which will allow Chrome to verify that SCTs it receives are trustworthy and that logs are behaving correctly. BUG=612439 Committed: https://crrev.com/59b6ea2217dbc10400b6a9d433ad13c91bb6b7c2 Cr-Commit-Position: refs/heads/master@{#403798} ==========
Message was sent while issue was closed.
Committed patchset #29 (id:590001)
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency DNS log client This can query CT logs over DNS, as defined by: https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft... This is required for obtaining audit proofs, which will allow Chrome to verify that SCTs it receives are trustworthy and that logs are behaving correctly. BUG=612439 Committed: https://crrev.com/59b6ea2217dbc10400b6a9d433ad13c91bb6b7c2 Cr-Commit-Position: refs/heads/master@{#403798} ========== to ========== Certificate Transparency DNS log client This can query CT logs over DNS, as defined by: https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft... This is required for obtaining audit proofs, which will allow Chrome to verify that SCTs it receives are trustworthy and that logs are behaving correctly. BUG=612439 Committed: https://crrev.com/59b6ea2217dbc10400b6a9d433ad13c91bb6b7c2 Committed: https://crrev.com/2c7cd56d182ae1a14191170c47154641a24633fa Cr-Original-Commit-Position: refs/heads/master@{#403798} Cr-Commit-Position: refs/heads/master@{#404199} ==========
Message was sent while issue was closed.
Patchset 29 (id:??) landed as https://crrev.com/2c7cd56d182ae1a14191170c47154641a24633fa Cr-Commit-Position: refs/heads/master@{#404199}
Message was sent while issue was closed.
primiano@chromium.org changed reviewers: + primiano@chromium.org
Message was sent while issue was closed.
This patch seems to have landed with some rebase conflicts, which dropped unittests from the build files. Can somebody re-audit this patch ? https://codereview.chromium.org/2066553002/diff/590001/components/components_... File components/components_tests.gyp (left): https://codereview.chromium.org/2066553002/diff/590001/components/components_... components/components_tests.gyp:633: 'proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc', I assume you didn't mean to drop this test on the floor either? https://codereview.chromium.org/2066553002/diff/590001/components/components_... components/components_tests.gyp:886: 'tracing/core/trace_ring_buffer_unittest.cc', Please be more careful with rebases. You just landed this change and dropped some tests on the floor. I luckily realized this just because I had a local merge conflict.
Message was sent while issue was closed.
Sorry about that, I'm surprised I didn't get any merge conflicts when I rebased. No, the only thing I intended to remove from build files was the conditional compilation of some DNS-related code. On Fri, 8 Jul 2016, 09:21 , <primiano@chromium.org> wrote: > This patch seems to have landed with some rebase conflicts, which dropped > unittests from the build files. > Can somebody re-audit this patch ? > > > > https://codereview.chromium.org/2066553002/diff/590001/components/components_... > File components/components_tests.gyp (left): > > > https://codereview.chromium.org/2066553002/diff/590001/components/components_... > components/components_tests.gyp:633: > > 'proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc', > I assume you didn't mean to drop this test on the floor either? > > > https://codereview.chromium.org/2066553002/diff/590001/components/components_... > components/components_tests.gyp:886: > 'tracing/core/trace_ring_buffer_unittest.cc', > Please be more careful with rebases. You just landed this change and > dropped some tests on the floor. > I luckily realized this just because I had a local merge conflict. > > https://codereview.chromium.org/2066553002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Re-added unittests to build file in https://codereview.chromium.org/2129113002/. Looks like they were accidentally dropped by patch set 21 and missed by both myself and my reviewers. https://codereview.chromium.org/2066553002/diff/590001/components/components_... File components/components_tests.gyp (left): https://codereview.chromium.org/2066553002/diff/590001/components/components_... components/components_tests.gyp:633: 'proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc', On 2016/07/08 08:21:46, Primiano Tucci wrote: > I assume you didn't mean to drop this test on the floor either? Acknowledged. https://codereview.chromium.org/2066553002/diff/590001/components/components_... components/components_tests.gyp:886: 'tracing/core/trace_ring_buffer_unittest.cc', On 2016/07/08 08:21:46, Primiano Tucci wrote: > Please be more careful with rebases. You just landed this change and dropped > some tests on the floor. > I luckily realized this just because I had a local merge conflict. Done.
Message was sent while issue was closed.
On 2016/07/08 08:51:06, Rob Percival wrote: > Re-added unittests to build file in https://codereview.chromium.org/2129113002/. > Looks like they were accidentally dropped by patch set 21 and missed by both > myself and my reviewers. Yup saw that. I started a thread on chromium-dev (https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GrZTVNcvlyE) To be clear, I can see how these kind of conflicts can happen. It's just unfortunate that we don't have any mechanism in place to prevent that. Some presubmit / CQ test should have went red here. Instead scarily this made its way to master (almost) unnoticed :-/ |
