Add InspectModule() that returns a populated ModuleInspectionResult struct

Add InspectModule() that returns a populated ModuleInspectionResult struct

BUG=690173
Review-Url: https://codereview.chromium.org/2720513005
Cr-Commit-Position: refs/heads/master@{#454986}
Committed: https://chromium.googlesource.com/chromium/src/+/61e93409b7a95da223db45f8abf4d9eaa8dd52c6

[Patrick Monette, 2017-02-27 20:02:22]

Patchset #1 (id:1) has been deleted

[Patrick Monette, 2017-02-27 20:02:45]

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

[Patrick Monette, 2017-02-27 20:02:46]

PTAL.

[chrisha, 2017-02-28 18:51:29]

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.cc (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.cc:269: const base::string16&
prefix = base::i18n::ToLower(mapping.first);
Could we just document that we expect StringMapping to contain lowercase strings
already? (GetEnvironmentVariablesMapping guarantees that for you, no?)

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.cc:270: if
(base::StartsWith(lower_string, prefix, base::CompareCase::SENSITIVE)) {
Do we have expectations that the paths in |prefix_mapping| have trailing
slashes? Should we? Can you document this in the .h files?

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.cc:275: if (length < min_length) {
We're only doing the collapsing if it's shorter? We should always be doing this
as we're trying to "normalize" paths to remove the effects of localization.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.h (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.h:56: // values in the mapping to
ensure the best fit is chosen.
Not the best wording? Maybe an example as well?

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win_unittest.cc (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win_unittest.cc:83: // The mapping for
these variables will be retrived.
retrieved

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win_unittest.cc:103: // These two are
to test that we select the maximum collapsed path.
I don't think we actually want this logic.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_win.cc (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.cc:20: void
PopulateModuleInfoData(const ModuleInfoKey& key,
A comment would be good here.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.cc:51: void
NormalizeModuleInfoData(ModuleInfoData* module_data) {
You probably want to expose and unittest this functions?

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_win.h (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.h:56: void CopyInspectionData(const
ModuleInfoData& other);
How about just a plain old operator=? Or pass a ModuleInfoData object into
InspectModule for it to use for output?

[Patrick Monette, 2017-02-28 23:37:20]

Patchset #2 (id:40001) has been deleted

[Patrick Monette, 2017-02-28 23:37:38]

Thanks! Please take another look.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.cc (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.cc:269: const base::string16&
prefix = base::i18n::ToLower(mapping.first);
On 2017/02/28 18:51:28, chrisha wrote:
> Could we just document that we expect StringMapping to contain lowercase
strings
> already? (GetEnvironmentVariablesMapping guarantees that for you, no?)

Indeed. Done.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.cc:270: if
(base::StartsWith(lower_string, prefix, base::CompareCase::SENSITIVE)) {
On 2017/02/28 18:51:29, chrisha wrote:
> Do we have expectations that the paths in |prefix_mapping| have trailing
> slashes? Should we? Can you document this in the .h files?

I don't think this function needs to expect paths with or without trailing
slashes. I made it so that GetEnvironmentVariablesMapping() always return
without trailing slashes so it'll be consistent.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.cc:275: if (length < min_length) {
On 2017/02/28 18:51:28, chrisha wrote:
> We're only doing the collapsing if it's shorter? We should always be doing
this
> as we're trying to "normalize" paths to remove the effects of localization.

Chatted offline. Ignored.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.h (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.h:56: // values in the mapping to
ensure the best fit is chosen.
On 2017/02/28 18:51:29, chrisha wrote:
> Not the best wording? Maybe an example as well?

I've reworded this and I think an example is not needed anymore. Let me know if
you disagree.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win_unittest.cc (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win_unittest.cc:83: // The mapping for
these variables will be retrived.
On 2017/02/28 18:51:29, chrisha wrote:
> retrieved

Done.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win_unittest.cc:103: // These two are
to test that we select the maximum collapsed path.
On 2017/02/28 18:51:29, chrisha wrote:
> I don't think we actually want this logic.

Chatted offline. Ignored.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_win.cc (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.cc:20: void
PopulateModuleInfoData(const ModuleInfoKey& key,
On 2017/02/28 18:51:29, chrisha wrote:
> A comment would be good here.

Done.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.cc:51: void
NormalizeModuleInfoData(ModuleInfoData* module_data) {
On 2017/02/28 18:51:29, chrisha wrote:
> You probably want to expose and unittest this functions?

Done.

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_win.h (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.h:56: void CopyInspectionData(const
ModuleInfoData& other);
On 2017/02/28 18:51:29, chrisha wrote:
> How about just a plain old operator=? Or pass a ModuleInfoData object into
> InspectModule for it to use for output?

I definitely don't like my current approach. Let me know if you think of
something better. One idea I had was to move all inspection data to a new
struct, and only that part of the ModuleInfoData would need to be copied.

The problem is that some of ModuleInfoData's member should not just be blindly
copied.
For example, process_types: When the module is first seen, process_types will
only have one bit set. By the time it gets inspected on the background thread,
we may receive a new notification that the module was loaded into another
process. When we get the ModuleInfoData back from the inspection, they have to
be OR'ed together.

I could change the semantic of the assign operator but I feel like it would be a
surprising behavior to most people. Hence the named function.

[chrisha, 2017-03-01 15:45:43]

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_win.h (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.h:56: void CopyInspectionData(const
ModuleInfoData& other);
On 2017/02/28 23:37:38, Patrick Monette wrote:
> On 2017/02/28 18:51:29, chrisha wrote:
> > How about just a plain old operator=? Or pass a ModuleInfoData object into
> > InspectModule for it to use for output?
> 
> I definitely don't like my current approach. Let me know if you think of
> something better. One idea I had was to move all inspection data to a new
> struct, and only that part of the ModuleInfoData would need to be copied.
> 
> The problem is that some of ModuleInfoData's member should not just be blindly
> copied.
> For example, process_types: When the module is first seen, process_types will
> only have one bit set. By the time it gets inspected on the background thread,
> we may receive a new notification that the module was loaded into another
> process. When we get the ModuleInfoData back from the inspection, they have to
> be OR'ed together.
> 
> I could change the semantic of the assign operator but I feel like it would be
a
> surprising behavior to most people. Hence the named function.

Yeah, having a separate section of ModuleInspectionResults, or something like
that might be cleaner. Then simply use the assignment operator for that when the
results are ready.

You could also consider have ModuleInfoData have a std::unique_ptr to the
inspection results, which remains null until they've been populated?

Agreed that the operator= is a bad idea in this case.

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.cc (right):

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.cc:278: size_t length =
collapsed_string.length() - mapping.second.length();
Shouldn't we have logic to ensure the next character after the match is a slash?
Otherwise we could end up with "unnatural" substitutions:

c:\windows\system32_some_dir_i_made_up\foo.exe

would end up as:

%systemroot%_some_dir_i_made_up\foo.exe

Which maybe we don't want?

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.h (right):

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.h:59: // strings.
Can you document expectations about trailing slashes in |prefix_mapping| as
well?

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win_unittest.cc (right):

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win_unittest.cc:19: class
ScopedEnvironmentVariableOverride {
Document this class very briefly.

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win_unittest.cc:112: };
Add a test to make sure prefix matches not followed by a slash aren't actually
matched?

[Patrick Monette, 2017-03-02 20:27:07]

Take another look please!

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_win.h (right):

https://codereview.chromium.org/2720513005/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.h:56: void CopyInspectionData(const
ModuleInfoData& other);
On 2017/03/01 15:45:42, chrisha wrote:
> On 2017/02/28 23:37:38, Patrick Monette wrote:
> > On 2017/02/28 18:51:29, chrisha wrote:
> > > How about just a plain old operator=? Or pass a ModuleInfoData object into
> > > InspectModule for it to use for output?
> > 
> > I definitely don't like my current approach. Let me know if you think of
> > something better. One idea I had was to move all inspection data to a new
> > struct, and only that part of the ModuleInfoData would need to be copied.
> > 
> > The problem is that some of ModuleInfoData's member should not just be
blindly
> > copied.
> > For example, process_types: When the module is first seen, process_types
will
> > only have one bit set. By the time it gets inspected on the background
thread,
> > we may receive a new notification that the module was loaded into another
> > process. When we get the ModuleInfoData back from the inspection, they have
to
> > be OR'ed together.
> > 
> > I could change the semantic of the assign operator but I feel like it would
be
> a
> > surprising behavior to most people. Hence the named function.
> 
> Yeah, having a separate section of ModuleInspectionResults, or something like
> that might be cleaner. Then simply use the assignment operator for that when
the
> results are ready.
> 
> You could also consider have ModuleInfoData have a std::unique_ptr to the
> inspection results, which remains null until they've been populated?
> 
> Agreed that the operator= is a bad idea in this case.

Done.

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.cc (right):

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.cc:278: size_t length =
collapsed_string.length() - mapping.second.length();
On 2017/03/01 15:45:42, chrisha wrote:
> Shouldn't we have logic to ensure the next character after the match is a
slash?
> Otherwise we could end up with "unnatural" substitutions:
> 
> c:\windows\system32_some_dir_i_made_up\foo.exe
> 
> would end up as:
> 
> %systemroot%_some_dir_i_made_up\foo.exe
> 
> Which maybe we don't want?

Done.

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.h (right):

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.h:59: // strings.
On 2017/03/01 15:45:43, chrisha wrote:
> Can you document expectations about trailing slashes in |prefix_mapping| as
> well?

Done.

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win_unittest.cc (right):

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win_unittest.cc:19: class
ScopedEnvironmentVariableOverride {
On 2017/03/01 15:45:43, chrisha wrote:
> Document this class very briefly.

Done.

https://codereview.chromium.org/2720513005/diff/60001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win_unittest.cc:112: };
On 2017/03/01 15:45:43, chrisha wrote:
> Add a test to make sure prefix matches not followed by a slash aren't actually
> matched?

Done.

[chrisha, 2017-03-04 15:54:17]

lgtm!

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.h (right):

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.h:59: // strings. Also,
|prefix_mapping| must not contain any trailling backslashes.
trailing*

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_win.h (right):

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.h:49: // information is expensive to
gather and require disk access, it should be
requires*

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.h:103: // particular,
particular:

[Patrick Monette, 2017-03-06 16:27:27]

Description was changed from

==========
Add InspectModule() to populate a ModuleInfoData struct from on disk file

BUG=690173
==========

to

==========
Add InspectModule() that returns a populated ModuleInspectionResult struct

BUG=690173
==========

[Patrick Monette, 2017-03-06 16:39:48]

Thanks!

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_util_win.h (right):

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_util_win.h:59: // strings. Also,
|prefix_mapping| must not contain any trailling backslashes.
On 2017/03/04 15:54:16, chrisha wrote:
> trailing*

Done.

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_win.h (right):

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.h:49: // information is expensive to
gather and require disk access, it should be
On 2017/03/04 15:54:17, chrisha wrote:
> requires*

Done.

https://codereview.chromium.org/2720513005/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.h:103: // particular,
On 2017/03/04 15:54:17, chrisha wrote:
> particular:

Done.

[Patrick Monette, 2017-03-06 16:39:51]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2017-03-06 16:39:51]

The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2720513005/#ps100001
(title: "Last nits")

[commit-bot: I haz the power, 2017-03-06 16:40:08]

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-03-06 18:22:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 18:22:24]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Patrick Monette, 2017-03-06 18:33:03]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2017-03-06 18:33:03]

The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2720513005/#ps120001
(title: "Unsigned vs signed comparison")

[commit-bot: I haz the power, 2017-03-06 18:33:46]

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-03-06 20:21:26]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Patrick Monette, 2017-03-06 20:22:20]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2017-03-06 20:22:21]

The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2720513005/#ps140001
(title: "Another")

[commit-bot: I haz the power, 2017-03-06 20:23:15]

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-03-06 21:30:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 21:30:58]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Patrick Monette, 2017-03-06 21:33:21]

The CQ bit was checked by pmonette@chromium.org

[commit-bot: I haz the power, 2017-03-06 21:33:47]

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-03-06 22:48:53]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488836001255750,
"parent_rev": "4373e6482a70511b1598f3f4de0170f357c06ae1", "commit_rev":
"61e93409b7a95da223db45f8abf4d9eaa8dd52c6"}

[commit-bot: I haz the power, 2017-03-06 22:49:35]

Description was changed from

==========
Add InspectModule() that returns a populated ModuleInspectionResult struct

BUG=690173
==========

to

==========
Add InspectModule() that returns a populated ModuleInspectionResult struct

BUG=690173

Review-Url: https://codereview.chromium.org/2720513005
Cr-Commit-Position: refs/heads/master@{#454986}
Committed:
https://chromium.googlesource.com/chromium/src/+/61e93409b7a95da223db45f8abf4...
==========

[commit-bot: I haz the power, 2017-03-06 22:49:36]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/61e93409b7a95da223db45f8abf4...