Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1643573002:
Add a ModuleLoadAnalyzer which checks modules against a whitelist (Closed)
DescriptionAdd a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules.
The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial.
BUG=585123
Committed: https://crrev.com/908da69d25746389d4b695e989b3df5e31e88341
Cr-Commit-Position: refs/heads/master@{#377604}
Patch Set 1 #Patch Set 2 : Remove accidentally added rsp files from the CL #Patch Set 3 : Only enable for extended reporting AND feature flag. Add unit test. #
Total comments: 12
Patch Set 4 : Update safe_browsing_db unit test and minor cleanups #
Total comments: 4
Patch Set 5 : Address comments for patchsets #3 and #4 #
Total comments: 4
Patch Set 6 : abort_reporting to static field, add test for shutdown case #
Total comments: 2
Patch Set 7 : s/soon when/soon as #
Total comments: 9
Patch Set 8 : Make module_load_analyzer classless, remove static global and use CONTINUE_ON_SHUTDOWN #
Total comments: 50
Patch Set 9 : Resolve comments on #8 and add consent level to Incidents #
Total comments: 67
Patch Set 10 : Address comments on #9 #Patch Set 11 : Rebase-update after dependent CL submitted #
Total comments: 2
Patch Set 12 : Fix incorrect and misleading unit tests in for blacklist_load and suspicious_module incidents #
Total comments: 2
Patch Set 13 : rebase-update'd #Messages
Total messages: 47 (15 generated)
Description was changed from ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. BUG= ========== to ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. BUG= ==========
proberge@chromium.org changed reviewers: + robertshield@chromium.org, veranika@chromium.org
Please only review what's inside the incident_reporting folder, I'll have a safe_browsing owner look at the rest of the code in a different CL, which will be submitted first.
https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h (right): https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h:102: // a profile participating in extended reporting is found. Add to the CL description that this also adds the ability to register reporting work only for people in the extended reporting group. https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc (right): https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:14: #if defined(OS_WIN) would prefer to have this in the _win.cc file. This could be done by moving the initialization in the constructor to an "Initialize" method or some such that has an empty stub implementation here and a real implementation in the _win.cc file. https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:39: std::set<base::string16>* suspicious_names = new std::set<base::string16>(); could you stick this right away in a scoped_ptr? base::Passed() below will do the right thing given a scoped_ptr since it is a movable type. https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:95: UMA_HISTOGRAM_TIMES("SBIRS.BLAHashTime", This will fire a histogram for each item in the module list, would it be better to accumulate the total time spent and report a histogram once? https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:130: incident_receiver_ = std::move(incident_receiver); keeping the incident_receiver_ in a local var that is cleared during this process makes me wonder about what will happen if VerifyModuleLoadState is called twice in succession. Seems like there are a number of permutations of callbacks that make that a little hard to predict. If this is correct, might be worth adding a DCHECK here to guard against multiple calls to VerifyModuleLoadState on the same instance of a ModuleLoadAnalyzer. https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:85: // module whitelist. Just curious, have we tried a massive whitelist here to see what it does to performance?
Description was changed from ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. BUG= ========== to ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Incident Reporting is found. In addition, a feature flag must be enabled through the command line or a feature flag. BUG= ==========
Description was changed from ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Incident Reporting is found. In addition, a feature flag must be enabled through the command line or a feature flag. BUG= ========== to ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a feature flag. BUG= ==========
Description was changed from ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a feature flag. BUG= ========== to ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial. BUG= ==========
https://codereview.chromium.org/1643573002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/1643573002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:411: prefs::kSafeBrowsingExtendedReportingEnabled)) Wouldn't it be better to use IncidentReportingService::IsEnabledForProfile here? https://codereview.chromium.org/1643573002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:504: prefs::kSafeBrowsingExtendedReportingEnabled)) { Same here.
https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h (right): https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h:102: // a profile participating in extended reporting is found. On 2016/02/05 21:20:32, robertshield wrote: > Add to the CL description that this also adds the ability to register reporting > work only for people in the extended reporting group. Done. https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc (right): https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:14: #if defined(OS_WIN) On 2016/02/05 21:20:32, robertshield wrote: > would prefer to have this in the _win.cc file. This could be done by moving the > initialization in the constructor to an "Initialize" method or some such that > has an empty stub implementation here and a real implementation in the _win.cc > file. > Done. https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:39: std::set<base::string16>* suspicious_names = new std::set<base::string16>(); On 2016/02/05 21:20:32, robertshield wrote: > could you stick this right away in a scoped_ptr? base::Passed() below will do > the right thing given a scoped_ptr since it is a movable type. > Great catch, curently I would leak the set if suspicious_names->empty(). https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:95: UMA_HISTOGRAM_TIMES("SBIRS.BLAHashTime", On 2016/02/05 21:20:32, robertshield wrote: > This will fire a histogram for each item in the module list, would it be better > to accumulate the total time spent and report a histogram once? Yes. Removed the existing histograms (used by the blacklist incident) and replaced with 3 different histograms. https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:130: incident_receiver_ = std::move(incident_receiver); On 2016/02/05 21:20:32, robertshield wrote: > keeping the incident_receiver_ in a local var that is cleared during this > process makes me wonder about what will happen if VerifyModuleLoadState is > called twice in succession. Seems like there are a number of permutations of > callbacks that make that a little hard to predict. If this is correct, might be > worth adding a DCHECK here to guard against multiple calls to > VerifyModuleLoadState on the same instance of a ModuleLoadAnalyzer. Done. https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/1643573002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:85: // module whitelist. On 2016/02/05 21:20:32, robertshield wrote: > Just curious, have we tried a massive whitelist here to see what it does to > performance? I have not. afaik there's no simple way to fake a part of the chunk server. https://codereview.chromium.org/1643573002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/1643573002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:411: prefs::kSafeBrowsingExtendedReportingEnabled)) On 2016/02/05 22:15:10, veranika wrote: > Wouldn't it be better to use IncidentReportingService::IsEnabledForProfile here? isEnabledForProfile can return true for a profile without safe browsing extended reporting. We want this new trigger to only be enabled for users with extended reporting. https://codereview.chromium.org/1643573002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:504: prefs::kSafeBrowsingExtendedReportingEnabled)) { On 2016/02/05 22:15:10, veranika wrote: > Same here. Acknowledged.
Sending one comment now, still completing second pass. https://codereview.chromium.org/1643573002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:103: if (abort_reporting_) As discussed, if this is true, then this method is running for a deleted object, meaning the this pointer is (maybe) referring to somewhere bogus. Suggest making this method static, passing in incident_receiver_ as a parameter and having a static abort_reporting_ field.
https://codereview.chromium.org/1643573002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:89: } Is it possible to test the shut down case? Something like: { ModuleLoadAnalyzer module_load_analyzer_; module_load_analyzer_.VerifyModuleLoadState(make_scoped_ptr(mock_incident_receiver_)); } base::RunLoop().RunUntilIdle(); content::RunAllBlockingPoolTasksUntilIdle();
https://codereview.chromium.org/1643573002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:103: if (abort_reporting_) On 2016/02/08 16:22:02, robertshield wrote: > As discussed, if this is true, then this method is running for a deleted object, > meaning the this pointer is (maybe) referring to somewhere bogus. > > Suggest making this method static, passing in incident_receiver_ as a parameter > and having a static abort_reporting_ field. Done. https://codereview.chromium.org/1643573002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:89: } On 2016/02/08 16:28:02, robertshield wrote: > Is it possible to test the shut down case? Something like: > > { > ModuleLoadAnalyzer module_load_analyzer_; > > module_load_analyzer_.VerifyModuleLoadState(make_scoped_ptr(mock_incident_receiver_)); > } > > base::RunLoop().RunUntilIdle(); > content::RunAllBlockingPoolTasksUntilIdle(); Done.
lgtm https://codereview.chromium.org/1643573002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:87: // soon when the analyzer is destroyed. s/soon when/soon as/
grt@chromium.org changed reviewers: + grt@chromium.org
I have some questions about the threading model. Please ping me when you have a chance.
Description was changed from ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial. BUG= ========== to ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial. BUG=585123 ==========
proberge@chromium.org changed reviewers: + rkaplow@chromium.org
Description was changed from ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial. BUG=585123 ========== to ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial. BUG=585123 ==========
proberge@chromium.org changed reviewers: + nparker@chromium.org
++rkaplow for histograms, ++nparker for csd.proto (please ignore safe_browsing_db related files as they will be submitted in https://codereview.chromium.org/1638223003/)
lgtm https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:10: #include "base/metrics/histogram.h" should be using histogram_macros.h https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:160: UMA_HISTOGRAM_TIMES("SBIRS.SuspiciousModuleReportingTime", you can use SCOPED_UMA_HISTOGRAM_TIMER https://codereview.chromium.org/1643573002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1643573002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:42553: +<histogram name="SBIRS.SuspiciousModuleReportCount"> should still have a unit ... "modules" is my guess for best
On 2016/02/09 15:21:23, grt wrote: > I have some questions about the threading model. Please ping me when you have a > chance. Addressed your comments. PTAL
Following discussion with @grt, made the analyzer classless and removed the global static variable. PTAL. https://codereview.chromium.org/1643573002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:87: // soon when the analyzer is destroyed. On 2016/02/09 14:57:25, robertshield wrote: > s/soon when/soon as/ Done. https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:10: #include "base/metrics/histogram.h" On 2016/02/09 20:45:52, rkaplow wrote: > should be using histogram_macros.h Done. https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:160: UMA_HISTOGRAM_TIMES("SBIRS.SuspiciousModuleReportingTime", On 2016/02/09 20:45:52, rkaplow wrote: > you can use SCOPED_UMA_HISTOGRAM_TIMER Done. https://codereview.chromium.org/1643573002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1643573002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:42553: +<histogram name="SBIRS.SuspiciousModuleReportCount"> On 2016/02/09 20:45:52, rkaplow wrote: > should still have a unit ... "modules" is my guess for best Done.
https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:222: LogIncidentDataType(DISCARDED, *incident); is it possible to retain this logging somehow? https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:412: extended_reporting_only_delayed_analysis_callbacks_.Start(); nit: i think most code in this directory uses braces for multi-line conditionals like this (e.g., line 505 :-) ). https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:502: if (profile && |profile| cannot be null here https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc (right): https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:8: #include "chrome/browser/browser_process.h" unused https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:10: #include "chrome/browser/safe_browsing/safe_browsing_service.h" unused https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:15: void RegisterModuleLoadAnalysis( this re-declares the function. you really want to define it with: void RegisterModuleLoadAnalysis( const scoped_refptr<SafeBrowsingDatabaseManager>&) { } https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:20: void GetLoadedSuspiciousModulesOnIOThread( get rid of these https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h (right): https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:8: #include <vector> unused https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:13: #include "base/strings/string16.h" move to module_load_analyzer_win.cc https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:14: #include "chrome/browser/install_verification/win/module_info.h" move to module_load_analyzer_win.cc https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:15: #include "components/safe_browsing_db/database_manager.h" forward decl SafeBrowsingDatabaseManager rather than including this https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:22: extern const base::Feature kIncidentReportingModuleLoadAnalysis; does this need to be in the public header? it seems to only be used in module_load_analyzer_win.cc. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:38: void GetLoadedSuspiciousModulesOnIOThread( remove these two from the public header -- they're implementation details. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:38: if (base::FeatureList::IsEnabled(kIncidentReportingModuleLoadAnalysis)) { i think the impl for this will be the same for all platforms, so move this to the non-_win.cc file with its contents in an #if defined(OS_WIN) block a la RegisterBinaryIntegrityAnalysis https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:50: void GetLoadedSuspiciousModulesOnIOThread( move into unnamed namespace on line 31 https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:50: void GetLoadedSuspiciousModulesOnIOThread( rename to something like CheckModuleWhitelistOnIOThread? https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:53: scoped_ptr<std::set<ModuleInfo>> module_info_set) { #include <set> https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:55: scoped_ptr<std::set<base::string16>> suspicious_names( can you make this a set of FilePaths since that's what is used in ReportIncidentsForSuspiciousModules? https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:58: std::set<ModuleInfo>::const_iterator module_iter(module_info_set->begin()); for (const ModuleInfo& module_info : *module_info_set) { https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:60: base::string16 module_file_name(base::ToLowerASCII( there's no guarantee that filenames are ascii. consider base::i18n::FoldCase https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:76: base::Passed(std::move(suspicious_names)), #include <utility> https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:82: void ReportIncidentsForSuspiciousModules( move into unnamed namespace https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:103: scoped_refptr<BinaryFeatureExtractor> binary_feature_extractor( can you create one instance of this outside of the loop and reuse it for each module? https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:113: std::wstring file_version = version_info->file_version(); base::string16 https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:130: // Send the report. nit: this sends the incident to the reporting service. whether or not a report is assembled and sent is up to the service itself. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:131: incident_receiver->AddIncidentForProcess(make_scoped_ptr( while the construction of extended_reporting_only_delayed_analysis_callbacks_ ensures that this analyzer doesn't run until sometime after a profile participating in extended safe browsing is added to the profile manager, it doesn't guarantee that the incident will only be reported if such a profile is still loaded at the time of report generation. ProcessIncidentsIfCollectionComplete() will ordinarily associate the incident with a profile participating in extended safe browsing if there is one. do you want to ensure that these incidents are not reported if the ESB profile was unloaded? in this case, consider doing something like extending the Incident class so that instances can be queried for their required level of consent so that the various spots where profile association is done can drop the incident if needed.
https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:412: extended_reporting_only_delayed_analysis_callbacks_.Start(); On 2016/02/11 15:54:59, grt wrote: > nit: i think most code in this directory uses braces for multi-line conditionals > like this (e.g., line 505 :-) ). Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:502: if (profile && On 2016/02/11 15:54:59, grt wrote: > |profile| cannot be null here Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc (right): https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:8: #include "chrome/browser/browser_process.h" On 2016/02/11 15:54:59, grt wrote: > unused Re-used when moving the logic of RegisterModuleLoadAnalysis here once more https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:10: #include "chrome/browser/safe_browsing/safe_browsing_service.h" On 2016/02/11 15:55:00, grt wrote: > unused Re-used when moving the logic of RegisterModuleLoadAnalysis here once more https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:15: void RegisterModuleLoadAnalysis( On 2016/02/11 15:54:59, grt wrote: > this re-declares the function. you really want to define it with: > void RegisterModuleLoadAnalysis( > const scoped_refptr<SafeBrowsingDatabaseManager>&) { > } Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:20: void GetLoadedSuspiciousModulesOnIOThread( On 2016/02/11 15:54:59, grt wrote: > get rid of these Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h (right): https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:8: #include <vector> On 2016/02/11 15:55:00, grt wrote: > unused Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:13: #include "base/strings/string16.h" On 2016/02/11 15:55:00, grt wrote: > move to module_load_analyzer_win.cc Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:14: #include "chrome/browser/install_verification/win/module_info.h" On 2016/02/11 15:55:00, grt wrote: > move to module_load_analyzer_win.cc Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:15: #include "components/safe_browsing_db/database_manager.h" On 2016/02/11 15:55:00, grt wrote: > forward decl SafeBrowsingDatabaseManager rather than including this Done; moved to module_load_analyzer.cc https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:22: extern const base::Feature kIncidentReportingModuleLoadAnalysis; On 2016/02/11 15:55:00, grt wrote: > does this need to be in the public header? it seems to only be used in > module_load_analyzer_win.cc. Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:38: void GetLoadedSuspiciousModulesOnIOThread( On 2016/02/11 15:55:00, grt wrote: > remove these two from the public header -- they're implementation details. Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:38: if (base::FeatureList::IsEnabled(kIncidentReportingModuleLoadAnalysis)) { On 2016/02/11 15:55:00, grt wrote: > i think the impl for this will be the same for all platforms, so move this to > the non-_win.cc file with its contents in an #if defined(OS_WIN) block a la > RegisterBinaryIntegrityAnalysis Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:50: void GetLoadedSuspiciousModulesOnIOThread( On 2016/02/11 15:55:00, grt wrote: > rename to something like CheckModuleWhitelistOnIOThread? Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:50: void GetLoadedSuspiciousModulesOnIOThread( On 2016/02/11 15:55:00, grt wrote: > move into unnamed namespace on line 31 Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:53: scoped_ptr<std::set<ModuleInfo>> module_info_set) { On 2016/02/11 15:55:00, grt wrote: > #include <set> Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:55: scoped_ptr<std::set<base::string16>> suspicious_names( On 2016/02/11 15:55:00, grt wrote: > can you make this a set of FilePaths since that's what is used in > ReportIncidentsForSuspiciousModules? Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:58: std::set<ModuleInfo>::const_iterator module_iter(module_info_set->begin()); On 2016/02/11 15:55:00, grt wrote: > for (const ModuleInfo& module_info : *module_info_set) { Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:60: base::string16 module_file_name(base::ToLowerASCII( On 2016/02/11 15:55:00, grt wrote: > there's no guarantee that filenames are ascii. consider base::i18n::FoldCase Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:76: base::Passed(std::move(suspicious_names)), On 2016/02/11 15:55:00, grt wrote: > #include <utility> Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:82: void ReportIncidentsForSuspiciousModules( On 2016/02/11 15:55:00, grt wrote: > move into unnamed namespace Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:103: scoped_refptr<BinaryFeatureExtractor> binary_feature_extractor( On 2016/02/11 15:55:00, grt wrote: > can you create one instance of this outside of the loop and reuse it for each > module? Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:113: std::wstring file_version = version_info->file_version(); On 2016/02/11 15:55:00, grt wrote: > base::string16 Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:130: // Send the report. On 2016/02/11 15:55:00, grt wrote: > nit: this sends the incident to the reporting service. whether or not a report > is assembled and sent is up to the service itself. Done. https://codereview.chromium.org/1643573002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:131: incident_receiver->AddIncidentForProcess(make_scoped_ptr( On 2016/02/11 15:55:00, grt wrote: > while the construction of extended_reporting_only_delayed_analysis_callbacks_ > ensures that this analyzer doesn't run until sometime after a profile > participating in extended safe browsing is added to the profile manager, it > doesn't guarantee that the incident will only be reported if such a profile is > still loaded at the time of report generation. > ProcessIncidentsIfCollectionComplete() will ordinarily associate the incident > with a profile participating in extended safe browsing if there is one. do you > want to ensure that these incidents are not reported if the ESB profile was > unloaded? in this case, consider doing something like extending the Incident > class so that instances can be queried for their required level of consent so > that the various spots where profile association is done can drop the incident > if needed. Done.
https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:222: LogIncidentDataType(DISCARDED, *incident); On 2016/02/11 15:54:59, grt wrote: > is it possible to retain this logging somehow? I think that this log statement is currently never being hit, so removing it has no effect on existing metrics. Making it work is possible (saving a copy of the incident before passing it) but counting the number of incidents discarded due to Chrome shutting down isn't worth it.
https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/1643573002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:222: LogIncidentDataType(DISCARDED, *incident); On 2016/02/12 13:37:11, proberge wrote: > On 2016/02/11 15:54:59, grt wrote: > > is it possible to retain this logging somehow? > > I think that this log statement is currently never being hit, so removing it has > no effect on existing metrics. > > Making it work is possible (saving a copy of the incident before passing it) but > counting the number of incidents discarded due to Chrome shutting down isn't > worth it. I'm not so certain of that. It was my hope that an "incident funnel" could be made out of this histogram so that it would be possible to see how incidents flow through the service. Any gap in this leads to question marks as to why x% of incidents are unaccounted for. It's interesting that this was _never_ hit before. I wonder why? At the moment, histograms logged this late in the game are dropped, so you could make the argument that the data would never be seen. bcwhite is working on closing this particular metrics gap such that a subsequent chrome launch would pick up the values and report them, so eventually the value(s) would be reported. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/incident.h (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident.h:36: // profile. Please keep this list in ascending order of privacy consent. nit: the ordering requirement isn't a gentle request, is it? "The list must be ordered by increasing privacy consent." or something like that. if it is not a requirement, then there's no need to make any mention of the ordering. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident.h:37: enum class MinimumProfileConsent : int32_t { omit " : int32_t" unless it's really required. in the case of the incident type, it is. for this, i'm not so sure. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident.h:62: virtual MinimumProfileConsent GetMinimumProfileConsent() const; // Returns the minimum level of consent required for reporting of the incident. or something like that https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:122: switch (incident.GetMinimumProfileConsent()) { i prefer the safer: if (!profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) return false; switch (incident.GetMinimumProfileConsent()) { case MinimumProfileConsent::SAFE_BROWSING_ENABLED: return true; case MinimumProfileConsent::SAFE_BROWSING_EXTENDED_REPORTING_ENABLED: return profile->GetPrefs()->GetBoolean( prefs::kSafeBrowsingExtendedReportingEnabled); } NOTREACHED(); return false; https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:128: default: note: it's best to omit the default case when the values are explicitly defined so that a compiler error will fire if someone adds a new value to the enum in the future. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:31: ->RegisterExtendedReportingOnlyDelayedAnalysisCallback( did git cl format put the -> on the second line? looks odd to me... https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:8: #include "base/feature_list.h" move to .cc file https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:27: } // namespace safe_browsing nit: blank line before this https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:16: #include "base/strings/string_number_conversions.h" unused? https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:17: #include "base/strings/string_util.h" is this used? https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:19: #include "chrome/browser/browser_process.h" unused? https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:25: #include "chrome/browser/safe_browsing/safe_browsing_service.h" unused? https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:40: scoped_ptr<std::set<base::FilePath>> module_names, module_names -> module_paths https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:47: for (const auto& module_name : *module_names) { module_name -> module_path and delete lines 54 and 55 https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:48: // TODO(proberge): Skip over modules that have already been reported. how hard do you want to work at this? do you desire to do the skipping here for the sake of efficiency rather than let the service's pruning take care of it for you? given the way that SuspiciousModuleIncident::ComputeDigest() is implemented, pruning is done based on the full payload of the incident, so all of this data needs to be collected anyway. if you like, you could change it so that the digest is a constant so that modules of a given path are reported at most once. in this case, you could bounce the list of paths to the UI thread and in some way call HasBeenReported before extracting their features. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:59: suspicious_module->set_path(base::WideToUTF8(sanitized_path.value())); base::WideToUTF8(sanitized_path.value()) -> sanitized_path.AsUTF8Unsafe() https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:71: suspicious_module->set_version(base::WideToUTF8(file_version)); base::UTF16ToUTF8 https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:97: scoped_ptr<std::set<base::FilePath>> suspicious_names( nit: this contains the full paths, not just the names. suspicious_paths seems more clear. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:102: base::FilePath(module_info.name).BaseName().value())); nit: stuff base::FilePath(module_info.name) in a local var so that it isn't created twice https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:102: base::FilePath(module_info.name).BaseName().value())); .value() -> .AsUTF16Unsafe() https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:114: content::BrowserThread::GetBlockingPool() #include "content/public/browser/browser_thread.h" https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:7: #include <vector> unused https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:26: using ::testing::IsNull; unused https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:33: namespace { nit: newline after this https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:45: virtual ~MockSafeBrowsingDatabaseManager() {} is this needed? if so, it should be "~MockSafeBrowsingDatabaseManager() override {}" https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:57: EXPECT_CALL(*mock_safe_browsing_database_manager_, if this is meant to be "return true by default", then you can say this explicitly with: ON_CALL(...).WillByDefault(Return(true)); this is the better choice if you really don't need test expectations for these calls. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:60: EXPECT_CALL(*mock_safe_browsing_database_manager_, this seems like a valid test expectation for TestNonWhitelistedDLLs. how about moving it down there? if it should only be called once, you could make it a WillOnce(Return(false)). in this case, you may be able to make it a StrictMock as well. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:65: void ExpectIncident(const std::string& module_to_load) { #include <string> https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/suspicious_module_incident_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/suspicious_module_incident_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016
https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/incident.h (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident.h:36: // profile. Please keep this list in ascending order of privacy consent. On 2016/02/15 16:46:50, grt wrote: > nit: the ordering requirement isn't a gentle request, is it? "The list must be > ordered by increasing privacy consent." or something like that. if it is not a > requirement, then there's no need to make any mention of the ordering. Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident.h:37: enum class MinimumProfileConsent : int32_t { On 2016/02/15 16:46:49, grt wrote: > omit " : int32_t" unless it's really required. in the case of the incident type, > it is. for this, i'm not so sure. Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident.h:62: virtual MinimumProfileConsent GetMinimumProfileConsent() const; On 2016/02/15 16:46:49, grt wrote: > // Returns the minimum level of consent required for reporting of the > incident. > or something like that Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:122: switch (incident.GetMinimumProfileConsent()) { On 2016/02/15 16:46:50, grt wrote: > i prefer the safer: > if (!profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) > return false; > switch (incident.GetMinimumProfileConsent()) { > case MinimumProfileConsent::SAFE_BROWSING_ENABLED: > return true; > case MinimumProfileConsent::SAFE_BROWSING_EXTENDED_REPORTING_ENABLED: > return profile->GetPrefs()->GetBoolean( > prefs::kSafeBrowsingExtendedReportingEnabled); > } > NOTREACHED(); > return false; Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:128: default: On 2016/02/15 16:46:50, grt wrote: > note: it's best to omit the default case when the values are explicitly defined > so that a compiler error will fire if someone adds a new value to the enum in > the future. Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.cc:31: ->RegisterExtendedReportingOnlyDelayedAnalysisCallback( On 2016/02/15 16:46:50, grt wrote: > did git cl format put the -> on the second line? looks odd to me... Yep https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:8: #include "base/feature_list.h" On 2016/02/15 16:46:50, grt wrote: > move to .cc file Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h:27: } // namespace safe_browsing On 2016/02/15 16:46:50, grt wrote: > nit: blank line before this Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:16: #include "base/strings/string_number_conversions.h" On 2016/02/15 16:46:50, grt wrote: > unused? Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:17: #include "base/strings/string_util.h" On 2016/02/15 16:46:50, grt wrote: > is this used? Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:19: #include "chrome/browser/browser_process.h" On 2016/02/15 16:46:50, grt wrote: > unused? Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:25: #include "chrome/browser/safe_browsing/safe_browsing_service.h" On 2016/02/15 16:46:50, grt wrote: > unused? Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:40: scoped_ptr<std::set<base::FilePath>> module_names, On 2016/02/15 16:46:50, grt wrote: > module_names -> module_paths Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:47: for (const auto& module_name : *module_names) { On 2016/02/15 16:46:50, grt wrote: > module_name -> module_path and delete lines 54 and 55 Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:48: // TODO(proberge): Skip over modules that have already been reported. On 2016/02/15 16:46:50, grt wrote: > how hard do you want to work at this? do you desire to do the skipping here for > the sake of efficiency rather than let the service's pruning take care of it for > you? given the way that SuspiciousModuleIncident::ComputeDigest() is > implemented, pruning is done based on the full payload of the incident, so all > of this data needs to be collected anyway. if you like, you could change it so > that the digest is a constant so that modules of a given path are reported at > most once. in this case, you could bounce the list of paths to the UI thread and > in some way call HasBeenReported before extracting their features. My main concern was that a Profile/ProfileContext is needed to read the StateStore from, which we don't know about at this point. Would prefer to do in a future CL, if at all, to minimize complexity in this one. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:59: suspicious_module->set_path(base::WideToUTF8(sanitized_path.value())); On 2016/02/15 16:46:50, grt wrote: > base::WideToUTF8(sanitized_path.value()) -> sanitized_path.AsUTF8Unsafe() Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:71: suspicious_module->set_version(base::WideToUTF8(file_version)); On 2016/02/15 16:46:50, grt wrote: > base::UTF16ToUTF8 Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:97: scoped_ptr<std::set<base::FilePath>> suspicious_names( On 2016/02/15 16:46:50, grt wrote: > nit: this contains the full paths, not just the names. suspicious_paths seems > more clear. Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:102: base::FilePath(module_info.name).BaseName().value())); On 2016/02/15 16:46:50, grt wrote: > nit: stuff base::FilePath(module_info.name) in a local var so that it isn't > created twice Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:102: base::FilePath(module_info.name).BaseName().value())); On 2016/02/15 16:46:50, grt wrote: > nit: stuff base::FilePath(module_info.name) in a local var so that it isn't > created twice Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:114: content::BrowserThread::GetBlockingPool() On 2016/02/15 16:46:50, grt wrote: > #include "content/public/browser/browser_thread.h" Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/02/15 16:46:51, grt wrote: > 2016 Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:7: #include <vector> On 2016/02/15 16:46:50, grt wrote: > unused Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:26: using ::testing::IsNull; On 2016/02/15 16:46:50, grt wrote: > unused Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:33: namespace { On 2016/02/15 16:46:50, grt wrote: > nit: newline after this Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:45: virtual ~MockSafeBrowsingDatabaseManager() {} On 2016/02/15 16:46:50, grt wrote: > is this needed? if so, it should be "~MockSafeBrowsingDatabaseManager() override > {}" Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:57: EXPECT_CALL(*mock_safe_browsing_database_manager_, On 2016/02/15 16:46:50, grt wrote: > if this is meant to be "return true by default", then you can say this > explicitly with: > ON_CALL(...).WillByDefault(Return(true)); > this is the better choice if you really don't need test expectations for these > calls. ON_CALL(...).WillByDefault and EXPECT_CALL(...).WillOnce don't work well together; the expect_call gets saturated by the wrong argument. Using ON_CALL for both works, but it spams the logs so I'm keeping the two EXPECT_CALL. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:60: EXPECT_CALL(*mock_safe_browsing_database_manager_, On 2016/02/15 16:46:50, grt wrote: > this seems like a valid test expectation for TestNonWhitelistedDLLs. how about > moving it down there? if it should only be called once, you could make it a > WillOnce(Return(false)). in this case, you may be able to make it a StrictMock > as well. Can't mock SafeBrowsingDatabaseManager directly, as it's an abstract class. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:65: void ExpectIncident(const std::string& module_to_load) { On 2016/02/15 16:46:50, grt wrote: > #include <string> Done. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/suspicious_module_incident_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/suspicious_module_incident_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/15 16:46:51, grt wrote: > 2016 Done.
Can you make this dependent on https://codereview.chromium.org/1638223003/? I can't tell what is added here. CSD proto LGTM.
I didn't realize that this contained code from another CL. In the future, please pipeline your changes with tracking branches so that "git cl upload" automatically only uploads the new stuff in the branch. It will automatically reference the CR for the parent branch. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc:48: // TODO(proberge): Skip over modules that have already been reported. On 2016/02/16 16:56:23, proberge wrote: > On 2016/02/15 16:46:50, grt wrote: > > how hard do you want to work at this? do you desire to do the skipping here > for > > the sake of efficiency rather than let the service's pruning take care of it > for > > you? given the way that SuspiciousModuleIncident::ComputeDigest() is > > implemented, pruning is done based on the full payload of the incident, so all > > of this data needs to be collected anyway. if you like, you could change it so > > that the digest is a constant so that modules of a given path are reported at > > most once. in this case, you could bounce the list of paths to the UI thread > and > > in some way call HasBeenReported before extracting their features. > > My main concern was that a Profile/ProfileContext is needed to read the > StateStore from, which we don't know about at this point. > Would prefer to do in a future CL, if at all, to minimize complexity in this > one. Ack https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:57: EXPECT_CALL(*mock_safe_browsing_database_manager_, On 2016/02/16 16:56:23, proberge wrote: > On 2016/02/15 16:46:50, grt wrote: > > if this is meant to be "return true by default", then you can say this > > explicitly with: > > ON_CALL(...).WillByDefault(Return(true)); > > this is the better choice if you really don't need test expectations for these > > calls. > > ON_CALL(...).WillByDefault and EXPECT_CALL(...).WillOnce don't work well > together; the expect_call gets saturated by the wrong argument. Why? I have used the two in other tests with success. > Using ON_CALL for both works, but it spams the logs so I'm keeping the two > EXPECT_CALL. Spams them with what? https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:60: EXPECT_CALL(*mock_safe_browsing_database_manager_, On 2016/02/16 16:56:23, proberge wrote: > On 2016/02/15 16:46:50, grt wrote: > > this seems like a valid test expectation for TestNonWhitelistedDLLs. how about > > moving it down there? if it should only be called once, you could make it a > > WillOnce(Return(false)). in this case, you may be able to make it a StrictMock > > as well. > > Can't mock SafeBrowsingDatabaseManager directly, as it's an abstract class. I don't understand what you're saying here. What's wrong with the mock you have? You can change it to a StrictMock with "new StrictMock<MockSafeBrowsingDatabaseManager>()" on line 54, no?
https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:57: EXPECT_CALL(*mock_safe_browsing_database_manager_, On 2016/02/17 18:04:59, grt wrote: > On 2016/02/16 16:56:23, proberge wrote: > > On 2016/02/15 16:46:50, grt wrote: > > > if this is meant to be "return true by default", then you can say this > > > explicitly with: > > > ON_CALL(...).WillByDefault(Return(true)); > > > this is the better choice if you really don't need test expectations for > these > > > calls. > > > > ON_CALL(...).WillByDefault and EXPECT_CALL(...).WillOnce don't work well > > together; the expect_call gets saturated by the wrong argument. > > Why? I have used the two in other tests with success. I'm probably doing something wrong, but I'm seeing the following: unknown file: error: Unexpected mock function call - taking default action specified at: [...]chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(57): Function call: MatchModuleWhitelistString(@0018F140 "user32.dll") Returns: true Google Mock tried the following 1 expectation, but it didn't match: [...]chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(115): EXPECT_CALL (*mock_safe_browsing_database_manager_, MatchModuleWhitelistString(kNonWhitelistedModuleName))... Expected arg #0: is equal to "blacklist_test_dll_1.dll" Actual: "user32.dll" Expected: to be called once Actual: called once - saturated and active > > Using ON_CALL for both works, but it spams the logs so I'm keeping the two > > EXPECT_CALL. > > Spams them with what? The following message (once for every dll on the system): GMOCK WARNING: Uninteresting mock function call - taking default action specified at: c:\src\chrome\src\chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(57): Function call: MatchModuleWhitelistString(@0018F280 "user32.dll") Returns: true NOTE: You can safely ignore the above warning unless this call should not happen. Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean to enforce the call. See http://code.google.com/p/googlemock/wiki/CookBook#Knowing_W hen_to_Expect for details. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:60: EXPECT_CALL(*mock_safe_browsing_database_manager_, On 2016/02/17 18:04:59, grt wrote: > On 2016/02/16 16:56:23, proberge wrote: > > On 2016/02/15 16:46:50, grt wrote: > > > this seems like a valid test expectation for TestNonWhitelistedDLLs. how > about > > > moving it down there? if it should only be called once, you could make it a > > > WillOnce(Return(false)). in this case, you may be able to make it a > StrictMock > > > as well. > > > > Can't mock SafeBrowsingDatabaseManager directly, as it's an abstract class. > > I don't understand what you're saying here. What's wrong with the mock you have? > You can change it to a StrictMock with "new > StrictMock<MockSafeBrowsingDatabaseManager>()" on line 54, no? Does it make sense to StrictMock a Mock class? That seemed like a weird thing to do, so I assumed you were suggesting to use StrictMock<SafeBrowsingDatabaseManager> instead of MockSafeBrowsingDatabaseManager.
lgtm once the test is fixed https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:57: EXPECT_CALL(*mock_safe_browsing_database_manager_, On 2016/02/17 21:16:32, proberge wrote: > On 2016/02/17 18:04:59, grt wrote: > > On 2016/02/16 16:56:23, proberge wrote: > > > On 2016/02/15 16:46:50, grt wrote: > > > > if this is meant to be "return true by default", then you can say this > > > > explicitly with: > > > > ON_CALL(...).WillByDefault(Return(true)); > > > > this is the better choice if you really don't need test expectations for > > these > > > > calls. > > > > > > ON_CALL(...).WillByDefault and EXPECT_CALL(...).WillOnce don't work well > > > together; the expect_call gets saturated by the wrong argument. > > > > Why? I have used the two in other tests with success. > > I'm probably doing something wrong, but I'm seeing the following: > > unknown file: error: > Unexpected mock function call - taking default action specified at: > [...]chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(57): > Function call: MatchModuleWhitelistString(@0018F140 "user32.dll") > Returns: true > Google Mock tried the following 1 expectation, but it didn't match: > [...]chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(115): > EXPECT_CALL > (*mock_safe_browsing_database_manager_, > MatchModuleWhitelistString(kNonWhitelistedModuleName))... > Expected arg #0: is equal to "blacklist_test_dll_1.dll" > Actual: "user32.dll" > Expected: to be called once > Actual: called once - saturated and active Riiiiight. I see (2nd paragraph of https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#...). Apologies for the mis-direction. > > > Using ON_CALL for both works, but it spams the logs so I'm keeping the two > > > EXPECT_CALL. > > > > Spams them with what? > > The following message (once for every dll on the system): > > GMOCK WARNING: > Uninteresting mock function call - taking default action specified at: > c:\src\chrome\src\chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(57): > Function call: MatchModuleWhitelistString(@0018F280 "user32.dll") > Returns: true > NOTE: You can safely ignore the above warning unless this call should not > happen. Do not suppress it by blindly adding > an EXPECT_CALL() if you don't mean to enforce the call. See > http://code.google.com/p/googlemock/wiki/CookBook#Knowing_W > hen_to_Expect for details. Oh, yeah. NiceMock is good for suppressing those, but in this case I think you need the expectations as you had them. https://codereview.chromium.org/1643573002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:60: EXPECT_CALL(*mock_safe_browsing_database_manager_, On 2016/02/17 21:16:32, proberge wrote: > On 2016/02/17 18:04:59, grt wrote: > > On 2016/02/16 16:56:23, proberge wrote: > > > On 2016/02/15 16:46:50, grt wrote: > > > > this seems like a valid test expectation for TestNonWhitelistedDLLs. how > > about > > > > moving it down there? if it should only be called once, you could make it > a > > > > WillOnce(Return(false)). in this case, you may be able to make it a > > StrictMock > > > > as well. > > > > > > Can't mock SafeBrowsingDatabaseManager directly, as it's an abstract class. > > > > I don't understand what you're saying here. What's wrong with the mock you > have? > > You can change it to a StrictMock with "new > > StrictMock<MockSafeBrowsingDatabaseManager>()" on line 54, no? > > Does it make sense to StrictMock a Mock class? Yup, they are wrappers around any mock: https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... https://codereview.chromium.org/1643573002/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/suspicious_module_incident_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/suspicious_module_incident_unittest.cc:43: ASSERT_EQ(MakeIncident("37")->ComputeDigest(), this test is broken: - this should be ASSERT_NE since the intent is that incidents with different payloads result in different digests - MakeIncident() ignores its param, unconditionally returning identical payloads, which is not the intent
On 2016/02/16 21:23:35, nparker wrote: > Can you make this dependent on https://codereview.chromium.org/1638223003/ I > can't tell what is added here. CSD proto LGTM. I've synced past 1638223003, so it looks like there's only the proto and safe_browsing_service.h/.cc for you to review.
https://codereview.chromium.org/1643573002/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/incident_reporting/suspicious_module_incident_unittest.cc (right): https://codereview.chromium.org/1643573002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/incident_reporting/suspicious_module_incident_unittest.cc:43: ASSERT_EQ(MakeIncident("37")->ComputeDigest(), On 2016/02/18 16:06:55, grt wrote: > this test is broken: > - this should be ASSERT_NE since the intent is that incidents with different > payloads result in different digests > - MakeIncident() ignores its param, unconditionally returning identical > payloads, which is not the intent Good catch. It looks like blacklist_load_incident_unittest has a similar issue. Fixed it for both.
lgtm https://codereview.chromium.org/1643573002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/1643573002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:151: void RegisterExtendedReportingOnlyDelayedAnalysisCallback( I'm wondering if it'd be simpler to add a method to expose the incident_service_, and remove these plus the AddDownloadManager. what do you think?
https://codereview.chromium.org/1643573002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/1643573002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:151: void RegisterExtendedReportingOnlyDelayedAnalysisCallback( On 2016/02/22 19:58:01, nparker wrote: > I'm wondering if it'd be simpler to add a method to expose the > incident_service_, and remove these plus the AddDownloadManager. what do you > think? I believe we expose AddDownloadManager instead of GetIncidentService to avoid exposing a large interface to other parts of Chrome. To me it seems reasonable to have all calls from outside of browser/safebrowsing have to go through this service. (AddDownloadManager is called from chrome/browser/download/chrome_download_manager_delegate.cc) For the duplicated mehtod, we're thinking about enabling IncidentReporting only for users that have extended reporting enabled, in which case we could clean this up a little.
lgtm
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org, rkaplow@chromium.org, grt@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/1643573002/#ps240001 (title: "rebase-update'd")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1643573002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643573002/240001
Message was sent while issue was closed.
Description was changed from ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial. BUG=585123 ========== to ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial. BUG=585123 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial. BUG=585123 ========== to ========== Add a ModuleLoadAnalyzer which checks modules against a whitelist and reports suspicious modules. The ModuleLoadAnalyzer is only enabled if a profile with Safebrowsing Extended Reporting is found. In addition, a feature flag must be enabled through the command line or a field trial. BUG=585123 Committed: https://crrev.com/908da69d25746389d4b695e989b3df5e31e88341 Cr-Commit-Position: refs/heads/master@{#377604} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/908da69d25746389d4b695e989b3df5e31e88341 Cr-Commit-Position: refs/heads/master@{#377604}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
This broke clang/win builds:
..\..\chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(38,7)
: error: [chromium-style] Classes that are ref-counted should have explicit
destructors that are declared protected or private.
class MockSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager {
^
..\..\chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(38,41)
: note: [chromium-style] 'MockSafeBrowsingDatabaseManager' inherits from
'safe_browsing::TestSafeBrowsingDatabaseManager' here
class MockSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager {
^
..\..\components/safe_browsing_db/test_database_manager.h(26,7) : note:
[chromium-style] 'TestSafeBrowsingDatabaseManager' inherits from
'safe_browsing::SafeBrowsingDatabaseManager' here
: public SafeBrowsingDatabaseManager {
^
..\..\components/safe_browsing_db/database_manager.h(34,7) : note:
[chromium-style] 'SafeBrowsingDatabaseManager' inherits from
'base::RefCountedThreadSafe<SafeBrowsingDatabaseManager>' here
: public base::RefCountedThreadSafe<SafeBrowsingDatabaseManager> {
^
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/6559/ste...
Can you take a look, please?
Message was sent while issue was closed.
On 2016/02/25 19:35:50, Nico wrote:
> This broke clang/win builds:
>
>
..\..\chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(38,7)
> : error: [chromium-style] Classes that are ref-counted should have explicit
> destructors that are declared protected or private.
> class MockSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager
{
> ^
>
..\..\chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(38,41)
> : note: [chromium-style] 'MockSafeBrowsingDatabaseManager' inherits from
> 'safe_browsing::TestSafeBrowsingDatabaseManager' here
> class MockSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager
{
> ^
> ..\..\components/safe_browsing_db/test_database_manager.h(26,7) : note:
> [chromium-style] 'TestSafeBrowsingDatabaseManager' inherits from
> 'safe_browsing::SafeBrowsingDatabaseManager' here
> : public SafeBrowsingDatabaseManager {
> ^
> ..\..\components/safe_browsing_db/database_manager.h(34,7) : note:
> [chromium-style] 'SafeBrowsingDatabaseManager' inherits from
> 'base::RefCountedThreadSafe<SafeBrowsingDatabaseManager>' here
> : public base::RefCountedThreadSafe<SafeBrowsingDatabaseManager> {
> ^
>
>
>
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/6559/ste...
>
>
> Can you take a look, please?
Thanks, I'll fix it forward asap.
|
