Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2661433002:
Introduce VerifiedRulesetDealer and its async Handle. (Closed)
|
Created:
3 years, 11 months ago by pkalinnikov Modified:
3 years, 10 months ago Reviewers:
engedy CC:
chromium-reviews, jam, darin-cc_chromium.org, mlamouri+watch-content_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce VerifiedRulesetDealer and its async Handle.
VerifiedRulesetDealer (VRD) is an non-thread-safe class that manages
memory-mapping and distributing the ruleset like RulesetDealer, but
additionally does integrity checking the first time before handing out
a given version of the ruleset.
VRD::Handle is a non-thread-safe counterpart of VRD that, regardless of the
thread it lives on, associates an instance of VRD with a SequencedTaskRunner
and provides asynchronous access to it using PostTask internally. Properties of
the SequencedTaskRunner guarantee sequential ordering of the accesses.
The VRD will usually live on a blocking pool thread because it does blocking
IO, and the Handle facilitates accessing it from the UI thread.
BUG=637415
Review-Url: https://codereview.chromium.org/2661433002
Cr-Commit-Position: refs/heads/master@{#447494}
Committed: https://chromium.googlesource.com/chromium/src/+/ab97ff6908a975ddf24b72fef99d2539d8d43aa0
Patch Set 1 #Patch Set 2 : Add forgotten files. #Patch Set 3 : Clean-up. #
Total comments: 14
Patch Set 4 : Address comments; remove AsyncPtr; add unittests. #Patch Set 5 : Add build dependency. #Patch Set 6 : Add one more build dependency. #
Total comments: 6
Patch Set 7 : Minor changes; clean up unittest. #
Total comments: 29
Patch Set 8 : Address comments; add more tests. #
Total comments: 22
Patch Set 9 : Address more comments; refactor tests; add test. #
Total comments: 20
Patch Set 10 : Refactor tests. #Dependent Patchsets: Messages
Total messages: 59 (43 generated)
The CQ bit was checked by pkalinnikov@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 checked by pkalinnikov@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...
pkalinnikov@chromium.org changed reviewers: + engedy@chromium.org
Balazs, PTAL. I will add unittests for VerifiedRulesetDealer soon.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looking good, some initial comments. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:34: // Memory maps the subresource filtering ruleset file received through It'd probably suffice to say here that this is same as RulesetDealer, but additionally does integrity checking on the ruleset before handing it out. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:39: class VerifiedRulesetDealer : public RulesetDealer { For most of these classes, it would be nice to use a base::ThreadChecker (detached in the ctor) to enforce correct usage. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:46: // Sets the |ruleset_file| to memory map and distribute from now on. Resets nit: For overrides, just use the following format: // RulesetDealer: void SetRulesetFile(base::File ruleset_file) override; scoped_refptr<const MemoryMappedRuleset> GetRuleset() override; https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:54: RulesetVerificationStatus status = RulesetVerificationStatus::UNVERIFIED; nit: status_ https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:90: base::SequencedTaskRunner* task_runner_; Same here, let's make this a scoped_refptr. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... File components/subresource_filter/core/common/async_ptr.h (right): https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/core/common/async_ptr.h:31: base::SequencedTaskRunner* task_runner_; This is not holding a reference to the task runner, should we use scoped_refptr<base::SequencedTaskRunner> instead? https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/core/common/async_ptr.h:37: using AsyncPtr = std::unique_ptr<T, impl::AsyncPtrDeleter<T>>; Let's try to pick a name where there is not a chance for confusion: There is nothing async about the pointer, it is just deleted on a task runner asynchronously. My first naming idea for the deleter would be "OnTaskRunnerDeleter".
PTAL. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:34: // Memory maps the subresource filtering ruleset file received through On 2017/01/27 09:47:11, engedy wrote: > It'd probably suffice to say here that this is same as RulesetDealer, but > additionally does integrity checking on the ruleset before handing it out. Done. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:39: class VerifiedRulesetDealer : public RulesetDealer { On 2017/01/27 09:47:11, engedy wrote: > For most of these classes, it would be nice to use a base::ThreadChecker > (detached in the ctor) to enforce correct usage. Done. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:46: // Sets the |ruleset_file| to memory map and distribute from now on. Resets On 2017/01/27 09:47:11, engedy wrote: > nit: For overrides, just use the following format: > > // RulesetDealer: > void SetRulesetFile(base::File ruleset_file) override; > scoped_refptr<const MemoryMappedRuleset> GetRuleset() override; Done. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:54: RulesetVerificationStatus status = RulesetVerificationStatus::UNVERIFIED; On 2017/01/27 09:47:11, engedy wrote: > nit: status_ Done. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/content/browser/verified_ruleset_dealer.h:90: base::SequencedTaskRunner* task_runner_; On 2017/01/27 09:47:11, engedy wrote: > Same here, let's make this a scoped_refptr. There is no need in it here, because |dealer_| owns the |task_runner_| in its deleter. Added comment. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... File components/subresource_filter/core/common/async_ptr.h (right): https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/core/common/async_ptr.h:31: base::SequencedTaskRunner* task_runner_; On 2017/01/27 09:47:11, engedy wrote: > This is not holding a reference to the task runner, should we use > scoped_refptr<base::SequencedTaskRunner> instead? Not relevant any more. Using base::OnTaskRunner instead, which btw has a scoped_refptr inside. https://codereview.chromium.org/2661433002/diff/40001/components/subresource_... components/subresource_filter/core/common/async_ptr.h:37: using AsyncPtr = std::unique_ptr<T, impl::AsyncPtrDeleter<T>>; On 2017/01/27 09:47:11, engedy wrote: > Let's try to pick a name where there is not a chance for confusion: There is > nothing async about the pointer, it is just deleted on a task runner > asynchronously. My first naming idea for the deleter would be > "OnTaskRunnerDeleter". Removing this file because base::OnTaskRunnerDeleter already exists :) As discussed, having just a typedef might be even more confusing than using a std::unique_ptr with a deleter explicitly.
The CQ bit was checked by pkalinnikov@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 pkalinnikov@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
The CQ bit was checked by pkalinnikov@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Introduce VerifiedRulesetDealer and its async Handler. BUG=637415 ========== to ========== Introduce VerifiedRulesetDealer and its async Handle. BUG=637415 ==========
Description was changed from ========== Introduce VerifiedRulesetDealer and its async Handle. BUG=637415 ========== to ========== Introduce VerifiedRulesetDealer and its async Handle. VerifiedRulesetDealer (VRD) is an non-thread-safe class that manages memory-mapping and distributing the ruleset like RulesetDealer, but additionally does integrity checking once per ruleset file before handing it out. VRD::Handle is a non-thread-safe counterpart of VRD that, regardless of the thread it lives on, associates an instance of VRD with a SequencedTaskRunner and provides asynchronous access to it using PostTask internally. Properties of the SequencedTaskRunner guarantee sequential ordering of the accesses. BUG=637415 ==========
The CQ bit was checked by pkalinnikov@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...
Please see patch#7.
Flushing pending comments on older patchset. https://codereview.chromium.org/2661433002/diff/100001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/100001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:42: class VerifiedRulesetDealer : public RulesetDealer, base::NonThreadSafe { According to https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?rcl=0&l=..., it is preferred to have a ThreadChecker as a member in a case such as here. Alternatively, we could mark RulesetDealer as NonThreadSafe, which, actually, would make sense. https://codereview.chromium.org/2661433002/diff/100001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:62: // Owns a VerifiedRulesetDealer living on a dedicated sequenced |task_runner|. nit: The UI-thread handle that owns ... https://codereview.chromium.org/2661433002/diff/100001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:87: // Note: The |task_runner_| is owned by |dealer_|, no need in scoped_refptr. Phrasing nit: Raw pointer, the |dealer_| already holds a reference to |task_runner_|.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some more comments. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:25: // State of the last ruleset file known to a VerifiedRulesetDealer. Phrasing nit: Integrity verification status of a given ruleset version. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:30: enum class RulesetVerificationStatus { What do you think about putting more emphasis on `integrity` in comments and naming? E.g., this would be: { NOT_VERIFIED, INTACT, CORRUPT } https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:50: RulesetVerificationStatus status() const; nit: Same here, can we make the test fixture a friend instead? https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:66: // Creates a VerifiedRulesetDealer and semantically associates it with the Phrasing suggestion: // Creates a VerifiedRulesetDealer that is owned by this handle, accessed through this handle, but lives on |task_runner|. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:70: // Destroys |this| handle and asynchronously deletes the owned nit: I'd drop this comment. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:77: // NOTE: Properties of the sequenced |task_runner| guarantee that the nit: Move this comment to implementation. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/content/common/ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/common/ruleset_dealer.h:43: bool IsRulesetFileAvailable() const; I wonder how useful this information is for clients of this class -- if they will still have to not-null-check the result of GetRuleset() later. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/common/ruleset_dealer.h:50: // For testing only. Would it be possible here to just mark the new test fixtures as friends instead? https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/core/common/test_ruleset_creator.cc (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/core/common/test_ruleset_creator.cc:72: ASSERT_TRUE(base::ReadFileToString(path, &file_contents)); nit: No need to read the file, the data is already there in |contents|. nit: Let's stick to using vector<uint8_t> here. File operations taking const char* is a mistake, let's do here what we do in CreateTestRulesetFromContents. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/core/common/test_ruleset_creator.h (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/core/common/test_ruleset_creator.h:35: void CorruptFile(size_t bytes_to_cut_from_end = 0); As discussed offline, it's probably worthwhile to be able to emulate both of the two most likely corruptions, i.e. CorruptWithZeroes, CorruptByTruncation, and have a unittest for each. Also, note that, as per the style guide, a struct is not supposed to have any methods with any real logic. One option is to make these static functions.
Last set of comments, will take a final look afterwards. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.cc (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.cc:30: scoped_refptr<const MemoryMappedRuleset> VerifiedRulesetDealer::GetRuleset() { nit: To increase readability, how about making this a single nice switch statement based on |status_|? https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.cc:31: if (status() == RulesetVerificationStatus::INVALID) nit: Add CalledOnValidThread() at the start of this method. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.cc:75: base::Bind(task, base::Unretained(dealer_.get()), base::Passed(&file))); You should be able to reduce this to: base::Bind(&VerifiedRulesetDealer::SetRulesetFile, dealer_.get(), base::Passed(&file));
PTAL. Resolved all nits but the ones about friendship. https://codereview.chromium.org/2661433002/diff/100001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/100001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:42: class VerifiedRulesetDealer : public RulesetDealer, base::NonThreadSafe { On 2017/01/30 10:55:01, engedy wrote: > According to > https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?rcl=0&l=..., > it is preferred to have a ThreadChecker as a member in a case such as here. > Alternatively, we could mark RulesetDealer as NonThreadSafe, which, actually, > would make sense. Done. https://codereview.chromium.org/2661433002/diff/100001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:62: // Owns a VerifiedRulesetDealer living on a dedicated sequenced |task_runner|. On 2017/01/30 10:55:01, engedy wrote: > nit: The UI-thread handle that owns ... Done. https://codereview.chromium.org/2661433002/diff/100001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:87: // Note: The |task_runner_| is owned by |dealer_|, no need in scoped_refptr. On 2017/01/30 10:55:01, engedy wrote: > Phrasing nit: Raw pointer, the |dealer_| already holds a reference to > |task_runner_|. Done. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.cc (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.cc:30: scoped_refptr<const MemoryMappedRuleset> VerifiedRulesetDealer::GetRuleset() { On 2017/01/30 15:59:34, engedy wrote: > nit: To increase readability, how about making this a single nice switch > statement based on |status_|? Done. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.cc:31: if (status() == RulesetVerificationStatus::INVALID) On 2017/01/30 15:59:34, engedy wrote: > nit: Add CalledOnValidThread() at the start of this method. Done. It actually wasn't necessary because status() had been checking it already. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.cc:75: base::Bind(task, base::Unretained(dealer_.get()), base::Passed(&file))); On 2017/01/30 15:59:34, engedy wrote: > You should be able to reduce this to: > > base::Bind(&VerifiedRulesetDealer::SetRulesetFile, dealer_.get(), > base::Passed(&file)); Of course. Thanks :) https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:25: // State of the last ruleset file known to a VerifiedRulesetDealer. On 2017/01/30 13:27:36, engedy wrote: > Phrasing nit: Integrity verification status of a given ruleset version. Done. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:30: enum class RulesetVerificationStatus { On 2017/01/30 13:27:36, engedy wrote: > What do you think about putting more emphasis on `integrity` in comments and > naming? E.g., this would be: { NOT_VERIFIED, INTACT, CORRUPT } Done. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:50: RulesetVerificationStatus status() const; On 2017/01/30 13:27:36, engedy wrote: > nit: Same here, can we make the test fixture a friend instead? I don't much like this, because it would also lead to polluting the fixture with a function like get_dealer_status(dealer) { return dealer.status() }. Do you feel strongly about this? https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:66: // Creates a VerifiedRulesetDealer and semantically associates it with the On 2017/01/30 13:27:36, engedy wrote: > Phrasing suggestion: > > // Creates a VerifiedRulesetDealer that is owned by this handle, accessed > through this handle, but lives on |task_runner|. Done. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:70: // Destroys |this| handle and asynchronously deletes the owned On 2017/01/30 13:27:36, engedy wrote: > nit: I'd drop this comment. Done. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:77: // NOTE: Properties of the sequenced |task_runner| guarantee that the On 2017/01/30 13:27:37, engedy wrote: > nit: Move this comment to implementation. Done. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/content/common/ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/common/ruleset_dealer.h:43: bool IsRulesetFileAvailable() const; On 2017/01/30 13:27:37, engedy wrote: > I wonder how useful this information is for clients of this class -- if they > will still have to not-null-check the result of GetRuleset() later. I think it's just as useful as the old RulesetDealer's IsRulesetAvailable method. We even have a UMA histogram collecting this bit of information. For the regular RulesetDealer IsRulesetFileAvailable() == true iff GetRuleset() returns non-null. For the verified dealer users it's just a good clue, i.e., IsRulesetAvailable() == false ==> GetRuleset() will return nullptr. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/common/ruleset_dealer.h:50: // For testing only. On 2017/01/30 13:27:37, engedy wrote: > Would it be possible here to just mark the new test fixtures as friends instead? But they are in content/browser, and this one is in content/common. Is common allowed to know about browser like that? https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/core/common/test_ruleset_creator.cc (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/core/common/test_ruleset_creator.cc:72: ASSERT_TRUE(base::ReadFileToString(path, &file_contents)); On 2017/01/30 13:27:37, engedy wrote: > nit: No need to read the file, the data is already there in |contents|. > > nit: Let's stick to using vector<uint8_t> here. File operations taking const > char* is a mistake, let's do here what we do in CreateTestRulesetFromContents. Done. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/core/common/test_ruleset_creator.h (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/core/common/test_ruleset_creator.h:35: void CorruptFile(size_t bytes_to_cut_from_end = 0); On 2017/01/30 13:27:37, engedy wrote: > As discussed offline, it's probably worthwhile to be able to emulate both of the > two most likely corruptions, i.e. CorruptWithZeroes, CorruptByTruncation, and > have a unittest for each. > > Also, note that, as per the style guide, a struct is not supposed to have any > methods with any real logic. One option is to make these static functions. Done.
The CQ bit was checked by pkalinnikov@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...
https://codereview.chromium.org/2661433002/diff/140001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:27: // A ruleset file starts from the UNVERIFIED state, after which it can be NOT_VERIFIED https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:40: // |this| object, and is reset to UNVERIFIED only when a new ruleset is NOT_VERIFIED
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Looking great, LGTM % comments. Also, could you please point out in the second paragraph of the CL description that the VRD will usually live on a blocking pool thread because it does blocking IO, and the handle facilitates accessing it from the UI thread? https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:50: RulesetVerificationStatus status() const; On 2017/01/30 17:14:11, pkalinnikov wrote: > On 2017/01/30 13:27:36, engedy wrote: > > nit: Same here, can we make the test fixture a friend instead? > > I don't much like this, because it would also lead to polluting the fixture with > a function like get_dealer_status(dealer) { return dealer.status() }. Do you > feel strongly about this? Yeah, that's the cost. But with the cross core-browser dependencies, let's just keep it as it is now. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... File components/subresource_filter/content/common/ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/common/ruleset_dealer.h:43: bool IsRulesetFileAvailable() const; On 2017/01/30 17:14:12, pkalinnikov wrote: > On 2017/01/30 13:27:37, engedy wrote: > > I wonder how useful this information is for clients of this class -- if they > > will still have to not-null-check the result of GetRuleset() later. > > I think it's just as useful as the old RulesetDealer's IsRulesetAvailable > method. We even have a UMA histogram collecting this bit of information. For the > regular RulesetDealer IsRulesetFileAvailable() == true iff GetRuleset() returns > non-null. For the verified dealer users it's just a good clue, i.e., > IsRulesetAvailable() == false ==> GetRuleset() will return nullptr. Fair enough. https://codereview.chromium.org/2661433002/diff/120001/components/subresource... components/subresource_filter/content/common/ruleset_dealer.h:50: // For testing only. On 2017/01/30 17:14:11, pkalinnikov wrote: > On 2017/01/30 13:27:37, engedy wrote: > > Would it be possible here to just mark the new test fixtures as friends > instead? > > But they are in content/browser, and this one is in content/common. Is common > allowed to know about browser like that? Great question, I think it would be okay, but I don't feel strongly about this. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.cc (right): https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.cc:42: if (!ruleset) How can this happen? https://codereview.chromium.org/2661433002/diff/140001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:25: // Integrity verification status of a given ruleset version. nit (sorry): The integrity... https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:29: // INTACT ruleset can become CORRUPT due to an error. nit: Can this kind of state transition really happen? https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:41: // substituted to SetRulesetFile() method. nit: s/substituted/supplied|provided/ https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:72: // Posts a task to |task_runner| to access the owned VerifiedRulesetDealer via nit: Let's phrase this and the next comment so that they are more informative to the caller: // Invokes |callback| on |task_runner|, providing a pointer to the underlying VerifiedRulesetDealer as an argument. The pointer is guaranteed to be valid at least until the callback returns. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:76: // Posts a task to |task_runner| to set the dealer's ruleset |file|. Same here: // Sets the |ruleset_file| that the VerifiedRulesetDealer should distribute from now on. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc (right): https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:138: testing::TestRuleset::CorruptByFilling(test_indexed_ruleset_1(), 2501, 4000, nit: Could you please provide corresponding argument names next to these values in /* */? https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:215: TEST(SubresourceFilterVerifiedRulesetDealerHandlerTest, nit: s/Handler/Handle/ https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:227: base::subtle::Atomic32 order = 0; Let's see if we can avoid atomics here, and maybe use base/test/mock_callback.h in conjuction with GoogleMock EXPECT_CALLs.
The CQ bit was checked by pkalinnikov@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...
Description was changed from ========== Introduce VerifiedRulesetDealer and its async Handle. VerifiedRulesetDealer (VRD) is an non-thread-safe class that manages memory-mapping and distributing the ruleset like RulesetDealer, but additionally does integrity checking once per ruleset file before handing it out. VRD::Handle is a non-thread-safe counterpart of VRD that, regardless of the thread it lives on, associates an instance of VRD with a SequencedTaskRunner and provides asynchronous access to it using PostTask internally. Properties of the SequencedTaskRunner guarantee sequential ordering of the accesses. BUG=637415 ========== to ========== Introduce VerifiedRulesetDealer and its async Handle. VerifiedRulesetDealer (VRD) is an non-thread-safe class that manages memory-mapping and distributing the ruleset like RulesetDealer, but additionally does integrity checking once per ruleset file before handing it out. VRD::Handle is a non-thread-safe counterpart of VRD that, regardless of the thread it lives on, associates an instance of VRD with a SequencedTaskRunner and provides asynchronous access to it using PostTask internally. Properties of the SequencedTaskRunner guarantee sequential ordering of the accesses. The VRD will usually live on a blocking pool thread because it does blocking IO, and the Handle facilitates accessing it from the UI thread. BUG=637415 ==========
PTAL. Changed the way how test callbacks work. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.cc (right): https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.cc:42: if (!ruleset) On 2017/01/31 13:10:56, engedy wrote: > How can this happen? Only if a base::File can become !IsValid() after being IsValid(), resulting in RulesetDealer::GetRuleset returning nullptr after non-nullptr. Seems like it's not possible, so let me put a DCHECK here. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:25: // Integrity verification status of a given ruleset version. On 2017/01/31 13:10:56, engedy wrote: > nit (sorry): The integrity... Done. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:27: // A ruleset file starts from the UNVERIFIED state, after which it can be On 2017/01/30 17:26:05, pkalinnikov wrote: > NOT_VERIFIED Done. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:29: // INTACT ruleset can become CORRUPT due to an error. On 2017/01/31 13:10:56, engedy wrote: > nit: Can this kind of state transition really happen? Removed this transition. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:40: // |this| object, and is reset to UNVERIFIED only when a new ruleset is On 2017/01/30 17:26:05, pkalinnikov wrote: > NOT_VERIFIED Done. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:41: // substituted to SetRulesetFile() method. On 2017/01/31 13:10:56, engedy wrote: > nit: s/substituted/supplied|provided/ Done. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:72: // Posts a task to |task_runner| to access the owned VerifiedRulesetDealer via On 2017/01/31 13:10:56, engedy wrote: > nit: Let's phrase this and the next comment so that they are more informative to > the caller: > > // Invokes |callback| on |task_runner|, providing a pointer to the underlying > VerifiedRulesetDealer as an argument. The pointer is guaranteed to be valid at > least until the callback returns. Done. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer.h:76: // Posts a task to |task_runner| to set the dealer's ruleset |file|. On 2017/01/31 13:10:56, engedy wrote: > Same here: > > // Sets the |ruleset_file| that the VerifiedRulesetDealer should distribute from > now on. Done. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc (right): https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:138: testing::TestRuleset::CorruptByFilling(test_indexed_ruleset_1(), 2501, 4000, On 2017/01/31 13:10:56, engedy wrote: > nit: Could you please provide corresponding argument names next to these values > in /* */? Done. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:215: TEST(SubresourceFilterVerifiedRulesetDealerHandlerTest, On 2017/01/31 13:10:56, engedy wrote: > nit: s/Handler/Handle/ Done. https://codereview.chromium.org/2661433002/diff/140001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:227: base::subtle::Atomic32 order = 0; On 2017/01/31 13:10:56, engedy wrote: > Let's see if we can avoid atomics here, and maybe use base/test/mock_callback.h > in conjuction with GoogleMock EXPECT_CALLs. Atomics are indeed unnecessary, because TestSimpleTaskRunner operates on the same thread. Mock are problematic to use because we need a custom code in the callbacks to verify the state of the ruleset. What do you think about the new approach with named callbacks?
Description was changed from ========== Introduce VerifiedRulesetDealer and its async Handle. VerifiedRulesetDealer (VRD) is an non-thread-safe class that manages memory-mapping and distributing the ruleset like RulesetDealer, but additionally does integrity checking once per ruleset file before handing it out. VRD::Handle is a non-thread-safe counterpart of VRD that, regardless of the thread it lives on, associates an instance of VRD with a SequencedTaskRunner and provides asynchronous access to it using PostTask internally. Properties of the SequencedTaskRunner guarantee sequential ordering of the accesses. The VRD will usually live on a blocking pool thread because it does blocking IO, and the Handle facilitates accessing it from the UI thread. BUG=637415 ========== to ========== Introduce VerifiedRulesetDealer and its async Handle. VerifiedRulesetDealer (VRD) is an non-thread-safe class that manages memory-mapping and distributing the ruleset like RulesetDealer, but additionally does integrity checking the first time before handing out a given version of the ruleset. VRD::Handle is a non-thread-safe counterpart of VRD that, regardless of the thread it lives on, associates an instance of VRD with a SequencedTaskRunner and provides asynchronous access to it using PostTask internally. Properties of the SequencedTaskRunner guarantee sequential ordering of the accesses. The VRD will usually live on a blocking pool thread because it does blocking IO, and the Handle facilitates accessing it from the UI thread. BUG=637415 ==========
LGTM % nits and simplification suggestions. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc (right): https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:47: testing::TestRuleset& indexed_1() { return test_ruleset_pair_1_.indexed; } nit: Can these be consts? https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:58: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:88: TestRulesets& rulesets() { return rulesets_; } nit: Same here, can this be const? https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:208: class TestDealerCallback { Naming nit: How about TestVerifiedRulesetDealerClient? https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:210: void Callback(bool read_ruleset, VerifiedRulesetDealer* dealer) { nit: If we moved the functions that creates the base::Callback into this calls, we could make this method private. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:214: if (read_ruleset) { Simplification suggestion: How about moving this block at the very end of the callback, and then trying to read the ruleset unconditionally? The first client could still verify the circumstances. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:251: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:288: std::unique_ptr<VerifiedRulesetDealer> ruleset_dealer_; nit: Unused. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:304: dealer_handle->GetDealerAsync(NewCallback("after_set_ruleset")); Simplification suggestion: Can we just make local instances of callbacks here, and avoid the map and helper functions above? TestVerifiedRulesetDealerClient before_set_ruleset; TestVerifiedRulesetDealerClient after_set_ruleset; TestVerifiedRulesetDealerClient after_warm_up; https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:320: dealer_handle->GetDealerAsync(NewCallback("before_set_ruleset")); nit: This is already tested above, let's avoid it here for brevity.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2661433002/diff/160001/components/subresource... File components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc (right): https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:47: testing::TestRuleset& indexed_1() { return test_ruleset_pair_1_.indexed; } On 2017/02/01 10:09:33, engedy wrote: > nit: Can these be consts? Done. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:58: }; On 2017/02/01 10:09:33, engedy wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:88: TestRulesets& rulesets() { return rulesets_; } On 2017/02/01 10:09:33, engedy wrote: > nit: Same here, can this be const? Done. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:208: class TestDealerCallback { On 2017/02/01 10:09:33, engedy wrote: > Naming nit: How about TestVerifiedRulesetDealerClient? Done. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:210: void Callback(bool read_ruleset, VerifiedRulesetDealer* dealer) { On 2017/02/01 10:09:33, engedy wrote: > nit: If we moved the functions that creates the base::Callback into this calls, > we could make this method private. Done. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:214: if (read_ruleset) { On 2017/02/01 10:09:33, engedy wrote: > Simplification suggestion: How about moving this block at the very end of the > callback, and then trying to read the ruleset unconditionally? The first client > could still verify the circumstances. Done. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:251: }; On 2017/02/01 10:09:33, engedy wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:288: std::unique_ptr<VerifiedRulesetDealer> ruleset_dealer_; On 2017/02/01 10:09:33, engedy wrote: > nit: Unused. Done. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:304: dealer_handle->GetDealerAsync(NewCallback("after_set_ruleset")); On 2017/02/01 10:09:33, engedy wrote: > Simplification suggestion: Can we just make local instances of callbacks here, > and avoid the map and helper functions above? > > TestVerifiedRulesetDealerClient before_set_ruleset; > TestVerifiedRulesetDealerClient after_set_ruleset; > TestVerifiedRulesetDealerClient after_warm_up; Done. https://codereview.chromium.org/2661433002/diff/160001/components/subresource... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:320: dealer_handle->GetDealerAsync(NewCallback("before_set_ruleset")); On 2017/02/01 10:09:33, engedy wrote: > nit: This is already tested above, let's avoid it here for brevity. Done.
The CQ bit was checked by engedy@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...
LGTM, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pkalinnikov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485953660399930,
"parent_rev": "ccc17b815af700b93551fdf23164dfb6eee7aada", "commit_rev":
"ab97ff6908a975ddf24b72fef99d2539d8d43aa0"}
Message was sent while issue was closed.
Description was changed from ========== Introduce VerifiedRulesetDealer and its async Handle. VerifiedRulesetDealer (VRD) is an non-thread-safe class that manages memory-mapping and distributing the ruleset like RulesetDealer, but additionally does integrity checking the first time before handing out a given version of the ruleset. VRD::Handle is a non-thread-safe counterpart of VRD that, regardless of the thread it lives on, associates an instance of VRD with a SequencedTaskRunner and provides asynchronous access to it using PostTask internally. Properties of the SequencedTaskRunner guarantee sequential ordering of the accesses. The VRD will usually live on a blocking pool thread because it does blocking IO, and the Handle facilitates accessing it from the UI thread. BUG=637415 ========== to ========== Introduce VerifiedRulesetDealer and its async Handle. VerifiedRulesetDealer (VRD) is an non-thread-safe class that manages memory-mapping and distributing the ruleset like RulesetDealer, but additionally does integrity checking the first time before handing out a given version of the ruleset. VRD::Handle is a non-thread-safe counterpart of VRD that, regardless of the thread it lives on, associates an instance of VRD with a SequencedTaskRunner and provides asynchronous access to it using PostTask internally. Properties of the SequencedTaskRunner guarantee sequential ordering of the accesses. The VRD will usually live on a blocking pool thread because it does blocking IO, and the Handle facilitates accessing it from the UI thread. BUG=637415 Review-Url: https://codereview.chromium.org/2661433002 Cr-Commit-Position: refs/heads/master@{#447494} Committed: https://chromium.googlesource.com/chromium/src/+/ab97ff6908a975ddf24b72fef99d... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/ab97ff6908a975ddf24b72fef99d... |
