Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2037883004:
[Win] Add reporting of total number of modules loaded in browser process. (Closed)
|
Created:
4 years, 6 months ago by chrisha Modified:
4 years, 3 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, jschuh, wfh5, grt (UTC plus 2) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Win] Add reporting of total number of modules loaded in browser process.
BUG=617176, 619923
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/8645b5898bbcb15d7886daf5399c923af20b08dc
Cr-Commit-Position: refs/heads/master@{#415375}
Patch Set 1 #Patch Set 2 : Ready for review. #
Total comments: 4
Patch Set 3 : Major cleanup. #Patch Set 4 : Remove debug code. #Patch Set 5 : Move vfunc impls to cc file. #
Total comments: 32
Patch Set 6 : Remove unref'd vars from unittest. #
Total comments: 2
Patch Set 7 : Address grt's comments. #Patch Set 8 : Fix missing include. #
Total comments: 6
Patch Set 9 : Refactor locking, add support for catalogs. #
Total comments: 46
Patch Set 10 : Address comments on patch 9. #
Total comments: 12
Patch Set 11 : Address grt's comments on patchset 10. #Patch Set 12 : Small fixes. #
Total comments: 40
Patch Set 13 : Address comments from cpu, thestig and pkasting. #Patch Set 14 : Fix typo. #Patch Set 15 : Fix unittest. #Patch Set 16 : Rebased. #Patch Set 17 : Fix clang error. #Patch Set 18 : Fix dispatch. #Patch Set 19 : Fix constant in unittest. #Patch Set 20 : Rebased. #Messages
Total messages: 87 (41 generated)
chrisha@chromium.org changed reviewers: + rkaplow@chromium.org
PTAL?
https://codereview.chromium.org/2037883004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2037883004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:159: CHECK(list->GetDictionary(i, &module)); DCHECK here and below https://codereview.chromium.org/2037883004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2037883004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57020: + The total number of modules loaded (or potentially loaded) into the browser can you mention on these two that they are windows only
https://codereview.chromium.org/2037883004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2037883004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:159: CHECK(list->GetDictionary(i, &module)); On 2016/06/03 18:17:31, rkaplow wrote: > DCHECK here and below that is, should DCHECK on the result (since it still needs to execute)
grt@chromium.org changed reviewers: + grt@chromium.org
https://codereview.chromium.org/2037883004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2037883004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:151: base::TimeDelta::FromSeconds(1)); please do something better than polling for the result. i see that EnumerateModulesModel uses NotificationService, which is on the chopping block (https://crbug.com/268984). perhaps now is the time to replace NOTIFICATION_MODULE_LIST_ENUMERATED with a better observer/delegate pattern.
Okay, completely reworked this by cleaning up the EnumerateModulesModel, and integrating the code directly into it. Does the following: - Makes this run everywhere, not only just on XP. (Doh!) - Remove use of NotificationService, and use observers. - Remove entire unused blacklist mechanism. - Simplify resulting code and unittests. - Added desired new metrics. PTAL?
The CQ bit was checked by chrisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037883004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by chrisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037883004/80001
Description was changed from ========== [Win] Add reporting of total number of modules loaded in browser process. BUG=617176 ========== to ========== [Win] Add reporting of total number of modules loaded in browser process. BUG=617176,619923 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
first pass. i suspect i'll have more comments after these are factored in. thanks for taking a stab at cleaning up old code! https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... File chrome/browser/enumerate_modules_model_unittest_win.cc (right): https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_unittest_win.cc:26: static const ModuleEnumerator::OperatingSystem kOs = cool: clang points out that this is now unused (among other constants in this file). https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... File chrome/browser/enumerate_modules_model_win.cc (right): https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.cc:503: for (const auto module : *enumerated_modules_) { nit: const auto& module https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.cc:532: return base::Singleton<EnumerateModulesModel>::get(); LazyInstance rather than Singleton here since this is the one and only getter for the global instance. if it doesn't need to be deleted at shutdown, then you could use a Leaky LazyInstance. adding on that: if you're sure all callers are on the UI thread, you could simplify further to: #include "base/debug/leak_annotations.h" static EnumerateModulesModel* instance = nullptr; if (!instance) { instance = new EnumerateModulesModel(); ANNOTATE_LEAKING_OBJECT_PTR(instance); } return instance; https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.cc:554: if (scanning_) accessing shared state outside of a lock? hmm. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.cc:677: lock = new base::Lock(); why is the lock a global outside of the instance? https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... File chrome/browser/enumerate_modules_model_win.h (right): https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:134: // through the MODULE_LIST_ENUMERATED notification. update doc comments regarding notifications throughout https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:140: void ScanNow(ModulesVector* list, bool limited_mode); i can't find any callers that use limited_mode=true. remove support for this? https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:201: void ReportThirdPartyMetrics(); please add doc comment https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:215: EnumerateModulesModel* observer_; why is the pointer to the model called "observer_"? https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:251: class Observer { the following suggestions are for consistency with observers in content/public that do not carry behavior https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:253: Observer() {} omit the ctor https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:254: virtual ~Observer() {} move dtor into "protected:" block i think " = default;" is a good choice here as well https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:257: virtual void OnScanCompleted(bool limited_mode); inline the empty methods with "{}" https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:264: DISALLOW_COPY_AND_ASSIGN(Observer); omit this https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:320: // The user will be taken to this site when the conflict bubble or app menu nit: // Returns the site to which the user should be taken when... as per https://google.github.io/styleguide/cppguide.html#Function_Comments https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:343: // When this singleton object is constructed we go and fire off this timer to this comment is a lie https://codereview.chromium.org/2037883004/diff/100001/chrome/browser/enumera... File chrome/browser/enumerate_modules_model_win.h (right): https://codereview.chromium.org/2037883004/diff/100001/chrome/browser/enumera... chrome/browser/enumerate_modules_model_win.h:271: void AddObserver(Observer* observer) { are observers always added/removed on the UI thread? can the list be the un-thread-safe variant?
lgtm new metrics lg
Tried to integrate most of your comments. Waiting to hear back about the remaining "recon" client to see if I can entirely rip out the limited mode. At which point I become the sole "client" of this code and can refactor at will. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... File chrome/browser/enumerate_modules_model_unittest_win.cc (right): https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_unittest_win.cc:26: static const ModuleEnumerator::OperatingSystem kOs = On 2016/06/16 18:27:52, grt (slow) wrote: > cool: clang points out that this is now unused (among other constants in this > file). Acknowledged. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... File chrome/browser/enumerate_modules_model_win.cc (right): https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.cc:503: for (const auto module : *enumerated_modules_) { On 2016/06/16 18:27:52, grt (slow) wrote: > nit: const auto& module Done. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.cc:532: return base::Singleton<EnumerateModulesModel>::get(); On 2016/06/16 18:27:52, grt (slow) wrote: > LazyInstance rather than Singleton here since this is the one and only getter > for the global instance. > > if it doesn't need to be deleted at shutdown, then you could use a Leaky > LazyInstance. > > adding on that: if you're sure all callers are on the UI thread, you could > simplify further to: > > #include "base/debug/leak_annotations.h" > > static EnumerateModulesModel* instance = nullptr; > if (!instance) { > instance = new EnumerateModulesModel(); > ANNOTATE_LEAKING_OBJECT_PTR(instance); > } > return instance; Fine to let this leak. Not sure about callers being on the UI thread due to the nebulous "recon" code. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.cc:554: if (scanning_) On 2016/06/16 18:27:53, grt (slow) wrote: > accessing shared state outside of a lock? hmm. Ouch, didn't see that! https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.cc:677: lock = new base::Lock(); On 2016/06/16 18:27:52, grt (slow) wrote: > why is the lock a global outside of the instance? No idea whatsoever. Made a member. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... File chrome/browser/enumerate_modules_model_win.h (right): https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:134: // through the MODULE_LIST_ENUMERATED notification. On 2016/06/16 18:27:53, grt (slow) wrote: > update doc comments regarding notifications throughout Done. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:140: void ScanNow(ModulesVector* list, bool limited_mode); On 2016/06/16 18:27:53, grt (slow) wrote: > i can't find any callers that use limited_mode=true. remove support for this? chrome/browser/diagnostics/recon_diagnostics.cc still uses this. I'm not sure how important that use case still is, given the general state of bitrot in this code. I'll track down the appropriate folks, but I'd prefer to not break them in the meantime. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:201: void ReportThirdPartyMetrics(); On 2016/06/16 18:27:53, grt (slow) wrote: > please add doc comment Done. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:215: EnumerateModulesModel* observer_; On 2016/06/16 18:27:53, grt (slow) wrote: > why is the pointer to the model called "observer_"? The implementation of the enumeration is done by this helper class (ModuleEnumerator), and it notifies the EnumerateModulesModel when it has completed. The 'observer' is passed in via the constructor. This could stand to be cleaned up, but I was hoping to avoid refactoring the whole world. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:251: class Observer { On 2016/06/16 18:27:53, grt (slow) wrote: > the following suggestions are for consistency with observers in content/public > that do not carry behavior Done. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:253: Observer() {} On 2016/06/16 18:27:53, grt (slow) wrote: > omit the ctor Done. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:254: virtual ~Observer() {} On 2016/06/16 18:27:53, grt (slow) wrote: > move dtor into "protected:" block i think " = default;" is a good choice here as > well Done. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:257: virtual void OnScanCompleted(bool limited_mode); On 2016/06/16 18:27:53, grt (slow) wrote: > inline the empty methods with "{}" Done. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:264: DISALLOW_COPY_AND_ASSIGN(Observer); On 2016/06/16 18:27:53, grt (slow) wrote: > omit this Done. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:320: // The user will be taken to this site when the conflict bubble or app menu On 2016/06/16 18:27:53, grt (slow) wrote: > nit: > // Returns the site to which the user should be taken when... > as per https://google.github.io/styleguide/cppguide.html#Function_Comments Done. https://codereview.chromium.org/2037883004/diff/80001/chrome/browser/enumerat... chrome/browser/enumerate_modules_model_win.h:343: // When this singleton object is constructed we go and fire off this timer to On 2016/06/16 18:27:53, grt (slow) wrote: > this comment is a lie Done. https://codereview.chromium.org/2037883004/diff/100001/chrome/browser/enumera... File chrome/browser/enumerate_modules_model_win.h (right): https://codereview.chromium.org/2037883004/diff/100001/chrome/browser/enumera... chrome/browser/enumerate_modules_model_win.h:271: void AddObserver(Observer* observer) { On 2016/06/16 18:27:53, grt (slow) wrote: > are observers always added/removed on the UI thread? can the list be the > un-thread-safe variant? They are indeed. Moved to the normal variant.
The CQ bit was checked by chrisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037883004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
nits while waiting for next round https://codereview.chromium.org/2037883004/diff/140001/base/lazy_instance.h File base/lazy_instance.h (right): https://codereview.chromium.org/2037883004/diff/140001/base/lazy_instance.h#n... base/lazy_instance.h:43: #include "base/lazy_instance.h" i don't think you meant to include this. :-) https://codereview.chromium.org/2037883004/diff/140001/chrome/browser/enumera... File chrome/browser/enumerate_modules_model_win.cc (right): https://codereview.chromium.org/2037883004/diff/140001/chrome/browser/enumera... chrome/browser/enumerate_modules_model_win.cc:521: static base::LazyInstance<EnumerateModulesModel>::Leaky model; model = LAZY_INSTANCE_INITIALIZER; https://codereview.chromium.org/2037883004/diff/140001/chrome/browser/enumera... chrome/browser/enumerate_modules_model_win.cc:522: return &model.Get(); model.Pointer();
Reworked all the locking, and added much more documentation as to what can be called from where, including liberal use of DCHECK_CURRENTLY_ON. Also removed the limited_mode, whose "diagnostics" use has long since been out of use. Another look? https://codereview.chromium.org/2037883004/diff/140001/base/lazy_instance.h File base/lazy_instance.h (right): https://codereview.chromium.org/2037883004/diff/140001/base/lazy_instance.h#n... base/lazy_instance.h:43: #include "base/lazy_instance.h" On 2016/06/20 20:36:28, grt (UTC plus 2) wrote: > i don't think you meant to include this. :-) Done. https://codereview.chromium.org/2037883004/diff/140001/chrome/browser/enumera... File chrome/browser/enumerate_modules_model_win.cc (right): https://codereview.chromium.org/2037883004/diff/140001/chrome/browser/enumera... chrome/browser/enumerate_modules_model_win.cc:521: static base::LazyInstance<EnumerateModulesModel>::Leaky model; On 2016/06/20 20:36:28, grt (UTC plus 2) wrote: > model = LAZY_INSTANCE_INITIALIZER; Done. https://codereview.chromium.org/2037883004/diff/140001/chrome/browser/enumera... chrome/browser/enumerate_modules_model_win.cc:522: return &model.Get(); On 2016/06/20 20:36:28, grt (UTC plus 2) wrote: > model.Pointer(); Done.
next round! https://codereview.chromium.org/2037883004/diff/160001/base/lazy_instance.h File base/lazy_instance.h (right): https://codereview.chromium.org/2037883004/diff/160001/base/lazy_instance.h#n... base/lazy_instance.h:43: #include "base/lazy_instance.h" ping https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:101: const base::FilePath& filename) { nit: unwrap https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:102: HCERTSTORE store = NULL; nullptr all the things https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:591: signed_by = GetSignedByInCatalog(filename); nit: return this directly rather than going through the local var https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:600: ++signed_modules; nit: indentation. (git cl format wil fix this) https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:601: if (module.digital_signer.find(L"Microsoft") != base::string16::npos) this check can be trivially made to succeed. are you hoping to just get a guess at how many binaries are signed by MS? (see http://arstechnica.com/security/2016/07/crypto-flaw-made-it-easy-for-attacker... for what happens when the subject name of a cert is used to make a security decision. :-) ) https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:619: static base::LazyInstance<EnumerateModulesModel>::Leaky model; = LAZY_INSTANCE_INITIALIZER; https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:620: return &model.Get(); model.Pointer(); https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:686: // Instruct the ModuleEnumerator class to load this on the File thread. FILE https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:805: base::TimeDelta::FromMilliseconds(kModuleCheckDelayMs), if this delay is meant to be "wait until the browser has started", please switch to content::BrowserThread::PostAfterStartupTask https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:812: module_enumerator_.reset(); somewhere in here to release resources? in which case, please add doc comments to the enumerator's ReportBack method indicating that |this| must not be touched after the call to DoneScanning() since its observer may delete it. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.h (right): https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:11: #include "base/callback.h" unused? https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:29: // A helper class that implements the enumerate module functionality on the File File -> FILE https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:31: class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> { it looks like this is owned by the model. does it need to be refcounted? https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:133: // will notify when done through the MODULE_LIST_ENUMERATED notification. MODULE_LIST_ENUMERATED notification -> observers' DoneScanning methods? https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:134: // The process will also send MODULE_INCOMPATIBILITY_BADGE_CHANGE to let OnConflictsAcknowledged https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:135: // observers know when it is time to update the wrench menu badge. This should should -> must, unless it's okay to call from any thread and add a DCHECK in the impl? https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:189: // process is done. It notifies the observer. This should only be called on should -> must https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:220: // This is a singleton class that enumerates all modules loaded into Chrome, if this is the class that consumers of this .h are actually supposed to use, then it should come first in the .h. since moving it will introduce a ton of churn and make this review difficult, please do so in a separate CL either before or after landing this refactor. actually the enumerator is sufficiently complex that it should be in its own .h and .cc file. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:230: // This class is intended to be used from the UI thread. The bulk of the work is after reading the individual method doc comments, it looks like a few are to be called from any thread. this comment up here is a bit misleading. maybe change it to something like "unless noted otherwise, all methods must be called on the UI thread." and then only document the exceptions below. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:258: virtual ~EnumerateModulesModel() {} this is a singleton, right? is it designed to be the base class of something else? if no, remove virtual. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:260: // Returns the singleton instance of this class. Can be called on any thread. why called on any thread? the class doc says it's to be used on the UI thread. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:327: // The object responsible for enumerating the modules on the File thread. FILE
Thanks for the review Greg. Another look? https://codereview.chromium.org/2037883004/diff/160001/base/lazy_instance.h File base/lazy_instance.h (right): https://codereview.chromium.org/2037883004/diff/160001/base/lazy_instance.h#n... base/lazy_instance.h:43: #include "base/lazy_instance.h" On 2016/07/25 10:57:10, grt (UTC plus 2) wrote: > ping Doh! https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:101: const base::FilePath& filename) { On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > nit: unwrap Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:102: HCERTSTORE store = NULL; On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > nullptr all the things Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:591: signed_by = GetSignedByInCatalog(filename); On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > nit: return this directly rather than going through the local var Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:600: ++signed_modules; On 2016/07/25 10:57:10, grt (UTC plus 2) wrote: > nit: indentation. (git cl format wil fix this) Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:601: if (module.digital_signer.find(L"Microsoft") != base::string16::npos) On 2016/07/25 10:57:10, grt (UTC plus 2) wrote: > this check can be trivially made to succeed. are you hoping to just get a guess > at how many binaries are signed by MS? (see > http://arstechnica.com/security/2016/07/crypto-flaw-made-it-easy-for-attacker... > for what happens when the subject name of a cert is used to make a security > decision. :-) ) For now, yup. Just to see how big the non-MS-signed surface is. Will get more detailed stats either via RAPPOR or anti-malware data. When making real decisions we'll actually have a whitelist of certificates themselves, and we'll be checking signatures. No need for concern (yet), but point well taken. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:619: static base::LazyInstance<EnumerateModulesModel>::Leaky model; On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > = LAZY_INSTANCE_INITIALIZER; Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:620: return &model.Get(); On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > model.Pointer(); Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:686: // Instruct the ModuleEnumerator class to load this on the File thread. On 2016/07/25 10:57:10, grt (UTC plus 2) wrote: > FILE Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:805: base::TimeDelta::FromMilliseconds(kModuleCheckDelayMs), On 2016/07/25 10:57:10, grt (UTC plus 2) wrote: > if this delay is meant to be "wait until the browser has started", please switch > to content::BrowserThread::PostAfterStartupTask Ooh, fancy. Thanks. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:812: On 2016/07/25 10:57:10, grt (UTC plus 2) wrote: > module_enumerator_.reset(); somewhere in here to release resources? in which > case, please add doc comments to the enumerator's ReportBack method indicating > that |this| must not be touched after the call to DoneScanning() since its > observer may delete it. Good suggestion. Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.h (right): https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:11: #include "base/callback.h" On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:29: // A helper class that implements the enumerate module functionality on the File On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > File -> FILE Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:31: class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> { On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > it looks like this is owned by the model. does it need to be refcounted? Can't see any reason why it needs to be. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:133: // will notify when done through the MODULE_LIST_ENUMERATED notification. On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > MODULE_LIST_ENUMERATED notification -> observers' DoneScanning methods? Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:134: // The process will also send MODULE_INCOMPATIBILITY_BADGE_CHANGE to let On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > OnConflictsAcknowledged Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:135: // observers know when it is time to update the wrench menu badge. This should On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > should -> must, unless it's okay to call from any thread > and add a DCHECK in the impl? Changed the wording. DCHECK already there. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:189: // process is done. It notifies the observer. This should only be called on On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > should -> must Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:220: // This is a singleton class that enumerates all modules loaded into Chrome, On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > if this is the class that consumers of this .h are actually supposed to use, > then it should come first in the .h. since moving it will introduce a ton of > churn and make this review difficult, please do so in a separate CL either > before or after landing this refactor. actually the enumerator is sufficiently > complex that it should be in its own .h and .cc file. +1. Will do in a follow-up CL. Added a comment to that effect. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:230: // This class is intended to be used from the UI thread. The bulk of the work is On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > after reading the individual method doc comments, it looks like a few are to be > called from any thread. this comment up here is a bit misleading. maybe change > it to something like "unless noted otherwise, all methods must be called on the > UI thread." and then only document the exceptions below. Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:258: virtual ~EnumerateModulesModel() {} On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > this is a singleton, right? is it designed to be the base class of something > else? if no, remove virtual. Done. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:260: // Returns the singleton instance of this class. Can be called on any thread. On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > why called on any thread? the class doc says it's to be used on the UI thread. It only gets used from the UI thread right now. Simply changed things to say UI only. https://codereview.chromium.org/2037883004/diff/160001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:327: // The object responsible for enumerating the modules on the File thread. On 2016/07/25 10:57:11, grt (UTC plus 2) wrote: > FILE Done.
Ping?
awesomeness! https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:16: #include <memory> remove (included in .h) https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:612: static base::LazyInstance<EnumerateModulesModel>::Leaky model = if this will always forevermore be UI-thread-only, you no longer need the thread-safety of LazyInstance and can use: #include "base/debug/leak_annotations.h" static EnumerateModulesModel* model = nullptr; if (!model) { model = new EnumerateModulesModel(); ANNOTATE_LEAKING_OBJECT_PTR(model); } return model; https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:685: base::AutoLock lock(lock_); is the lock needed? it looks like all accesses of scanning_ take place on the UI thread now that you've added the DCHECKs. https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:686: if (scanning_) can you get rid of scanning_ now and use the nullness of module_enumerator_ instead? https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.h (right): https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:295: ~EnumerateModulesModel() {} this class might be sufficiently complex that this dtor needs to be out-of-lined in the .cc file. i think the win clang bots will tell you if this is the case. alternatively, just out-of-line it to avoid anyone else wondering. https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:315: std::unique_ptr<ModuleEnumerator> module_enumerator_; #include <memory> for this
Okay, hopefully a last look? https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:16: #include <memory> On 2016/07/29 07:35:34, grt (UTC plus 2) wrote: > remove (included in .h) Done. https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:612: static base::LazyInstance<EnumerateModulesModel>::Leaky model = On 2016/07/29 07:35:34, grt (UTC plus 2) wrote: > if this will always forevermore be UI-thread-only, you no longer need the > thread-safety of LazyInstance and can use: > #include "base/debug/leak_annotations.h" > static EnumerateModulesModel* model = nullptr; > if (!model) { > model = new EnumerateModulesModel(); > ANNOTATE_LEAKING_OBJECT_PTR(model); > } > return model; Nice, wasn't aware of the leak annotations! I'll go with that for now. I don't see any reason why this needs to be multi-threaded. https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:685: base::AutoLock lock(lock_); On 2016/07/29 07:35:34, grt (UTC plus 2) wrote: > is the lock needed? it looks like all accesses of scanning_ take place on the UI > thread now that you've added the DCHECKs. Indeed I can. Done. https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:686: if (scanning_) On 2016/07/29 07:35:34, grt (UTC plus 2) wrote: > can you get rid of scanning_ now and use the nullness of module_enumerator_ > instead? And that too. Done. https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.h (right): https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:295: ~EnumerateModulesModel() {} On 2016/07/29 07:35:34, grt (UTC plus 2) wrote: > this class might be sufficiently complex that this dtor needs to be out-of-lined > in the .cc file. i think the win clang bots will tell you if this is the case. > alternatively, just out-of-line it to avoid anyone else wondering. Done. https://codereview.chromium.org/2037883004/diff/180001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:315: std::unique_ptr<ModuleEnumerator> module_enumerator_; On 2016/07/29 07:35:34, grt (UTC plus 2) wrote: > #include <memory> for this Done.
lgtm.
Thanks, committing!
Patchset #13 (id:240001) has been deleted
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2037883004/#ps220001 (title: "Small fixes.")
The CQ bit was unchecked by chrisha@chromium.org
chrisha@chromium.org changed reviewers: + cpu@chromium.org, pkasting@chromium.org, thestig@chromium.org
chrisha@chromium.org changed required reviewers: + cpu@chromium.org, pkasting@chromium.org, thestig@chromium.org
PTAL? cpu: chrome/browser/diagnostics pkasting: chrome/browser/ui thestig: everything else in chrome/
https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_icon_controller.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_icon_controller.cc:89: auto modules = EnumerateModulesModel::GetInstance(); auto* https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/conflicts_ui.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/conflicts_ui.cc:107: auto model = EnumerateModulesModel::GetInstance(); auto* https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/conflicts_ui.cc:108: model->AddObserver(this); Can this end up adding the same observer twice? This is a JS handler right? Can JS send 2 requestModuleList messages before OnScanCompleted() ? https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:251: buffer.data(), 0)) { indentation looks a bit weird. git cl format the whole thing? https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:394: // Send a reply back on the UI thread. The |observer_| is a leaky singleton While the leaky singleton part is true, it's not a great assumption to make. Can it instead be simply that |observer_| outlives this class? Maybe add a comment to indicate this requirement to the ModuleEnumerator ctor? https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:593: if (module.digital_signer.find(L"Microsoft") != base::string16::npos) Would you be looking for strings that start with Microsoft by any chance? https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:615: ANNOTATE_LEAKING_OBJECT_PTR(model); Use a LazyInstance? https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:688: if (module_enumerator_.get()) You can omit the .get() https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:709: for (ModuleEnumerator::ModulesVector::const_iterator module = It would be nice to eventually switch to a range-based for-loop and use unique_ptrs inside. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:750: if (!actions.empty()) Is this ever true? https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.h (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:202: // The observers, who need to be notified when the scan is complete. Isn't it still only a single observer from the perspective of ModuleEnumerator?
LGTM, mostly food for thought https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/chro... File chrome/browser/ui/chrome_pages.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/chro... chrome/browser/ui/chrome_pages.cc:189: if (conflict_url.is_valid()) { I don't think the system should be set up so you need to do both the ShouldShowConflictWarning() check and this check. Either: * Make GetConflictUrl() unconditionally return a valid URL, and remove this is_valid() check, or * Make GetConflictUrl() solely responsible for checking ShouldShowConflictWarning(), and remove that condition from this caller. Currently, GetConflictUrl() does the latter, so the smallest change would be to implement that. You might choose to do the former anyway, depending on what your future plans for that method are. If you do the former, consider whether you could make the URL in question a constant instead of something returned by a method. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_icon_controller.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_icon_controller.cc:90: modules->AddObserver(this); Nit: Consider whether it would be cleaner or safer to use a ScopedObserver in place of the manual AddObserver() and RemoveObserver() calls. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_icon_controller.h (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_icon_controller.h:15: #if defined(OS_WIN) Nit: Consider whether it might be cleaner to have at least the Observer class from here defined in a not-win-specific file. This would let you avoid all the ifdefs in this file and at least some in the .cc file. You may decide that it's worth keeping the ifdefs here, to emphasize that none of this functionality is reached in non-Windows. On the other end, you might go all the way to making EnumerateModulesModel itself be declared in cross-platform files, and only ever actually do anything when run on Windows, which would remove all the ifdefs from here and the .cc file. This would depend partly on the degree of "black-boxing" you want for that class, and how much callers should know about what it does and why we care about it. It would also depend on whether it would ever make sense conceptually to run such code on other OSes. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_model.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_model.cc:690: return false; Nit: Could factor IDC_PIN_TO_START_SCREEN out of the #ifdef since both implementations are the same. The absolute most compact way (although this is a bit fragile, and you might not choose to do it) would be: case IDC_VIEW_INCOMPATIBILITIES: #if defined(OS_WIN) return EnumerateModulesModel::GetInstance()->ShouldShowConflictWarning(); #endif case IDC_PIN_TO_START_SCREEN: return false; If that makes you uneasy, the simpler route would be to just explicitly do: case IDC_VIEW_INCOMPATIBILITIES: #if defined(OS_WIN) return EnumerateModulesModel::GetInstance()->ShouldShowConflictWarning(); #else return false; #endif case IDC_PIN_TO_START_SCREEN: return false; https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/conflicting_module_view_win.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/conflicting_module_view_win.cc:54: EnumerateModulesModel::GetInstance()->AddObserver(this); Nit: See earlier comment about whether using a ScopedObserver is cleaner or safer https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/conflicting_module_view_win.cc:66: if (!url.is_valid()) { Nit: See earlier comment about whether this is necessary https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/conflicts_ui.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/conflicts_ui.cc:108: model->AddObserver(this); On 2016/08/04 14:33:32, Lei Zhang wrote: > Can this end up adding the same observer twice? This is a JS handler right? Can > JS send 2 requestModuleList messages before OnScanCompleted() ? If this is a real concern, using a ScopedObserver would allow this to be checked simply using the IsObserving() method. OTOH, that's a less-helpful pattern otherwise in this class, since the RemoveObserver call does not happen at destruction. (Although, do you need to worry about being destroyed without OnScanCompleted() being reached? Because if so then you do need such a call.)
https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:137: you need to close |store| and free |message|
https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:137: On 2016/08/11 15:00:57, cpu wrote: > you need to close |store| and free |message| Indeed! I didn't review this code since it looked like it just moved. Ooops.
Thanks for the thorough reviews. Another look? https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/chro... File chrome/browser/ui/chrome_pages.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/chro... chrome/browser/ui/chrome_pages.cc:189: if (conflict_url.is_valid()) { On 2016/08/05 00:10:08, Peter Kasting wrote: > I don't think the system should be set up so you need to do both the > ShouldShowConflictWarning() check and this check. Either: > > * Make GetConflictUrl() unconditionally return a valid URL, and remove this > is_valid() check, or > * Make GetConflictUrl() solely responsible for checking > ShouldShowConflictWarning(), and remove that condition from this caller. > > Currently, GetConflictUrl() does the latter, so the smallest change would be to > implement that. You might choose to do the former anyway, depending on what > your future plans for that method are. If you do the former, consider whether > you could make the URL in question a constant instead of something returned by a > method. Collapsed logic into GetConflictUrl. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_icon_controller.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_icon_controller.cc:89: auto modules = EnumerateModulesModel::GetInstance(); On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > auto* Done. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_icon_controller.cc:90: modules->AddObserver(this); On 2016/08/05 00:10:08, Peter Kasting wrote: > Nit: Consider whether it would be cleaner or safer to use a ScopedObserver in > place of the manual AddObserver() and RemoveObserver() calls. I'm not a huge fan of precompiler logic in initializer lists, and think it reads cleaner this way. Would be happen to change this if we ever make the class definition exist across OSes. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_icon_controller.h (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_icon_controller.h:15: #if defined(OS_WIN) On 2016/08/05 00:10:08, Peter Kasting wrote: > Nit: Consider whether it might be cleaner to have at least the Observer class > from here defined in a not-win-specific file. This would let you avoid all the > ifdefs in this file and at least some in the .cc file. > > You may decide that it's worth keeping the ifdefs here, to emphasize that none > of this functionality is reached in non-Windows. On the other end, you might go > all the way to making EnumerateModulesModel itself be declared in cross-platform > files, and only ever actually do anything when run on Windows, which would > remove all the ifdefs from here and the .cc file. This would depend partly on > the degree of "black-boxing" you want for that class, and how much callers > should know about what it does and why we care about it. It would also depend > on whether it would ever make sense conceptually to run such code on other OSes. I'll stick with the existing system for now. I think it makes sense to make the definition available across platforms if it ever becomes used across more than one of them, and have left a comment to that effect. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_model.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_model.cc:690: return false; On 2016/08/05 00:10:08, Peter Kasting wrote: > Nit: Could factor IDC_PIN_TO_START_SCREEN out of the #ifdef since both > implementations are the same. The absolute most compact way (although this is a > bit fragile, and you might not choose to do it) would be: > > case IDC_VIEW_INCOMPATIBILITIES: > #if defined(OS_WIN) > return EnumerateModulesModel::GetInstance()->ShouldShowConflictWarning(); > #endif > case IDC_PIN_TO_START_SCREEN: > return false; > > If that makes you uneasy, the simpler route would be to just explicitly do: > > case IDC_VIEW_INCOMPATIBILITIES: > #if defined(OS_WIN) > return EnumerateModulesModel::GetInstance()->ShouldShowConflictWarning(); > #else > return false; > #endif > case IDC_PIN_TO_START_SCREEN: > return false; Thanks for the suggestion. Went the less fragile route. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/conflicting_module_view_win.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/conflicting_module_view_win.cc:54: EnumerateModulesModel::GetInstance()->AddObserver(this); On 2016/08/05 00:10:08, Peter Kasting wrote: > Nit: See earlier comment about whether using a ScopedObserver is cleaner or > safer Done. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/conflicting_module_view_win.cc:66: if (!url.is_valid()) { On 2016/08/05 00:10:08, Peter Kasting wrote: > Nit: See earlier comment about whether this is necessary Acknowledged. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/conflicts_ui.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/conflicts_ui.cc:107: auto model = EnumerateModulesModel::GetInstance(); On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > auto* Done. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/conflicts_ui.cc:108: model->AddObserver(this); On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > Can this end up adding the same observer twice? This is a JS handler right? Can > JS send 2 requestModuleList messages before OnScanCompleted() ? Shouldn't be possible from how the JS code is written, but using a ScopedObserver to safely handle this case is easy enough. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/conflicts_ui.cc:108: model->AddObserver(this); On 2016/08/05 00:10:08, Peter Kasting wrote: > On 2016/08/04 14:33:32, Lei Zhang wrote: > > Can this end up adding the same observer twice? This is a JS handler right? > Can > > JS send 2 requestModuleList messages before OnScanCompleted() ? > > If this is a real concern, using a ScopedObserver would allow this to be checked > simply using the IsObserving() method. OTOH, that's a less-helpful pattern > otherwise in this class, since the RemoveObserver call does not happen at > destruction. (Although, do you need to worry about being destroyed without > OnScanCompleted() being reached? Because if so then you do need such a call.) The ScopedObserver is cleaner IMO, and also allows me to handle this possible case of 'abuse' from the JS. Switched to using that. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:137: On 2016/08/11 15:00:57, cpu wrote: > you need to close |store| and free |message| Ach... copy and pasted code, thanks for catching that. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:137: On 2016/08/12 07:56:05, grt (UTC plus 2) wrote: > On 2016/08/11 15:00:57, cpu wrote: > > you need to close |store| and free |message| > > Indeed! I didn't review this code since it looked like it just moved. Ooops. Indeed it was only moved. I suppose I should have looked through it in more detail as well :/ https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:251: buffer.data(), 0)) { On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > indentation looks a bit weird. git cl format the whole thing? Done. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:394: // Send a reply back on the UI thread. The |observer_| is a leaky singleton On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > While the leaky singleton part is true, it's not a great assumption to make. Can > it instead be simply that |observer_| outlives this class? Maybe add a comment > to indicate this requirement to the ModuleEnumerator ctor? Fair enough. Done. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:593: if (module.digital_signer.find(L"Microsoft") != base::string16::npos) On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > Would you be looking for strings that start with Microsoft by any chance? Yeah, starting with Microsoft is fine. Fixed. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:615: ANNOTATE_LEAKING_OBJECT_PTR(model); On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > Use a LazyInstance? LazyInstance is thread safe, which is a stronger guarantee than we needed. Previous reviewer suggested I drop the LazyInstance and simply annotate, using the "weakest" form necessary. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:688: if (module_enumerator_.get()) On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > You can omit the .get() Done. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:709: for (ModuleEnumerator::ModulesVector::const_iterator module = On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > It would be nice to eventually switch to a range-based for-loop and use > unique_ptrs inside. Acknowledged. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.cc:750: if (!actions.empty()) On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > Is this ever true? Nope, indeed it can't be. Fixed. https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... File chrome/browser/win/enumerate_modules_model.h (right): https://codereview.chromium.org/2037883004/diff/220001/chrome/browser/win/enu... chrome/browser/win/enumerate_modules_model.h:202: // The observers, who need to be notified when the scan is complete. On 2016/08/04 14:33:32, Lei Zhang (OOO) wrote: > Isn't it still only a single observer from the perspective of ModuleEnumerator? Yup, not sure why I changed the comment. Reverted.
The CQ bit was checked by chrisha@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_...)
The CQ bit was checked by chrisha@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
LGTM
On 2016/08/12 19:04:41, chrisha (slow) wrote: > Thanks for the thorough reviews. Another look? Red win bots are yours?
lgtm
On 2016/08/15 00:58:55, Lei Zhang wrote: > On 2016/08/12 19:04:41, chrisha (slow) wrote: > > Thanks for the thorough reviews. Another look? > > Red win bots are yours? Not landing until I can get a change to check this out, thanks for the heads up.
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, grt@chromium.org, cpu@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2037883004/#ps310001 (title: "Rebased.")
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
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by thestig@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...
On 2016/08/17 17:23:05, chrisha (slow) wrote: > On 2016/08/15 00:58:55, Lei Zhang wrote: > > On 2016/08/12 19:04:41, chrisha (slow) wrote: > > > Thanks for the thorough reviews. Another look? > > > > Red win bots are yours? > > Not landing until I can get a change to check this out, thanks for the heads up. It looks like your CL is still causing Windows bots to go red.
The only one that looked relevant was the win-clang failure, which I fixed (but apparently didn't upload)?
On 2016/08/22 17:15:01, chrisha (slow) wrote: > The only one that looked relevant was the win-clang failure, which I fixed (but > apparently didn't upload)? Patch set 15 has this try job: https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... The AutomationApiTest.Attributes browser_tests failure link is: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2... If you scroll down, you'll see: [2608:3880:0812/135902:FATAL:enumerate_modules_model.cc(719)] Check failed: ::content::BrowserThread::CurrentlyOn(BrowserThread::UI). Must be called on Chrome_UIThread; actually called on Chrome_FileThread. Backtrace: base::debug::StackTrace::StackTrace [0x000000014195EBE1+33] logging::LogMessage::~LogMessage [0x000000014190581C+76] EnumerateModulesModel::ScanNow [0x0000000144E8B71F+159] AfterStartupTaskUtils::PostTask [0x0000000141B14CCA+662]
The CQ bit was checked by chrisha@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...
Patchset #18 (id:350001) has been deleted
On 2016/08/22 17:17:54, Lei Zhang wrote: > On 2016/08/22 17:15:01, chrisha (slow) wrote: > > The only one that looked relevant was the win-clang failure, which I fixed > (but > > apparently didn't upload)? > > Patch set 15 has this try job: > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... > > The AutomationApiTest.Attributes browser_tests failure link is: > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2... > > If you scroll down, you'll see: > > [2608:3880:0812/135902:FATAL:enumerate_modules_model.cc(719)] Check failed: > ::content::BrowserThread::CurrentlyOn(BrowserThread::UI). Must be called on > Chrome_UIThread; actually called on Chrome_FileThread. > Backtrace: > base::debug::StackTrace::StackTrace [0x000000014195EBE1+33] > logging::LogMessage::~LogMessage [0x000000014190581C+76] > EnumerateModulesModel::ScanNow [0x0000000144E8B71F+159] > AfterStartupTaskUtils::PostTask [0x0000000141B14CCA+662] Oops, was dispatching to the wrong thread. Fixed. A last look, hopefully?
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== [Win] Add reporting of total number of modules loaded in browser process. BUG=617176,619923 ========== to ========== [Win] Add reporting of total number of modules loaded in browser process. BUG=617176,619923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, cpu@chromium.org, pkasting@chromium.org, grt@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2037883004/#ps390001 (title: "Fix constant in unittest.")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, rkaplow@chromium.org, cpu@chromium.org, pkasting@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2037883004/#ps410001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Win] Add reporting of total number of modules loaded in browser process. BUG=617176,619923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [Win] Add reporting of total number of modules loaded in browser process. BUG=617176,619923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Message was sent while issue was closed.
Committed patchset #20 (id:410001)
Message was sent while issue was closed.
Description was changed from ========== [Win] Add reporting of total number of modules loaded in browser process. BUG=617176,619923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [Win] Add reporting of total number of modules loaded in browser process. BUG=617176,619923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/8645b5898bbcb15d7886daf5399c923af20b08dc Cr-Commit-Position: refs/heads/master@{#415375} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/8645b5898bbcb15d7886daf5399c923af20b08dc Cr-Commit-Position: refs/heads/master@{#415375}
Message was sent while issue was closed.
🎆! |
