Add InspectModule() that returns a populated ModuleInspectionResult struct

chrome/browser/conflicts/module_info_win.h

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_CONFLICTS_MODULE_INFO_WIN_H_
 #define CHROME_BROWSER_CONFLICTS_MODULE_INFO_WIN_H_
 
 #include "base/files/file_path.h"
 #include "chrome/browser/conflicts/module_info_util_win.h"
 
@@ skipping 33 matching lines @@
 };
 
 // Holds more detailed information about a given module. Because most of this
 // information is expensive to gather and require disk access, it should be
 // collected on a task runner that allow blocking.
 struct ModuleInfoData {
   ModuleInfoData();
   ModuleInfoData(const ModuleInfoData& others);
   ~ModuleInfoData();
 
+  // Used to copy a ModuleInfoData returned by InspectModule() into an empty,
+  // existing one.
+  void CopyInspectionData(const ModuleInfoData& other);

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

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:38]

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:42]

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.

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

Done.

+
   // Set of all process types in which this module has been seen (may not be
   // currently present in a process of that type). This is a conversion of
   // ProcessType enumeration to a bitfield. See "ProcessTypeToBit" and
   // "BitIndexToProcessType" for details.
   uint32_t process_types;
 
   // The following pieces of information are determined via a detailed
   // inspection of the module.
 
+  // Indicates if the module has been inspected.
+  bool inspected;
+
   // The module path, not including the basename. This is cleaned and normalized
   // so that common paths are converted to their environment variable mappings
   // (ie, %systemroot%). This makes i18n localized paths easily comparable.
   base::string16 location;
 
   // The basename of the module.
   base::string16 basename;
 
   // The name of the product the module belongs to.
   base::string16 product_name;
 
   // The module file description.
   base::string16 description;
 
   // The module version. This is usually in the form a.b.c.d (where a, b, c and
   // d are integers), but may also have fewer than 4 components.
   base::string16 version;
 
   // The certificate info for the module.
   CertificateInfo certificate_info;
 };
 
+// Given a module identified by |module_key|, returns a populated ModuleInfoData
+// that contains detailed information about the module on disk. This is a
+// blocking task that requires access to disk.
+ModuleInfoData InspectModule(const StringMapping& env_variable_mapping,
+                             const ModuleInfoKey& module_key);
+
 #endif  // CHROME_BROWSER_CONFLICTS_MODULE_INFO_WIN_H_