Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2473783005:
[Win] Create ModuleWatcher. (Closed)
Description[Win] Create ModuleWatcher.
This is a utility class that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI.
BUG=662084
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/9d5f46c81db254725e6b8215937097a88b2f558c
Cr-Commit-Position: refs/heads/master@{#434251}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Address pmonette's comments. #
Total comments: 10
Patch Set 3 : Refactor threading and observer model. #
Total comments: 49
Patch Set 4 : Address grt's review. #Patch Set 5 : Small cleanup. #
Total comments: 46
Patch Set 6 : Addressed grt's comments. #Patch Set 7 : Small cleanup. #
Total comments: 14
Patch Set 8 : Address grt's nits. #
Total comments: 15
Patch Set 9 : Rebased. #Patch Set 10 : Address wfh@ nit. #Patch Set 11 : Remove errant line post rebase. #Patch Set 12 : Fix unittest. #Patch Set 13 : Add conflits_dll for testing. #
Messages
Total messages: 68 (27 generated)
chrisha@chromium.org changed reviewers: + pmonette@chromium.org, sky@chromium.org
PTAL?
https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:17: namespace { Usually, the unnamed namespace is put inside the file namespace (conflicts). Unless it was causing problems with the typedefs below. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:87: CHECK_NE(static_cast<const UNICODE_STRING*>(NULL), str); This should probably be a DCHECK. Also s/NULL/nullptr. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:93: CHECK_NE(static_cast<HMODULE>(NULL), ntdll); Same https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:149: ModuleEvent event = {}; All 4 members of ModuleEvent are always initialized right after creation. Add a constructor or use uniform initialization syntax: ModuleEvent event = { MODULE_LOADED, base::FilePath(ToStringPiece(load_data.FullDllName)), load_data.DllBase, load_data.SizeOfImage, }; https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:169: ModuleWatcher::ModuleWatcher() I looked at the design doc. Won't each ModuleWatcher only have one observer? If this is the case the logic could be simplified. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:174: ModuleWatcher::~ModuleWatcher() {} = default; https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:202: bool ModuleWatcher::RegisterDllNotificationCallback() { lock_.AssertAquired() in all relevant functions? https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:207: if (reg_fn == NULL) if (!reg_fn) https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:215: bool ModuleWatcher::UnregisterDllNotificationCallback() { The return value is unused. Not sure if it's worth removing though. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:220: if (unreg_fn == NULL) if (!unreg_fn) https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:231: if (!snap.Get()) if (!snap.IsValid()) https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.h:64: std::unique_ptr<Observer> RegisterCallback( Seems like you are implementing base::CallbackList<> in a thread-safe way. Maybe you could implement the CallbackListThreadSafe class in order to separate this responsability from ModuleWatcher, which would make its implementation quite a bit simpler. Feel free to ignore this suggestion.
Thanks pmonette. Another look? sky: Just looking for OWNER approval to carve out a new space for the work outlined in this document: https://docs.google.com/document/d/1iCQyTuTRG-2buDso421jjlyGgyWuXRTPPfF7FBICu... https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:17: namespace { On 2016/11/03 20:18:15, Patrick Monette wrote: > Usually, the unnamed namespace is put inside the file namespace (conflicts). > > Unless it was causing problems with the typedefs below. Done. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:87: CHECK_NE(static_cast<const UNICODE_STRING*>(NULL), str); On 2016/11/03 20:18:15, Patrick Monette wrote: > This should probably be a DCHECK. Also s/NULL/nullptr. (This is cut and pasted code from elsewhere, oops.) Actually, we have a policy to not explcitly CHECK or DCHECK pointers, as dereferencing them is the same as advertising the fact that we expect them to be non-null, and it will deterministically crash anyways. Removed. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:93: CHECK_NE(static_cast<HMODULE>(NULL), ntdll); On 2016/11/03 20:18:15, Patrick Monette wrote: > Same Removed this helper function entirely, as the CHECK is redundant. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:149: ModuleEvent event = {}; On 2016/11/03 20:18:15, Patrick Monette wrote: > All 4 members of ModuleEvent are always initialized right after creation. > > Add a constructor or use uniform initialization syntax: > ModuleEvent event = { > MODULE_LOADED, > base::FilePath(ToStringPiece(load_data.FullDllName)), > load_data.DllBase, > load_data.SizeOfImage, > }; Done. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:169: ModuleWatcher::ModuleWatcher() On 2016/11/03 20:18:15, Patrick Monette wrote: > I looked at the design doc. Won't each ModuleWatcher only have one observer? > > If this is the case the logic could be simplified. Yeah, but I need the thread affinity, as I want the notifications on the file thread (not mentioned in the design doc... I'll go update that). Rather than do the thread bouncing myself I figured it was easier to use ObserverListThreadSafe. Either way I write it, there's some amount of complexity. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:174: ModuleWatcher::~ModuleWatcher() {} On 2016/11/03 20:18:15, Patrick Monette wrote: > = default; Always forgetting about that new feature... Done https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:202: bool ModuleWatcher::RegisterDllNotificationCallback() { On 2016/11/03 20:18:15, Patrick Monette wrote: > lock_.AssertAquired() in all relevant functions? Done. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:207: if (reg_fn == NULL) On 2016/11/03 20:18:15, Patrick Monette wrote: > if (!reg_fn) Done. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:215: bool ModuleWatcher::UnregisterDllNotificationCallback() { On 2016/11/03 20:18:15, Patrick Monette wrote: > The return value is unused. Not sure if it's worth removing though. This is a thin wrapper around the system function, so I wanted to preserve the original semantics. This apparently can fail, although there's no meaningful action for us to take in that case. I'm ambivalent. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:220: if (unreg_fn == NULL) On 2016/11/03 20:18:15, Patrick Monette wrote: > if (!unreg_fn) Done. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.cc:231: if (!snap.Get()) On 2016/11/03 20:18:15, Patrick Monette wrote: > if (!snap.IsValid()) Done. https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_watcher_win.h:64: std::unique_ptr<Observer> RegisterCallback( On 2016/11/03 20:18:15, Patrick Monette wrote: > Seems like you are implementing base::CallbackList<> in a thread-safe way. > > Maybe you could implement the CallbackListThreadSafe class in order to separate > this > responsability from ModuleWatcher, which would make its implementation quite a > bit simpler. > > Feel free to ignore this suggestion. I am indeed. I'm not sure if anybody else needs this, so I'll punt on this for now. I will, however, leave a comment with CallbackListThreadSafe so that people find this code if they actually want such a thing.
On Fri, Nov 4, 2016 at 8:32 AM, <chrisha@chromium.org> wrote: > Thanks pmonette. Another look? > > sky: Just looking for OWNER approval to carve out a new space for the work > outlined in this document: > https://docs.google.com/document/d/1iCQyTuTRG- > 2buDso421jjlyGgyWuXRTPPfF7FBICurI/edit# > <https://docs.google.com/document/d/1iCQyTuTRG-2buDso421jjlyGgyWuXRTPPfF7FBICu...> > I will take a look at the patch shortly. This is a nice writeup! It doesn't seem like you sent this to chromium-dev. I encourage you to do that to get more eyes on it. -Scott > > > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc > File chrome/common/conflicts/module_watcher_win.cc (right): > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode17 > chrome/common/conflicts/module_watcher_win.cc:17: namespace { > On 2016/11/03 20:18:15, Patrick Monette wrote: > > Usually, the unnamed namespace is put inside the file namespace > (conflicts). > > > > Unless it was causing problems with the typedefs below. > > Done. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode87 > chrome/common/conflicts/module_watcher_win.cc:87: > CHECK_NE(static_cast<const UNICODE_STRING*>(NULL), str); > On 2016/11/03 20:18:15, Patrick Monette wrote: > > This should probably be a DCHECK. Also s/NULL/nullptr. > > (This is cut and pasted code from elsewhere, oops.) > > Actually, we have a policy to not explcitly CHECK or DCHECK pointers, as > dereferencing them is the same as advertising the fact that we expect > them to be non-null, and it will deterministically crash anyways. > > Removed. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode93 > chrome/common/conflicts/module_watcher_win.cc:93: > CHECK_NE(static_cast<HMODULE>(NULL), ntdll); > On 2016/11/03 20:18:15, Patrick Monette wrote: > > Same > > Removed this helper function entirely, as the CHECK is redundant. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode149 > chrome/common/conflicts/module_watcher_win.cc:149: ModuleEvent event = > {}; > On 2016/11/03 20:18:15, Patrick Monette wrote: > > All 4 members of ModuleEvent are always initialized right after > creation. > > > > Add a constructor or use uniform initialization syntax: > > ModuleEvent event = { > > MODULE_LOADED, > > base::FilePath(ToStringPiece(load_data.FullDllName)), > > load_data.DllBase, > > load_data.SizeOfImage, > > }; > > Done. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode169 > chrome/common/conflicts/module_watcher_win.cc:169: > ModuleWatcher::ModuleWatcher() > On 2016/11/03 20:18:15, Patrick Monette wrote: > > I looked at the design doc. Won't each ModuleWatcher only have one > observer? > > > > If this is the case the logic could be simplified. > > Yeah, but I need the thread affinity, as I want the notifications on the > file thread (not mentioned in the design doc... I'll go update that). > > Rather than do the thread bouncing myself I figured it was easier to use > ObserverListThreadSafe. Either way I write it, there's some amount of > complexity. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode174 > chrome/common/conflicts/module_watcher_win.cc:174: > ModuleWatcher::~ModuleWatcher() {} > On 2016/11/03 20:18:15, Patrick Monette wrote: > > = default; > > Always forgetting about that new feature... > > Done > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode202 > chrome/common/conflicts/module_watcher_win.cc:202: bool > ModuleWatcher::RegisterDllNotificationCallback() { > On 2016/11/03 20:18:15, Patrick Monette wrote: > > lock_.AssertAquired() in all relevant functions? > > Done. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode207 > chrome/common/conflicts/module_watcher_win.cc:207: if (reg_fn == NULL) > On 2016/11/03 20:18:15, Patrick Monette wrote: > > if (!reg_fn) > > Done. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode215 > chrome/common/conflicts/module_watcher_win.cc:215: bool > ModuleWatcher::UnregisterDllNotificationCallback() { > On 2016/11/03 20:18:15, Patrick Monette wrote: > > The return value is unused. Not sure if it's worth removing though. > > This is a thin wrapper around the system function, so I wanted to > preserve the original semantics. This apparently can fail, although > there's no meaningful action for us to take in that case. I'm > ambivalent. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode220 > chrome/common/conflicts/module_watcher_win.cc:220: if (unreg_fn == NULL) > On 2016/11/03 20:18:15, Patrick Monette wrote: > > if (!unreg_fn) > > Done. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.cc#newcode231 > chrome/common/conflicts/module_watcher_win.cc:231: if (!snap.Get()) > On 2016/11/03 20:18:15, Patrick Monette wrote: > > if (!snap.IsValid()) > > Done. > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.h > File chrome/common/conflicts/module_watcher_win.h (right): > > https://codereview.chromium.org/2473783005/diff/1/chrome/ > common/conflicts/module_watcher_win.h#newcode64 > chrome/common/conflicts/module_watcher_win.h:64: > std::unique_ptr<Observer> RegisterCallback( > On 2016/11/03 20:18:15, Patrick Monette wrote: > > Seems like you are implementing base::CallbackList<> in a thread-safe > way. > > > > Maybe you could implement the CallbackListThreadSafe class in order to > separate > > this > > responsability from ModuleWatcher, which would make its implementation > quite a > > bit simpler. > > > > Feel free to ignore this suggestion. > > I am indeed. I'm not sure if anybody else needs this, so I'll punt on > this for now. I will, however, leave a comment with > CallbackListThreadSafe so that people find this code if they actually > want such a thing. > > https://codereview.chromium.org/2473783005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I got part way through and realized I can't fully review this until I understand the underlying threading model you're going for with ModuleWatcher. https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:35: typedef struct _LDR_DLL_LOADED_NOTIFICATION_DATA { using for all these? https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:123: }; DISALLOW_IMPLICIT_CONSTRUCTORS https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:30: enum EventType { Generally favor enum class for new code. https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:61: // Registers a callback with this ModuleWatcher. The thread on which the Can you motivate why you need the threading complexity here? I'm wondering why you can't have all observers added on a single thread? Better yet, can you detail what threading policy you are implementing here? By that I mean can one thread call start and another call stop? Do you need to worry about observers being added while processing start/stop? https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:69: std::unique_ptr<Observer> RegisterCallback( Generally in chrome code and Observer is an interface. Using the name Observer here is mildly confusing. Maybe CallbackToken? https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:152: OnModuleEventCallback callback_; Style guide says no protected members.
https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:35: typedef struct _LDR_DLL_LOADED_NOTIFICATION_DATA { On 2016/11/04 16:27:24, sky wrote: > using for all these? I'm never sure what the right solution is here. These are actually Windows definitions, so I've declared them in the Windows style. I'm ambivalent, but generally prefer to have OS things look like OS things, and Chrome things look like Chrome things. https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:30: enum EventType { On 2016/11/04 16:27:24, sky wrote: > Generally favor enum class for new code. Good point. I'll actually be moving this in a Mojo definition to remove the need to translate these things, which takes care of that for me. https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:61: // Registers a callback with this ModuleWatcher. The thread on which the On 2016/11/04 16:27:24, sky wrote: > Can you motivate why you need the threading complexity here? I'm wondering why > you can't have all observers added on a single thread? The problem is with the LdrDllNotification callback. The callback comes in on the thread that is doing the DLL loading, which can be most any thread (almost always the main thread, which becomes the UI thread). However, in the context of injected third party software it could be on a thread that Chrome doesn't manage or supervise. Without manually doing the thread bouncing I was looking for a simple way to have the callback propagate to a thread of my choice. And this component needs to run in child processes as well, so thanks for opening up this can of worms :) In the browser, I want the notifications to arrive on the FILE thread, as they'll feed directly into the ModuleDatabaseImpl, which needs to go to disk and do some work. In child processes the notifications will be thin wrappers that push the data directly to the browser via the ModuleDatabase Mojo interface. Thus, I actually don't care about the thread in which they're run, unless Mojo has some threading requirements in child processes. > Better yet, can you detail what threading policy you are implementing here? By > that I mean can one thread call start and another call stop? Do you need to > worry about observers being added while processing start/stop? I expect the watcher to be controlled from the browser impl and renderer impl, and realistically it can be a leaky singleton that is only ever started. For unittests, I need to be able to "stop" it so that the callback gets uninstalled and doesn't leak into other tests. The "Stop" could be made private and renamed StopForTesting. Thoughts? https://codereview.chromium.org/2473783005/diff/20001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:69: std::unique_ptr<Observer> RegisterCallback( On 2016/11/04 16:27:24, sky wrote: > Generally in chrome code and Observer is an interface. Using the name Observer > here is mildly confusing. Maybe CallbackToken? Yeah, or "Subscription" if I want to follow the CallbackList model. Point taken.
Okay... thought this through a bit more and chatted with gab@. I've added a "Threading Model" discussion to the linked doc which explains the rationale, but basically the ModuleWatcher doesn't need to care about which thread it's running on at all. The only constraint is that Start and Stop be called from the same thread. The ModuleDatabaseImpl will have to use thread hopping for other reasons, so I'll push all that complexity to it. I'll follow up with a revised CL, and much more thorough comments in the header. Thanks for asking the right questions, sky :)
My inclination would be to only only support notifying on the thread that creates ModuleWatcher. If you get notification on any other thread, bounce it to the right thread and process it there. -Scott On Fri, Nov 4, 2016 at 1:28 PM, <chrisha@chromium.org> wrote: > Okay... thought this through a bit more and chatted with gab@. I've added a > "Threading Model" discussion to the linked doc which explains the rationale, > but > basically the ModuleWatcher doesn't need to care about which thread it's > running > on at all. The only constraint is that Start and Stop be called from the > same > thread. > > The ModuleDatabaseImpl will have to use thread hopping for other reasons, so > I'll push all that complexity to it. > > I'll follow up with a revised CL, and much more thorough comments in the > header. > > Thanks for asking the right questions, sky :) > > https://codereview.chromium.org/2473783005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== [Win] Create ModuleWatcher. This is a reusable component that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 ========== to ========== [Win] Create ModuleWatcher. This is a reusable component that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 ==========
> My inclination would be to only only support notifying on the thread > that creates ModuleWatcher. If you get notification on any other > thread, bounce it to the right thread and process it there. I simplified even further and made this a "single observer" object, without thread affinity or knowledge, and killing the "Start" and "Stop". The two observers of this class are: - in renderers, a class that will forward the message to ModuleDatabaseImpl in browser, via Mojo IPC. Mojo handles thread hopping for us. - in browser, ModuleDatabaseImpl will own a local ModuleWatcher. It will already have to handle thread hopping due to receiving and dispatching work across multiple threads, so it might as well also thread-hopping for any received messages. PTAL?
I like the simplification. One question though. If the callback can occur on any thread, how do you protect against the ModuleWatcher being destroyed at the same time the a dll load/unload notification is coming in and trying to be processed?
grt@chromium.org changed reviewers: + grt@chromium.org
drive-by https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:8: #include <windows.h> nit: windows.h often needs to be included before other MS headers, so we usually put it first. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:66: typedef VOID(CALLBACK* PLDR_DLL_NOTIFICATION_FUNCTION)( nit: using rather than typedef? https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:85: // Helper function for converting a UNICODE_STRING to an 8-bit string piece. "an 8-bit string piece" -> "a UTF-8 string" https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:88: CHECK(base::WideToUTF8(str->Buffer, str->Length / sizeof(wchar_t), &s)); why check? https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:98: class ModuleWatcher::Bridge { The style guide more or less says not to make a class of only static member functions (https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...). https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:124: ModuleWatcher* module_watcher = reinterpret_cast<ModuleWatcher*>(context); looks like this is racy since any thread could be getting notified of a load while another thread is destroying the watcher. wdyt of having |context| hold a ref on a TaskRunner to which the data is posted via a WeakPtr. there are probably other safe ways of slicing it, too. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:127: return OnModuleLoad(notification_data->Loaded, module_watcher); returning void is a bit odd. i don't think that's very idiomatic in Chromium. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:169: CHECK(RegisterDllNotificationCallback()); should a failure here really prevent Chrome from running at all? https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:174: CHECK(UnregisterDllNotificationCallback()); same comment here https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:205: ::CreateToolhelp32Snapshot(TH32CS_SNAPMODULE, ::GetCurrentProcessId())); ?TH32CS_SNAPMODULE | TH32CS_SNAPMODULE32? https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:206: if (!snap.IsValid()) msdn says to retry if it fails with ERROR_BAD_LENGTH. do you want to do that here? https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:216: CHECK(base::WideToUTF8(module.szExePath, ::wcslen(module.szExePath), &s)); why crash? https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:216: CHECK(base::WideToUTF8(module.szExePath, ::wcslen(module.szExePath), &s)); how about doing this directly into event.module_path rather than using |s|? https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:11: namespace conflicts { don't put this in a namespace; see https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pcgYLSJsii8/fYtyX.... https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:15: // This class observes modules as they are loaded and unloaded into a process' process's https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:16: // address space. It works by installing a DllNotification callback, and is this documenting an implementation detail, or does a consumer need to know this? please move any parts of this comment that relate to the implementation out of the .h and into the .cc file somewhere. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:24: // run concurrently on two separate threads (a bad idea anyways). are you saying i shouldn't use this foot gun i have here? :-) https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:30: // Creates and starts a watcher. This enumerates all loaded modules does this mean that |callback| is invoked synchronously during construction of the object? if so, please make that clear. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:32: // callback for continued notifications. Notifications originating from the is this saying that |callback| may be invoked from any thread? if so, please make that clear. the fact that DllNotifications can arrive on any thread sounds like an implementation detail to me. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:41: // Destructor. Unregisters the DllNotification callback if it was successfully "Destructor" is obvious, remove it. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:68: // called on any thread. Invoked by the constructor. "Invoked by the constructor." is it necessary to document that here? the doc should explain what the function does, not who is doing it. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:71: // Invoked by ModuleWatcher::Bridge. Dispatches the notification via "Invoked by..." same comment https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:79: void* dll_notification_cookie_; = nullptr; here rather than in the ctor
Description was changed from ========== [Win] Create ModuleWatcher. This is a reusable component that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 ========== to ========== [Win] Create ModuleWatcher. This is a utility class that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 ==========
On 2016/11/07 21:08:52, sky wrote: > I like the simplification. One question though. If the callback can occur on any > thread, how do you protect against the ModuleWatcher being destroyed at the same > time the a dll load/unload notification is coming in and trying to be processed? Doh, I had assumed (never a good idea), that the Ldr* calls were synchronized with the loader lock, as are the callbacks. Which takes care of this for me. I just tested that hypothesis and discovered it false. Will think on this a bit.
Another look grt? https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:8: #include <windows.h> On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > nit: windows.h often needs to be included before other MS headers, so we usually > put it first. Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:66: typedef VOID(CALLBACK* PLDR_DLL_NOTIFICATION_FUNCTION)( On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > nit: using rather than typedef? Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:85: // Helper function for converting a UNICODE_STRING to an 8-bit string piece. On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > "an 8-bit string piece" -> "a UTF-8 string" Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:88: CHECK(base::WideToUTF8(str->Buffer, str->Length / sizeof(wchar_t), &s)); On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > why check? Good point. Removed. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:98: class ModuleWatcher::Bridge { On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > The style guide more or less says not to make a class of only static member > functions > (https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...). I'm generally in favour of this, but I feel this case is a warranted exception: it lets me hide more of the implementation details (I don't need to know the callback function signature). I've rejigged it using a static member function. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:124: ModuleWatcher* module_watcher = reinterpret_cast<ModuleWatcher*>(context); On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > looks like this is racy since any thread could be getting notified of a load > while another thread is destroying the watcher. wdyt of having |context| hold a > ref on a TaskRunner to which the data is posted via a WeakPtr. there are > probably other safe ways of slicing it, too. I had assumed (terrible idea) that the Ldr* calls were under the loader lock, as the callbacks themselves are. Turns out this is not true (just tried it out). I'm not sure how your suggestion works? Who would own and clean up the ref that is the |context|? This pushes some complexity to the creator/owner of the ModuleWatcher, which isn't ideal IMO. Since this thing is meant to be a singleton, I could make that officially true. I've moved to using a singleton and a static lock to ensure synchronization. This is no worse than the synchronization I was assuming was occurring, and doesn't artificially bind the object to any particular task runner or thread. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:127: return OnModuleLoad(notification_data->Loaded, module_watcher); On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > returning void is a bit odd. i don't think that's very idiomatic in Chromium. Saved me a "break" :) Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:169: CHECK(RegisterDllNotificationCallback()); On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > should a failure here really prevent Chrome from running at all? No, it shouldn't. Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:174: CHECK(UnregisterDllNotificationCallback()); On 2016/11/08 14:07:51, grt (UTC plus 1) wrote: > same comment here Ditto. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:205: ::CreateToolhelp32Snapshot(TH32CS_SNAPMODULE, ::GetCurrentProcessId())); On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > ?TH32CS_SNAPMODULE | TH32CS_SNAPMODULE32? Indeed... cut and paste code from XP days. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:206: if (!snap.IsValid()) On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > msdn says to retry if it fails with ERROR_BAD_LENGTH. do you want to do that > here? Yeah, not a terrible idea. I'll put an upper bound on it. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:216: CHECK(base::WideToUTF8(module.szExePath, ::wcslen(module.szExePath), &s)); On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > why crash? Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:216: CHECK(base::WideToUTF8(module.szExePath, ::wcslen(module.szExePath), &s)); On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > how about doing this directly into event.module_path rather than using |s|? It's a mojo::String, and not a std::string :( https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:11: namespace conflicts { On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > don't put this in a namespace; see > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pcgYLSJsii8/fYtyX.... Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:15: // This class observes modules as they are loaded and unloaded into a process' On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > process's Oh sweet jebus, you just opened a can of worms: http://www.grammarbook.com/punctuation/apostro.asp (Although your suggestion is a commonly used approach, I've always been taught to end the word with an apostrophe.) https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:16: // address space. It works by installing a DllNotification callback, and On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > is this documenting an implementation detail, or does a consumer need to know > this? please move any parts of this comment that relate to the implementation > out of the .h and into the .cc file somewhere. Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:24: // run concurrently on two separate threads (a bad idea anyways). On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > are you saying i shouldn't use this foot gun i have here? :-) Acknowledged. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:30: // Creates and starts a watcher. This enumerates all loaded modules On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > does this mean that |callback| is invoked synchronously during construction of > the object? if so, please make that clear. Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:32: // callback for continued notifications. Notifications originating from the On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > is this saying that |callback| may be invoked from any thread? if so, please > make that clear. the fact that DllNotifications can arrive on any thread sounds > like an implementation detail to me. Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:41: // Destructor. Unregisters the DllNotification callback if it was successfully On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > "Destructor" is obvious, remove it. Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:68: // called on any thread. Invoked by the constructor. On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > "Invoked by the constructor." is it necessary to document that here? the doc > should explain what the function does, not who is doing it. Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:71: // Invoked by ModuleWatcher::Bridge. Dispatches the notification via On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > "Invoked by..." same comment Done. https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:79: void* dll_notification_cookie_; On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > = nullptr; here rather than in the ctor I'm not 100% a fan of that, as it leaves construction details in several places... I see the current thinking is to "encourage" its use, and this drawback is highlighted. But I'm pretty ambivalent. Done.
https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:124: ModuleWatcher* module_watcher = reinterpret_cast<ModuleWatcher*>(context); On 2016/11/08 21:45:21, chrisha (slow) wrote: > On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > > looks like this is racy since any thread could be getting notified of a load > > while another thread is destroying the watcher. wdyt of having |context| hold > a > > ref on a TaskRunner to which the data is posted via a WeakPtr. there are > > probably other safe ways of slicing it, too. > > I had assumed (terrible idea) that the Ldr* calls were under the loader lock, as > the callbacks themselves are. Turns out this is not true (just tried it out). > > I'm not sure how your suggestion works? Who would own and clean up the ref that > is the |context|? What I was thinking is that the context ptr passed to LdrRegisterDllNotification would not be a pointer to the ModuleWatcher instance, but rather a simple forwarder that holds a WeakPtr to the ModuleWatcher and a ref on the SequencedTaskRunner to which the WeakPtr is bound. This forwarder would be "owned" by the Ldr in the sense that it would be created in RegisterDllNotificationCallback and destroyed in UnregisterDllNotificationCallback. I don't think you would need any additional locking. This does mean that ModuleWatcher would need to live and die on a specific SequencedTaskRunner. Would this be a problem? I feel that a thread bouncing approach is more Chrome-ish than a locking approach, but maybe you really want/need the "any thread" aspect? Another potential benefit to the bouncing approach is that you know you're doing very little work in the dll notification function (only posting a task), whereas who knows what happens if you directly run the consumer's callback from within the notification function. > This pushes some complexity to the creator/owner of the > ModuleWatcher, which isn't ideal IMO. > > Since this thing is meant to be a singleton, I could make that officially true. > I've moved to using a singleton and a static lock to ensure synchronization. > This is no worse than the synchronization I was assuming was occurring, and > doesn't artificially bind the object to any particular task runner or thread. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:172: ::GetCurrentProcessId())); is GetCurrentProcessId free? if no, move outside of loop https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:175: if (::GetLastError() == ERROR_BAD_LENGTH) personal style: i'd invert the logic here and do away with the continue statement. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:184: if (!::Module32First(snap.Get(), &module)) both functions are documented as handing success/failure the same way, so you could switch this to: for (BOOL result = ::Module32First(snap.Get(), &module); result != FALSE; result = ::Module32Next(snap.Get(), &module)) { ... } return ::GetLastError() == ERROR_NO_MORE_FILES; then again, the return value of this is ignored, so you could get rid of the last statement. in fact, the return value is ignored for all uses of these three member functions. why not simplify and get rid of the bool? https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:234: DCHECK(false) << "Unknown LDR_DLL_NOTIFICATION_REASON: " NOTREACHED()
https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:124: ModuleWatcher* module_watcher = reinterpret_cast<ModuleWatcher*>(context); On 2016/11/09 08:37:52, grt (UTC plus 1) wrote: > On 2016/11/08 21:45:21, chrisha (slow) wrote: > > On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > > > looks like this is racy since any thread could be getting notified of a load > > > while another thread is destroying the watcher. wdyt of having |context| > hold > > a > > > ref on a TaskRunner to which the data is posted via a WeakPtr. there are > > > probably other safe ways of slicing it, too. > > > > I had assumed (terrible idea) that the Ldr* calls were under the loader lock, > as > > the callbacks themselves are. Turns out this is not true (just tried it out). > > > > I'm not sure how your suggestion works? Who would own and clean up the ref > that > > is the |context|? > > What I was thinking is that the context ptr passed to LdrRegisterDllNotification > would not be a pointer to the ModuleWatcher instance, but rather a simple > forwarder that holds a WeakPtr to the ModuleWatcher and a ref on the > SequencedTaskRunner to which the WeakPtr is bound. This forwarder would be > "owned" by the Ldr in the sense that it would be created in > RegisterDllNotificationCallback and destroyed in > UnregisterDllNotificationCallback. I don't think you would need any additional > locking. This does mean that ModuleWatcher would need to live and die on a > specific SequencedTaskRunner. Would this be a problem? I feel that a thread > bouncing approach is more Chrome-ish than a locking approach, but maybe you > really want/need the "any thread" aspect? > > Another potential benefit to the bouncing approach is that you know you're doing > very little work in the dll notification function (only posting a task), whereas > who knows what happens if you directly run the consumer's callback from within > the notification function. > > > This pushes some complexity to the creator/owner of the > > ModuleWatcher, which isn't ideal IMO. > > > > Since this thing is meant to be a singleton, I could make that officially > true. > > I've moved to using a singleton and a static lock to ensure synchronization. > > This is no worse than the synchronization I was assuming was occurring, and > > doesn't artificially bind the object to any particular task runner or thread. > Thanks for the detailed explanation. The only issue I have with this is that we'll already be doing thread bouncing in the ModuleDatabaseImpl, thus we'd be pushing thread-bouncing implementation logic to two places. In the renderer process we'll be converting the notifications directly to Mojo IPC messages, which means they'll immediately be thread-bounced a second time. I agree thread-bouncing is more Chrome-ish, but it also feels like unneeded additional complexity in this case? (This is along the lines of what sky@ was suggesting as well. I'm ambivalent overall, but have a mild dislike of the additional complexity.) https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:234: DCHECK(false) << "Unknown LDR_DLL_NOTIFICATION_REASON: " On 2016/11/09 08:37:52, grt (UTC plus 1) wrote: > NOTREACHED() Oops, I thought NOTREACHED was a fatal thing. Didn't realize it was effectively a #define for DCHECK(false).
looks good https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/40001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:124: ModuleWatcher* module_watcher = reinterpret_cast<ModuleWatcher*>(context); On 2016/11/09 15:38:07, chrisha (slow) wrote: > On 2016/11/09 08:37:52, grt (UTC plus 1) wrote: > > On 2016/11/08 21:45:21, chrisha (slow) wrote: > > > On 2016/11/08 14:07:52, grt (UTC plus 1) wrote: > > > > looks like this is racy since any thread could be getting notified of a > load > > > > while another thread is destroying the watcher. wdyt of having |context| > > hold > > > a > > > > ref on a TaskRunner to which the data is posted via a WeakPtr. there are > > > > probably other safe ways of slicing it, too. > > > > > > I had assumed (terrible idea) that the Ldr* calls were under the loader > lock, > > as > > > the callbacks themselves are. Turns out this is not true (just tried it > out). > > > > > > I'm not sure how your suggestion works? Who would own and clean up the ref > > that > > > is the |context|? > > > > What I was thinking is that the context ptr passed to > LdrRegisterDllNotification > > would not be a pointer to the ModuleWatcher instance, but rather a simple > > forwarder that holds a WeakPtr to the ModuleWatcher and a ref on the > > SequencedTaskRunner to which the WeakPtr is bound. This forwarder would be > > "owned" by the Ldr in the sense that it would be created in > > RegisterDllNotificationCallback and destroyed in > > UnregisterDllNotificationCallback. I don't think you would need any additional > > locking. This does mean that ModuleWatcher would need to live and die on a > > specific SequencedTaskRunner. Would this be a problem? I feel that a thread > > bouncing approach is more Chrome-ish than a locking approach, but maybe you > > really want/need the "any thread" aspect? > > > > Another potential benefit to the bouncing approach is that you know you're > doing > > very little work in the dll notification function (only posting a task), > whereas > > who knows what happens if you directly run the consumer's callback from within > > the notification function. > > > > > This pushes some complexity to the creator/owner of the > > > ModuleWatcher, which isn't ideal IMO. > > > > > > Since this thing is meant to be a singleton, I could make that officially > > true. > > > I've moved to using a singleton and a static lock to ensure synchronization. > > > This is no worse than the synchronization I was assuming was occurring, and > > > doesn't artificially bind the object to any particular task runner or > thread. > > > > Thanks for the detailed explanation. > > The only issue I have with this is that we'll already be doing thread bouncing > in the ModuleDatabaseImpl, thus we'd be pushing thread-bouncing implementation > logic to two places. In the renderer process we'll be converting the > notifications directly to Mojo IPC messages, which means they'll immediately be > thread-bounced a second time. > > I agree thread-bouncing is more Chrome-ish, but it also feels like unneeded > additional complexity in this case? I think that's subjective. One could make the case that this impl is more complex because it includes locking to resolve cross-thread lifetime issues rather than using the Chromium style of binding object lifetime to a SequencedTaskRunner. That said, I think it would be more lines of code with the bouncing, so this is prolly fine. > (This is along the lines of what sky@ was suggesting as well. I'm ambivalent > overall, but have a mild dislike of the additional complexity.) https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_event_win.mojom (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_event_win.mojom:23: // The load address of the module, in an integer form. This is 64-bit so that nit: "in an integer form" is superfluous. i'm not really a fan of the second sentence. i'm not sure it needs to be stated. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:17: base::Lock module_watcher_lock; nit: g_foo here and below https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:17: base::Lock module_watcher_lock; you need to use a leaky lazy instance here to avoid creating a static dtor: base::LazyInstance<base::Lock>::Leaky g_module_watcher_lock = LAZY_INSTANCE_INITIALIZER; https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:96: // static remove https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:121: return std::unique_ptr<ModuleWatcher>(module_watcher_instance); nit: return base::WrapUnique<ModuleWatcher>(module_watcher_instance); https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:124: ModuleWatcher::ModuleWatcher(const OnModuleEventCallback& callback) nit: move down below LoaderNotificationCallback to maintain ordering of declarations in .h file https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:148: &ModuleWatcher::LoaderNotificationCallback), nit: omit "ModuleWatcher::" https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:212: ModuleWatcher* module_watcher = reinterpret_cast<ModuleWatcher*>(context); |module_watcher| shouldn't actually be used as one outside of the lock. to avoid a future where someone inadvertently adds code to use it, wdyt about introducing a function in the unnamed namespace along these lines: ModuleWatcher::OnModuleEventCallback GetCallbackForLoaderNotificationContext(void* context) { acquire the lock, compare the instance, and either return an empty callback or the instance's } and then change this to: OnModuleEventCallback callback = GetCallbackForLoaderNotificationContext(context); if (callback.is_null()) return; or something along those lines. maybe shrink the name of the fn to avoid wrapping. :-) https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:213: ModuleWatcher::OnModuleEventCallback callback; nit: omit "ModuleWatcher::" https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:21: using OnModuleEventCallback = please add a doc comment explaining the restrictions on this. for example, if the loader lock is held when the callback is invoked, the implementer needs to know what they can and cannot do. in any case, consider documenting that the implementer shouldn't do anything expensive since the callback is run synchronously with the actual load event (if that is the case), so it will block some thread trying to do some work. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:28: // loader's lock!), and as such may be invoked on any thread in the process. is it under the loader's lock? i thought your discovery in https://codereview.chromium.org/2473783005/#msg16 was that it isn't. in any case, don't assume that the reader knows the implications of "under the loader's lock". https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:29: // Note that it is possible to receive two modifications for some modules as modifications -> notifications? https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:67: const void* notification_data, can you forward-decl LDR_DLL_NOTIFICATION_DATA in this file so that you don't needs the various casts for this? https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win_unittest.cc (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:22: module_event_count_++; prefer preincrement always https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:44: static const wchar_t kModuleName[] = L"chrome.dll"; is chrome.dll already a datadep of the unit_tests target? if it isn't, is there something else you can use so that building unit_tests doesn't require waiting for chrome.dll to link? chrome_elf is at least a whole lot smaller than chrome.dll. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:44: static const wchar_t kModuleName[] = L"chrome.dll"; nit: static constexpr https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:92: EXPECT_LT(0u, module_event_count_); the counts are unsigned, so just check for equality with zero. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:99: EXPECT_EQ(1u, module_loaded_event_count_); does this assume that the loaded module doesn't also bring in dependencies? if so, maybe it's better to make your own test binary that you control.
Thanks for the detailed review, grt. Another look? https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_event_win.mojom (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_event_win.mojom:23: // The load address of the module, in an integer form. This is 64-bit so that On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > nit: "in an integer form" is superfluous. i'm not really a fan of the second > sentence. i'm not sure it needs to be stated. Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:17: base::Lock module_watcher_lock; On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > you need to use a leaky lazy instance here to avoid creating a static dtor: > base::LazyInstance<base::Lock>::Leaky g_module_watcher_lock = > LAZY_INSTANCE_INITIALIZER; Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:17: base::Lock module_watcher_lock; On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > nit: g_foo here and below Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:96: // static On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > remove Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:121: return std::unique_ptr<ModuleWatcher>(module_watcher_instance); On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > nit: > return base::WrapUnique<ModuleWatcher>(module_watcher_instance); Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:124: ModuleWatcher::ModuleWatcher(const OnModuleEventCallback& callback) On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > nit: move down below LoaderNotificationCallback to maintain ordering of > declarations in .h file Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:148: &ModuleWatcher::LoaderNotificationCallback), On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > nit: omit "ModuleWatcher::" Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:172: ::GetCurrentProcessId())); On 2016/11/09 08:37:52, grt (UTC plus 1) wrote: > is GetCurrentProcessId free? if no, move outside of loop Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:175: if (::GetLastError() == ERROR_BAD_LENGTH) On 2016/11/09 08:37:52, grt (UTC plus 1) wrote: > personal style: i'd invert the logic here and do away with the continue > statement. Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:184: if (!::Module32First(snap.Get(), &module)) On 2016/11/09 08:37:52, grt (UTC plus 1) wrote: > both functions are documented as handing success/failure the same way, so you > could switch this to: > for (BOOL result = ::Module32First(snap.Get(), &module); result != FALSE; > result = ::Module32Next(snap.Get(), &module)) { > ... > } > return ::GetLastError() == ERROR_NO_MORE_FILES; > > then again, the return value of this is ignored, so you could get rid of the > last statement. in fact, the return value is ignored for all uses of these three > member functions. why not simplify and get rid of the bool? Yeah, I initially was CHECKing on these, but obviously that's a little too aggressive. I don't expect these to fail, but having that case be DCHECKed doesn't tell me much either. So removed all the bools. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:212: ModuleWatcher* module_watcher = reinterpret_cast<ModuleWatcher*>(context); On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > |module_watcher| shouldn't actually be used as one outside of the lock. to avoid > a future where someone inadvertently adds code to use it, wdyt about introducing > a function in the unnamed namespace along these lines: > > ModuleWatcher::OnModuleEventCallback > GetCallbackForLoaderNotificationContext(void* context) { > acquire the lock, compare the instance, and either return an empty callback > or the instance's > } > > and then change this to: > > OnModuleEventCallback callback = > GetCallbackForLoaderNotificationContext(context); > if (callback.is_null()) > return; > > or something along those lines. maybe shrink the name of the fn to avoid > wrapping. :-) Great idea! Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:213: ModuleWatcher::OnModuleEventCallback callback; On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > nit: omit "ModuleWatcher::" Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:21: using OnModuleEventCallback = On 2016/11/10 08:52:13, grt (UTC plus 1) wrote: > please add a doc comment explaining the restrictions on this. for example, if > the loader lock is held when the callback is invoked, the implementer needs to > know what they can and cannot do. in any case, consider documenting that the > implementer shouldn't do anything expensive since the callback is run > synchronously with the actual load event (if that is the case), so it will block > some thread trying to do some work. Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:28: // loader's lock!), and as such may be invoked on any thread in the process. On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > is it under the loader's lock? i thought your discovery in > https://codereview.chromium.org/2473783005/#msg16 was that it isn't. > > in any case, don't assume that the reader knows the implications of "under the > loader's lock". Removed the loader lock discussion entirely, as per my earlier comments. I have the long-term memory of a fruit fly. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:29: // Note that it is possible to receive two modifications for some modules as On 2016/11/10 08:52:13, grt (UTC plus 1) wrote: > modifications -> notifications? Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.h:67: const void* notification_data, On 2016/11/10 08:52:12, grt (UTC plus 1) wrote: > can you forward-decl LDR_DLL_NOTIFICATION_DATA in this file so that you don't > needs the various casts for this? Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win_unittest.cc (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:22: module_event_count_++; On 2016/11/10 08:52:13, grt (UTC plus 1) wrote: > prefer preincrement always Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:44: static const wchar_t kModuleName[] = L"chrome.dll"; On 2016/11/10 08:52:13, grt (UTC plus 1) wrote: > nit: static constexpr Done. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:44: static const wchar_t kModuleName[] = L"chrome.dll"; On 2016/11/10 08:52:13, grt (UTC plus 1) wrote: > is chrome.dll already a datadep of the unit_tests target? if it isn't, is there > something else you can use so that building unit_tests doesn't require waiting > for chrome.dll to link? chrome_elf is at least a whole lot smaller than > chrome.dll. Yeah, it's already a dependency of the unit_tests target. I could definitely use chrome_elf instead. I initially considered building my own test library just for this purpose, but that seems like a bit of overkill. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:92: EXPECT_LT(0u, module_event_count_); On 2016/11/10 08:52:13, grt (UTC plus 1) wrote: > the counts are unsigned, so just check for equality with zero. You're suggested that _NE is more readable that _LT? _LT more precisely states what I mean, and doesn't require the additional contextual knowledge that the count is an unsigned quantity. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:99: EXPECT_EQ(1u, module_loaded_event_count_); On 2016/11/10 08:52:13, grt (UTC plus 1) wrote: > does this assume that the loaded module doesn't also bring in dependencies? if > so, maybe it's better to make your own test binary that you control. I just want to ensure that at least one module gets loaded in response to my LoadLibrary, so it's fine if there are other dependencies. I'll update this statement to reflect that.
lgtm with nits https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win_unittest.cc (right): https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:92: EXPECT_LT(0u, module_event_count_); On 2016/11/14 16:06:45, chrisha (slow) wrote: > On 2016/11/10 08:52:13, grt (UTC plus 1) wrote: > > the counts are unsigned, so just check for equality with zero. > > You're suggested that _NE is more readable that _LT? _LT more precisely states > what I mean, and doesn't require the additional contextual knowledge that the > count is an unsigned quantity. okay. now that i think about it, the counts should all be of type "int" as per https://google.github.io/styleguide/cppguide.html#Integer_Types, making _LT even more better. https://codereview.chromium.org/2473783005/diff/80001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win_unittest.cc:99: EXPECT_EQ(1u, module_loaded_event_count_); On 2016/11/14 16:06:45, chrisha (slow) wrote: > On 2016/11/10 08:52:13, grt (UTC plus 1) wrote: > > does this assume that the loaded module doesn't also bring in dependencies? if > > so, maybe it's better to make your own test binary that you control. > > I just want to ensure that at least one module gets loaded in response to my > LoadLibrary, so it's fine if there are other dependencies. I'll update this > statement to reflect that. great. that addresses my concern -- that in some configurations there are multiple loads. https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:84: base::LazyInstance<base::Lock>::Leaky g_module_watcher_lock = #include "base/synchronization/lock.h" https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:97: std::string ToString(const UNICODE_STRING* str) { #include <string> https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:143: if (!reg_fn) nit: without the return value, i think this reads better as: if (reg_fn) reg_fn(...); but it's personal preference so whatever works for you https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:180: std::string s; move this outside of the loop to save on heap churn https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:200: return ModuleWatcher::OnModuleEventCallback(); omit "ModuleWatcher::" https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.h:26: // Keep the amount of work performed here to an absolute minimum. maybe also mention that the callback may be invoked after the ModuleWatcher is destroyed. it could be surprising to a consumer if they don't expect it. https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.h:43: static std::unique_ptr<ModuleWatcher> Create( #include <memory>
chrisha@chromium.org changed reviewers: + wfh@chromium.org
Thanks grt. sky: care to take a look as owner? wfh: care to look at the new IPC? https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:84: base::LazyInstance<base::Lock>::Leaky g_module_watcher_lock = On 2016/11/14 20:30:31, grt (UTC plus 1) wrote: > #include "base/synchronization/lock.h" Done. https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:97: std::string ToString(const UNICODE_STRING* str) { On 2016/11/14 20:30:31, grt (UTC plus 1) wrote: > #include <string> Done. https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:143: if (!reg_fn) On 2016/11/14 20:30:31, grt (UTC plus 1) wrote: > nit: without the return value, i think this reads better as: > if (reg_fn) > reg_fn(...); > but it's personal preference so whatever works for you Done. https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:180: std::string s; On 2016/11/14 20:30:31, grt (UTC plus 1) wrote: > move this outside of the loop to save on heap churn Done. https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:200: return ModuleWatcher::OnModuleEventCallback(); On 2016/11/14 20:30:31, grt (UTC plus 1) wrote: > omit "ModuleWatcher::" Done. https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.h:26: // Keep the amount of work performed here to an absolute minimum. On 2016/11/14 20:30:31, grt (UTC plus 1) wrote: > maybe also mention that the callback may be invoked after the ModuleWatcher is > destroyed. it could be surprising to a consumer if they don't expect it. Done. https://codereview.chromium.org/2473783005/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.h:43: static std::unique_ptr<ModuleWatcher> Create( On 2016/11/14 20:30:31, grt (UTC plus 1) wrote: > #include <memory> Done.
LGTM https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:87: base::LazyInstance<base::Lock>::Leaky g_module_watcher_lock = These aren't global, so you shouldn't use "g_".
have you tested that the APIs you're using work within the Chrome sandbox? Is it worth having a content_browser test to verify this?
https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:87: base::LazyInstance<base::Lock>::Leaky g_module_watcher_lock = On 2016/11/14 22:14:16, sky wrote: > These aren't global, so you shouldn't use "g_". Hi Scott. I asked Chris to put the g_ there, so I'll share why. These are global to the module even though they can't be seen outside of this compilation unit and they're not local in function scope where they're used. I think of that as being global enough. I checked the Google C++ style guide and Chromium's style guide and found guidance in neither. The Chromium C++ Dos and Donts, on the other hand, shows an example with the leading "g_" exactly like this: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-s.... Are you okay with "g_" here, or would you like to fire up a discussion on chromium-dev to see if there's consensus one way or another from the arbiters of style? Cheers https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.h (right): https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.h:30: // watcher, but very unlikely. what makes it unlikely? is it any less likely than any other race (aren't they all unlikely :-) )? do i misunderstand that any thread loading a module concurrently with destruction will race with the unregistration in the dtor?
I'm LGTMing as is. The discussion of whether 'g_' should be used here can be done in parallel and if necessary update the code after landing. https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:87: base::LazyInstance<base::Lock>::Leaky g_module_watcher_lock = On 2016/11/15 10:35:34, grt (UTC plus 1) wrote: > On 2016/11/14 22:14:16, sky wrote: > > These aren't global, so you shouldn't use "g_". > > Hi Scott. I asked Chris to put the g_ there, so I'll share why. These are global > to the module even though they can't be seen outside of this compilation unit > and they're not local in function scope where they're used. I think of that as > being global enough. I checked the Google C++ style guide and Chromium's style > guide and found guidance in neither. The Chromium C++ Dos and Donts, on the > other hand, shows an example with the leading "g_" exactly like this: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-s.... > Are you okay with "g_" here, or would you like to fire up a discussion on > chromium-dev to see if there's consensus one way or another from the arbiters of > style? > > Cheers This has come up in the past on a number of reviews and the guidance I've received from folks (including Darin), is no g_. I think it's worth a discussion on chromium-dev to reach consensus.
Patchset #9 (id:160001) has been deleted
On 2016/11/14 22:17:17, Will Harris wrote: > have you tested that the APIs you're using work within the Chrome sandbox? Is it > worth having a content_browser test to verify this? I could add a browser test to check this, but AFAIK (and according to creis@) there's no easy way to test a component from inside of the renderer sandbox. I've run local tests and have verified that everything works fine from both inside the renderer and the browser, post sandbox initialization. Would you mind if I defer checking in full tests until I've wired everything up, and I can easily do an integration test?
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:176: MODULEENTRY32 module = {sizeof(module)}; module.dwSize = sizeof(module_entry) before calling ::Module32First https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:196: if (context != g_module_watcher_instance) when does this ever happen? https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:207: if (callback.is_null()) when will this ever be null?
fwiw looking over this again, my gut tells me there should be some kind of weakptr action happening here, with perhaps a post task to the thread that owns the weakptr, why was the locking design chosen instead?
On 2016/11/17 18:19:10, Will Harris wrote: > fwiw looking over this again, my gut tells me there should be some kind of > weakptr action happening here, with perhaps a post task to the thread that owns > the weakptr, why was the locking design chosen instead? You can see the discussion between Greg and I in the review history. The condensed version: - In the renderer this object will have a single client that posts IPC messages. Mojo takes care of thread hopping, etc, so thread hopping twice would be unnecessary complexity. - In the browser process the client of this object will be the ModuleDatabaseImpl. It is going to have to implement thread hopping / weakptrs logic already, and doing this twice is redundant. (If people feel strongly about this I'm not against rewriting to be bound to a task runner, with notifications backed by a weakptr. I just personally find the locking mechanism simpler.) https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:176: MODULEENTRY32 module = {sizeof(module)}; On 2016/11/17 18:12:36, Will Harris wrote: > module.dwSize = sizeof(module_entry) before calling ::Module32First The initializer does exactly that (dwSize is the first field). https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:196: if (context != g_module_watcher_instance) On 2016/11/17 18:12:36, Will Harris wrote: > when does this ever happen? If you tore down the watcher while a ldr notification was pending (and also if you then subsequently created a new one). The context will refer to the old watcher, while the global can be null or refer to a new one. https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:207: if (callback.is_null()) On 2016/11/17 18:12:36, Will Harris wrote: > when will this ever be null? Same story as above. If a pending ldr notification is being processed after the watcher has been destroyed then GetCallbackForContext will return a null callback to indicate that no watcher exists. These two functions make the watcher safe to destruct even though notifications are racing against it.
lgtm % a question for my own education https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:176: MODULEENTRY32 module = {sizeof(module)}; On 2016/11/17 21:40:31, chrisha (slow) wrote: > On 2016/11/17 18:12:36, Will Harris wrote: > > module.dwSize = sizeof(module_entry) before calling ::Module32First > > The initializer does exactly that (dwSize is the first field). Acknowledged. https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:196: if (context != g_module_watcher_instance) On 2016/11/17 21:40:31, chrisha (slow) wrote: > On 2016/11/17 18:12:36, Will Harris wrote: > > when does this ever happen? > > If you tore down the watcher while a ldr notification was pending (and also if > you then subsequently created a new one). The context will refer to the old > watcher, while the global can be null or refer to a new one. Acknowledged. https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:207: if (callback.is_null()) I see "if (callback)" everywhere else this is done, I presume is_null() is the same?
https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:207: if (callback.is_null()) On 2016/11/18 02:46:03, Will Harris wrote: > I see "if (callback)" everywhere else this is done, I presume is_null() is the > same? Awesome! I hadn't noticed the bool operator before. TIL!
Thanks all, committing. https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2473783005/diff/140001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:207: if (callback.is_null()) On 2016/11/18 02:46:03, Will Harris wrote: > I see "if (callback)" everywhere else this is done, I presume is_null() is the > same? Switched to using the bool operator.
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, sky@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2473783005/#ps200001 (title: "Address wfh@ nit.")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, sky@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2473783005/#ps220001 (title: "Remove errant line post rebase.")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== [Win] Create ModuleWatcher. This is a utility class that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 ========== to ========== [Win] Create ModuleWatcher. This is a utility class that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 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 grt@chromium.org, sky@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2473783005/#ps240001 (title: "Fix 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: 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
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, sky@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2473783005/#ps260001 (title: "Add conflits_dll for testing.")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chrisha@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1479925527801640,
"parent_rev": "5826821a3b129d83151b7e8456892f7e5fe2d565", "commit_rev":
"33fa746be3bb5c83b37811b8b206436481364257"}
Message was sent while issue was closed.
Description was changed from ========== [Win] Create ModuleWatcher. This is a utility class that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [Win] Create ModuleWatcher. This is a utility class that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [Win] Create ModuleWatcher. This is a utility class that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [Win] Create ModuleWatcher. This is a utility class that will be instantiated in each process type. The resulting events will be funneled back to the browser process for use in third-party metrics gathering and the chrome://conflicts UI. BUG=662084 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/9d5f46c81db254725e6b8215937097a88b2f558c Cr-Commit-Position: refs/heads/master@{#434251} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/9d5f46c81db254725e6b8215937097a88b2f558c Cr-Commit-Position: refs/heads/master@{#434251} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
