[win] Enable ModuleDatabase behind a flag.

Enable ModuleDatabase behind a flag.

This plumbs together the various pieces of the ModuleDatabase, enabling it in an observation mode behind a feature flag. Further CLs will port over the various bits of module inspection logic from EnumerateModulesModel, and finally switch chrome://conflicts to source its data from here.

BUG=662084
Review-Url: https://codereview.chromium.org/2613803005
Cr-Commit-Position: refs/heads/master@{#444534}
Committed: https://chromium.googlesource.com/chromium/src/+/927d8fea13c6aa514afb44e8273ae5f85225cb64

[chrisha, 2017-01-05 15:52:06]

Description was changed from

==========
Enable ModuleDatabase behind a flag.

This plumbs together the various pieces of the ModuleDatabase, enabling it
in an observation mode behind a feature flag. Further CLs will port over the
various bits of module inspection logic from EnumerateModulesModel, and
finally switch chrome://conflicts to source its data from here.

BUG=662084
==========

to

==========
Enable ModuleDatabase behind a flag.

This plumbs together the various pieces of the ModuleDatabase, enabling it in an
observation mode behind a feature flag. Further CLs will port over the various
bits of module inspection logic from EnumerateModulesModel, and finally switch
chrome://conflicts to source its data from here.

BUG=662084
==========

[chrisha, 2017-01-05 15:52:38]

grt, wfh: this is the next step of https://codereview.chromium.org/2576843002/

[chrisha, 2017-01-05 15:57:44]

chrisha@chromium.org changed reviewers:
+ grt@chromium.org, wfh@chromium.org

[chrisha, 2017-01-05 15:57:44]

This is the next step of https://codereview.chromium.org/2576843002/. Feel free
to take a look.

[grt (UTC plus 2), 2017-01-06 09:44:58]

looks pretty good. this is some neat stuff.

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:151:
base::FeatureList::IsEnabled(features::kModuleDatabase);
apologies if this is a silly question, but why isn't the call in
ChromeBrowserMainPartsWin::PostProfileInit sufficient? i've never needed to do
anything like this here before. have i been doing it wrong?

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:212: auto* base =
reinterpret_cast<const uint8_t*>(module_load_address);
can you use base::win::PEImage for this?

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:372: if
(base::FeatureList::IsEnabled(features::kModuleDatabase)) {
nit: pull this out into a function in the unnamed namespace just before
GetModuleTimeDateStamp so that it's next to the related code. it's then a bit
easier to see why it makes sense for the call to OnProcessStarted be here rather
than hidden in ModuleDatabase

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:373: uint64_t creation_time = 0;
#include <stdint.h>

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:374:
ModuleEventSinkImpl::GetProcessCreationTime(::GetCurrentProcess(),
TIL: base::CurrentProcessInfo::CreationTime()!

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:2988: auto get_process =
base::Bind(&GetProcessHandleFromRenderer,
can you cut out the middle-man with:
  base::Bind(&content::RenderProcessHost::GetHandle,
base::Unretained(render_process_host))?

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:2991:
base::Bind(&ModuleEventSinkImpl::Create, get_process,
nit: pass ownership of get_process to the bound callback via
std::move(get_process). (i think :-) )

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_database_win.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_database_win.cc:40: // This is deliberately
leaked.
  DCHECK_EQ(nullptr, g_instance);

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_database_win.cc:41: g_instance =
module_database.release();
wdyt of adding a dtor with something like DCHECK_NE(g_instance, this);? or have
the dtor clear g_instance if it equals |this|?

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_database_win.h (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_database_win.h:34: // Sets the global instance
of the ModuleDatabase.
nit: blank line before comment

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_event_sink_impl_win.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_event_sink_impl_win.cc:125: base::ProcessHandle
process = get_process_handle.Run();
std::move(get_process_handle).Run() to "consume" the callback?

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_event_sink_impl_win.h (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_event_sink_impl_win.h:10: #include
"base/callback.h"
will callback_forward.h work here?

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_event_sink_impl_win.h:38: static void
Create(const GetProcessHandleCallback& get_process_handle,
should this be passed by value as per
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:298: // Dispatches a module
|event| to the provided |module_database| interface.
nit: module_database -> module_event_sink?

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:299: void
OnModuleEventImpl(mojom::ModuleEventSinkPtr* module_event_sink,
this will trigger a UAF if a ChromeContentRendererClient can be destroyed after
OnModuleEvent is called but before the IO thread picks up OnModuleEventImpl, no
(mojom::ModuleEventSinkPtr will have been destroyed)? is this possible?

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:301: // Simply the send the
module load address. The browser can validate this and
nix extra " the"

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:317:
base::Bind(&OnModuleEventImpl,
nit: OnModuleEventImpl -> HandleModuleEventOnIOThread?

[chrisha, 2017-01-10 21:01:46]

Thanks for the review!

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:151:
base::FeatureList::IsEnabled(features::kModuleDatabase);
On 2017/01/06 09:44:57, grt (UTC plus 1) wrote:
> apologies if this is a silly question, but why isn't the call in
> ChromeBrowserMainPartsWin::PostProfileInit sufficient? i've never needed to do
> anything like this here before. have i been doing it wrong?

AFAIK it's not a big deal, it's just that the earlier you "initialize" them the
higher the likelihood of them showing up in the system profile with crash
reports. I was just copying the code below with kDesktopFastShutdown. I'm not
sure if this is good practice or not...

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:212: auto* base =
reinterpret_cast<const uint8_t*>(module_load_address);
On 2017/01/06 09:44:57, grt (UTC plus 1) wrote:
> can you use base::win::PEImage for this?

Yeah, should be fine. I forget that thing exists.

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:372: if
(base::FeatureList::IsEnabled(features::kModuleDatabase)) {
On 2017/01/06 09:44:57, grt (UTC plus 1) wrote:
> nit: pull this out into a function in the unnamed namespace just before
> GetModuleTimeDateStamp so that it's next to the related code. it's then a bit
> easier to see why it makes sense for the call to OnProcessStarted be here
rather
> than hidden in ModuleDatabase

Done.

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:373: uint64_t creation_time = 0;
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> #include <stdint.h>

Done.

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:374:
ModuleEventSinkImpl::GetProcessCreationTime(::GetCurrentProcess(),
On 2017/01/06 09:44:57, grt (UTC plus 1) wrote:
> TIL: base::CurrentProcessInfo::CreationTime()!

Is there any guarantee that base::Time is lossless wrt FILETIME, and will always
be? The intent of the creation time here is to use it as a key to uniquely
identify a process. If base::Time is at all "lossy" wrt FILETIME then that will
no longer work.

(Looking at the implementation, it's not. FILETIME is in 100 ns increments, but
base::Time uses microseconds. So there's a miniature possibility of PID reuse
within the window of a base::Time-backed creation time.)

https://cs.chromium.org/chromium/src/base/time/time_win.cc?q=FileTimeToMicros...

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:2988: auto get_process =
base::Bind(&GetProcessHandleFromRenderer,
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> can you cut out the middle-man with:
>   base::Bind(&content::RenderProcessHost::GetHandle,
> base::Unretained(render_process_host))?

Indeed. /facepalm

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:2991:
base::Bind(&ModuleEventSinkImpl::Create, get_process,
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> nit: pass ownership of get_process to the bound callback via
> std::move(get_process). (i think :-) )

Done.

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_database_win.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_database_win.cc:40: // This is deliberately
leaked.
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
>   DCHECK_EQ(nullptr, g_instance);

Done.

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_database_win.cc:41: g_instance =
module_database.release();
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> wdyt of adding a dtor with something like DCHECK_NE(g_instance, this);? or
have
> the dtor clear g_instance if it equals |this|?

Sure, I'm fine with that.

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_database_win.h (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_database_win.h:34: // Sets the global instance
of the ModuleDatabase.
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> nit: blank line before comment

Done.

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_event_sink_impl_win.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_event_sink_impl_win.cc:125: base::ProcessHandle
process = get_process_handle.Run();
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> std::move(get_process_handle).Run() to "consume" the callback?

I'm fine with that, but have never seen it. Is this a common pattern?

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_event_sink_impl_win.h (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_event_sink_impl_win.h:10: #include
"base/callback.h"
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> will callback_forward.h work here?

Indeed it will!

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_event_sink_impl_win.h:38: static void
Create(const GetProcessHandleCallback& get_process_handle,
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> should this be passed by value as per
>
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...

Yup... old habits die hard.

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:298: // Dispatches a module
|event| to the provided |module_database| interface.
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> nit: module_database -> module_event_sink?

Done.

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:299: void
OnModuleEventImpl(mojom::ModuleEventSinkPtr* module_event_sink,
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> this will trigger a UAF if a ChromeContentRendererClient can be destroyed
after
> OnModuleEvent is called but before the IO thread picks up OnModuleEventImpl,
no
> (mojom::ModuleEventSinkPtr will have been destroyed)? is this possible?

Great catch. ChromeContentRendererClient is a lazy initialized singleton that is
leaked, so this is not an issue. It lives here:

https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?q=Chr...

I'll add comments that make this explicit.

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:301: // Simply the send the
module load address. The browser can validate this and
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> nix extra " the"

Done.

https://codereview.chromium.org/2613803005/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:317:
base::Bind(&OnModuleEventImpl,
On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> nit: OnModuleEventImpl -> HandleModuleEventOnIOThread?

Done.

[Will Harris, 2017-01-10 23:58:40]

all this base::Unretained is giving me the "heebie jeebies" but I'm assuming
grt@ has looked over this and the lifetimes are fine, especially as you seem to
be leaking the object anyway. Another good person to spot subtle issues with
this is often dcheng@ if you want to add him

I'm happy with the Windows code, in fact as this is mostly plumbing anyway, from
what I can tell :)

[grt (UTC plus 2), 2017-01-11 15:07:54]

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_event_sink_impl_win.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_event_sink_impl_win.cc:125: base::ProcessHandle
process = get_process_handle.Run();
On 2017/01/10 21:01:46, chrisha (slow) wrote:
> On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> > std::move(get_process_handle).Run() to "consume" the callback?
> 
> I'm fine with that, but have never seen it. Is this a common pattern?

I found it in
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md. I had
second thoughts about this comment after I sent it. I think it's for
OnceCallback, which you're not using, so maybe it's not good style.

https://codereview.chromium.org/2613803005/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2613803005/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2979:
base::Unretained(render_process_host));
is it guaranteed that |render_process_host| will still be alive when this
callback is eventually run? if yes, please document. thanks.

[chrisha, 2017-01-11 21:14:11]

Thanks wfh and grt. I've added verbose comments for each use of
"base::Unretained" to hopefully calm any fears.

sky: This is the next step of https://codereview.chromium.org/2576843002/. Since
you have context, care to take a look?

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_event_sink_impl_win.cc (right):

https://codereview.chromium.org/2613803005/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_event_sink_impl_win.cc:125: base::ProcessHandle
process = get_process_handle.Run();
On 2017/01/11 15:07:54, grt (UTC plus 1) wrote:
> On 2017/01/10 21:01:46, chrisha (slow) wrote:
> > On 2017/01/06 09:44:58, grt (UTC plus 1) wrote:
> > > std::move(get_process_handle).Run() to "consume" the callback?
> > 
> > I'm fine with that, but have never seen it. Is this a common pattern?
> 
> I found it in
> https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md. I
had
> second thoughts about this comment after I sent it. I think it's for
> OnceCallback, which you're not using, so maybe it's not good style.

Acknowledged.

https://codereview.chromium.org/2613803005/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2613803005/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2979:
base::Unretained(render_process_host));
On 2017/01/11 15:07:54, grt (UTC plus 1) wrote:
> is it guaranteed that |render_process_host| will still be alive when this
> callback is eventually run? if yes, please document. thanks.

Yes, it's guaranteed as "GetInstance" is called in the context of initializing
IPC for a render process host. Documented.

[grt (UTC plus 2), 2017-01-12 10:50:11]

lgtm

[chrisha, 2017-01-13 20:46:05]

chrisha@chromium.org changed reviewers:
+ sky@chromium.org

[chrisha, 2017-01-13 20:46:05]

sky: Care to take a look at this, given that you have context?

[Will Harris, 2017-01-13 22:33:21]

token lgtm; I don't think there's anything here I particularly need to look at
since most of the win code is in the other CL

[sky, 2017-01-13 23:58:43]

LGTM

https://codereview.chromium.org/2613803005/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_win.h (right):

https://codereview.chromium.org/2613803005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_win.h:13: #include
"chrome/common/conflicts/module_watcher_win.h"
Can you forward declare ModuleWatcher?

[chrisha, 2017-01-18 22:20:02]

Thanks all.

https://codereview.chromium.org/2613803005/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_win.h (right):

https://codereview.chromium.org/2613803005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_win.h:13: #include
"chrome/common/conflicts/module_watcher_win.h"
On 2017/01/13 23:58:43, sky wrote:
> Can you forward declare ModuleWatcher?

Done.

[chrisha, 2017-01-18 22:20:11]

The CQ bit was checked by chrisha@chromium.org

[chrisha, 2017-01-18 22:20:11]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org,
sky@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2613803005/#ps70001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-01-18 22:20:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 23:09:54]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1484778011224410,
"parent_rev": "8bec6b2ef1c8d45c613aa0aca47f1ae15bd05281", "commit_rev":
"927d8fea13c6aa514afb44e8273ae5f85225cb64"}

[commit-bot: I haz the power, 2017-01-18 23:10:26]

Description was changed from

==========
Enable ModuleDatabase behind a flag.

This plumbs together the various pieces of the ModuleDatabase, enabling it in an
observation mode behind a feature flag. Further CLs will port over the various
bits of module inspection logic from EnumerateModulesModel, and finally switch
chrome://conflicts to source its data from here.

BUG=662084
==========

to

==========
Enable ModuleDatabase behind a flag.

This plumbs together the various pieces of the ModuleDatabase, enabling it in an
observation mode behind a feature flag. Further CLs will port over the various
bits of module inspection logic from EnumerateModulesModel, and finally switch
chrome://conflicts to source its data from here.

BUG=662084

Review-Url: https://codereview.chromium.org/2613803005
Cr-Commit-Position: refs/heads/master@{#444534}
Committed:
https://chromium.googlesource.com/chromium/src/+/927d8fea13c6aa514afb44e8273a...
==========

[commit-bot: I haz the power, 2017-01-18 23:10:28]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/927d8fea13c6aa514afb44e8273a...