Adding an API to read module annotations in snapshot.gyp

Adding an API to read module annotations in snapshot.gyp

Kasko needs a way to read crash keys from out of process. This API reuses the functionality of PEImageAnnotationsReader.

BUG=chromium:485149

[Patrick Monette, 2016-01-12 22:39:59]

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

[Patrick Monette, 2016-01-12 22:40:00]

PTAL. Does this look reasonable?

[Mark Mentovai, 2016-01-12 22:41:46]

Patrick Monette wrote:
> PTAL. Does this look reasonable?

Depends. What did we leave exposed from snapshot that this was needed for?

[Patrick Monette, 2016-01-12 22:45:20]

Description was changed from

==========
Make external projects able to include snapshot.gyp
==========

to

==========
Make external projects able to include snapshot.gyp

Kasko needs a way to read crash keys from out of process.
It will be able to reuse PEImageAnnotationsReader to do so.
==========

[Patrick Monette, 2016-01-12 22:47:01]

The goal is to reuse PEImageAnnotationsReader. Create a function
GetAnnotationsForProcess(HANDLE) in components/crash/content/app/crashpad.h in
Chrome that can be used in the Chrome Watcher.

Added context to the bug. I had only discussed this with Scott.

[Mark Mentovai, 2016-01-12 22:48:52]

mark@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2016-01-12 22:48:53]

Can you cite the bug (BUG=project:number)?

[Patrick Monette, 2016-01-12 22:52:54]

Description was changed from

==========
Make external projects able to include snapshot.gyp

Kasko needs a way to read crash keys from out of process.
It will be able to reuse PEImageAnnotationsReader to do so.
==========

to

==========
Make external projects able to include snapshot.gyp

Kasko needs a way to read crash keys from out of process.
It will be able to reuse PEImageAnnotationsReader to do so.

BUG=chromium:485149
==========

[Patrick Monette, 2016-01-12 22:54:32]

Done. Not much context in the bug. Most of the discussion took place through
email.

[Mark Mentovai, 2016-01-12 23:00:51]

Chromium bug 485149 is about getting the client ID. You won’t find the crash
client ID via anything that PEImageAnnotationsReader can do now in the future,
because the crash client ID isn’t stored that way. It’s something that
crashpad_handler “pushes in” on its own. Process annotations don’t actually live
in the process or in any of its modules, they’re called “process annotations”
because they’re conceptually process-scoped (as opposed to module-scoped) and
they occupy the process slots in the minidump’s crashpad info stream.

You can get the UMA client ID in this way because it’s stored as a module
annotation, but I don’t think that this is what you want.

[scottmg, 2016-01-12 23:01:37]

Rather than exporting "all" of snapshot, I wonder if we should have something
like client that exports the re-usable functionality we want to support as an
API.

[Patrick Monette, 2016-01-12 23:08:47]

The values that are read by the annotations reader are still very useful.

But we definitely need to find a way to get the client id. Do you have any
suggestion on how to do this?

scottmg: Maybe a new target in snapshot.gyp named 'snapshot_client'. Should the
API files exist in another folder?

[Mark Mentovai, 2016-01-12 23:16:32]

You can get the client ID out of the database (provided you know what database
is in use) by using CrashReportDatabase::GetSettings() and then
Setting::GetClientID(). Both of these are in client.

[Patrick Monette, 2016-01-12 23:24:25]

I'll take a look at this.

About this CL, the bug specifically mention the client id but in the description
it also refers to other crash keys. I think that's because with breakpad, they
were all in the same location.

Would you prefer to split the bug in 2? Or is this fine?

[Mark Mentovai, 2016-01-12 23:28:50]

Scott knows more about what’s currently exposed for Kasko’s benefit, so I’ll
throw the mike back over to him.

[scottmg, 2016-01-13 18:28:42]

Ah, I didn't realize getting the client ID was the primary goal here.

We don't have any visibility controls in gyp, but in my mind snapshot is an
internal detail, so I'm a little uncomfortable with Crashpad users digging into
snapshot, as far as having to maintain all of that as an API surface area.

So I think having a separate target, with the minimal exposed functionality of
reading annotations from a process returned as a map would make that a bit
clearer and easier to evolve snapshot in the future.

I don't have a great place in mind for it. I guess snapshot/api, with an
associated crashpad_snapshot_api target is OK for now.

(It's a bit awkward that it can't just go in client. snapshot depends on client,
I think mostly for CrashpadInfo, so client can't use snapshot for that utility
function.)

[Patrick Monette, 2016-01-13 18:35:28]

Patchset #2 (id:20001) has been deleted

[Patrick Monette, 2016-01-13 18:36:18]

Patchset #2 (id:40001) has been deleted

[Patrick Monette, 2016-01-13 18:37:52]

scott PTAL

[Patrick Monette, 2016-01-13 18:51:17]

I just realized that we are actually already fine for the guid. This value is
known very early so calling GetCrashKeyForKasko in kasko_client.cc includes it.

The ugly side of it is we'd need to keep 2 different mechanisms to read inside
the hanged process. The crashpad one with PEImageAnnotationsReader that doesn't
include the guid, and the kasko one that gets a very early version of the crash
keys that does include the guid.

[scottmg, 2016-01-13 19:12:42]

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
File snapshot/client/crashpad_annotations_win.cc (right):

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:29: if
(!process_reader.Initialize(process, ProcessSuspensionState::kRunning))
Would it be a good idea to suspend the process before reading here? See
ScopedProcessSuspend.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:32: MODULEINFO module_info = {};
I don't think this needs to be initialized, as it's documented as [out] for
GetModuleInformation().

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:33: if (!::GetModuleInformation(
No :: prefix in Crashpad unless it's necessary to disambiguate.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:33: if (!::GetModuleInformation(
I believe this needs the GET_FUNCTION_REQUIRED dance that
snapshot/win/pe_image_reader_test.cc does in order to work correctly on
pre-Win7.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:35: return false;
PLOG(ERROR) << "GetModuleInformation" here.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
File snapshot/client/crashpad_annotations_win.h (right):

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:15: #ifndef
CRASHPAD_SNAPSHOT_SNAPSHOT_CLIENT_CRASHPAD_ANNOTATIONS_WIN_H_
There's an extra SNAPSHOT_ in the guard here.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:24: // Reads the crashpad annotations
associated to |module| inside |process| and
These need to be Doxygen-ized in Crashpad. Comments start with //!, followed by
a \brief summary. Instead of |arg|, use \a arg, and add \param[in] and
\param[out] documentation for the arguments.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:24: // Reads the crashpad annotations
associated to |module| inside |process| and
crashpad -> Crashpad. Or maybe instead, since we know they're Crashpad
annotations when we're in Crashpad, it should be module annotations. The file
name and function name should be changed to match.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:29: HMODULE module,
Is it going to be tricky for a user to provide HMODULE?

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:30: std::map<std::string,
std::string>* annotations);
#include <string>

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp
File snapshot/snapshot.gyp (right):

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp#n...
snapshot/snapshot.gyp:131: 'target_name': 'snapshot_client',
This should probably be 'crashpad_' prefixed for consistency.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp#n...
snapshot/snapshot.gyp:131: 'target_name': 'snapshot_client',
I think 'client' is a bit confusing because it won't necessarily be in a client
(in the sense that it's registering with the handler).

Maybe 'api'? Or maybe Mark has a better suggestion.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp#n...
snapshot/snapshot.gyp:142: 'client/crashpad_annotations_win.cc',
api/ here too.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp#n...
snapshot/snapshot.gyp:142: 'client/crashpad_annotations_win.cc',
(This will be an empty target on non-Windows, not sure if that will cause
problems.)

[Mark Mentovai, 2016-01-13 19:40:23]

Who or what is Kasko already talking to when you want it to read annotations?

When this happens, will crashpad_handler have already built up its own snapshot?

[Patrick Monette, 2016-01-13 22:59:40]

mark@ When the Chrome watcher process detects that the browser is hanged, it
wants to get annotations before asking Kasko to create a crash report for the
process. Everything is going to be initialized then.

scottmg@ I responded to your comments. Now I need to create a test. PTAL in the
meantime.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
File snapshot/client/crashpad_annotations_win.cc (right):

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:29: if
(!process_reader.Initialize(process, ProcessSuspensionState::kRunning))
On 2016/01/13 19:12:42, scottmg wrote:
> Would it be a good idea to suspend the process before reading here? See
> ScopedProcessSuspend.

If you think it's a good idea, I don't see any objections.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:32: MODULEINFO module_info = {};
On 2016/01/13 19:12:42, scottmg wrote:
> I don't think this needs to be initialized, as it's documented as [out] for
> GetModuleInformation().

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:33: if (!::GetModuleInformation(
On 2016/01/13 19:12:42, scottmg wrote:
> No :: prefix in Crashpad unless it's necessary to disambiguate.

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:33: if (!::GetModuleInformation(
On 2016/01/13 19:12:42, scottmg wrote:
> I believe this needs the GET_FUNCTION_REQUIRED dance that
> snapshot/win/pe_image_reader_test.cc does in order to work correctly on
> pre-Win7.

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.cc:35: return false;
On 2016/01/13 19:12:42, scottmg wrote:
> PLOG(ERROR) << "GetModuleInformation" here.

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
File snapshot/client/crashpad_annotations_win.h (right):

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:15: #ifndef
CRASHPAD_SNAPSHOT_SNAPSHOT_CLIENT_CRASHPAD_ANNOTATIONS_WIN_H_
On 2016/01/13 19:12:42, scottmg wrote:
> There's an extra SNAPSHOT_ in the guard here.

Removed.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:24: // Reads the crashpad annotations
associated to |module| inside |process| and
On 2016/01/13 19:12:42, scottmg wrote:
> crashpad -> Crashpad. Or maybe instead, since we know they're Crashpad
> annotations when we're in Crashpad, it should be module annotations. The file
> name and function name should be changed to match.

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:24: // Reads the crashpad annotations
associated to |module| inside |process| and
On 2016/01/13 19:12:42, scottmg wrote:
> These need to be Doxygen-ized in Crashpad. Comments start with //!, followed
by
> a \brief summary. Instead of |arg|, use \a arg, and add \param[in] and
> \param[out] documentation for the arguments.

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:29: HMODULE module,
On 2016/01/13 19:12:42, scottmg wrote:
> Is it going to be tricky for a user to provide HMODULE?

Good question. Currently I was thinking about enumerating all modules and
chosing the one with basename chrome.exe. Does that seem sufficient?

https://codereview.chromium.org/1586433003/diff/60001/snapshot/client/crashpa...
snapshot/client/crashpad_annotations_win.h:30: std::map<std::string,
std::string>* annotations);
On 2016/01/13 19:12:42, scottmg wrote:
> #include <string>

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp
File snapshot/snapshot.gyp (right):

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp#n...
snapshot/snapshot.gyp:131: 'target_name': 'snapshot_client',
On 2016/01/13 19:12:42, scottmg wrote:
> I think 'client' is a bit confusing because it won't necessarily be in a
client
> (in the sense that it's registering with the handler).
> 
> Maybe 'api'? Or maybe Mark has a better suggestion.

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp#n...
snapshot/snapshot.gyp:131: 'target_name': 'snapshot_client',
On 2016/01/13 19:12:42, scottmg wrote:
> This should probably be 'crashpad_' prefixed for consistency.

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp#n...
snapshot/snapshot.gyp:142: 'client/crashpad_annotations_win.cc',
On 2016/01/13 19:12:42, scottmg wrote:
> api/ here too.

Done.

https://codereview.chromium.org/1586433003/diff/60001/snapshot/snapshot.gyp#n...
snapshot/snapshot.gyp:142: 'client/crashpad_annotations_win.cc',
On 2016/01/13 19:12:42, scottmg wrote:
> (This will be an empty target on non-Windows, not sure if that will cause
> problems.)

Fixed.

[scottmg, 2016-01-13 23:13:39]

Changes look good, let me know when there's a test up and I'll take another
look.

https://codereview.chromium.org/1586433003/diff/80001/snapshot/api/module_ann...
File snapshot/api/module_annotations_win.cc (right):

https://codereview.chromium.org/1586433003/diff/80001/snapshot/api/module_ann...
snapshot/api/module_annotations_win.cc:25: HMODULE module,
Reformat for indent here.

https://codereview.chromium.org/1586433003/diff/80001/snapshot/api/module_ann...
File snapshot/api/module_annotations_win.h (right):

https://codereview.chromium.org/1586433003/diff/80001/snapshot/api/module_ann...
snapshot/api/module_annotations_win.h:32: //!     Remains unchanged if the
function return false.
return -> returns, and `false`.

[Patrick Monette, 2016-01-14 22:48:54]

Description was changed from

==========
Make external projects able to include snapshot.gyp

Kasko needs a way to read crash keys from out of process.
It will be able to reuse PEImageAnnotationsReader to do so.

BUG=chromium:485149
==========

to

==========
Adding an API to read module annotations in snapshot.gyp

Kasko needs a way to read crash keys from out of process. This API reuses the
functionality of PEImageAnnotationsReader.

BUG=chromium:485149
==========

[Patrick Monette, 2016-01-14 22:58:53]

I added a test. PTAL

https://codereview.chromium.org/1586433003/diff/80001/snapshot/api/module_ann...
File snapshot/api/module_annotations_win.cc (right):

https://codereview.chromium.org/1586433003/diff/80001/snapshot/api/module_ann...
snapshot/api/module_annotations_win.cc:25: HMODULE module,
On 2016/01/13 23:13:39, scottmg wrote:
> Reformat for indent here.

Done.

https://codereview.chromium.org/1586433003/diff/80001/snapshot/api/module_ann...
File snapshot/api/module_annotations_win.h (right):

https://codereview.chromium.org/1586433003/diff/80001/snapshot/api/module_ann...
snapshot/api/module_annotations_win.h:32: //!     Remains unchanged if the
function return false.
On 2016/01/13 23:13:39, scottmg wrote:
> return -> returns, and `false`.

Done.

[scottmg, 2016-01-18 17:11:59]

lgtm

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
File snapshot/api/module_annotations_win.h (right):

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
snapshot/api/module_annotations_win.h:30: //!     will be read.
The test makes it clear, but note here that `module` should be as loaded in the
target process.

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
File snapshot/api/module_annotations_win_test.cc (right):

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
snapshot/api/module_annotations_win_test.cc:28: virtual void
WinMultiprocessParent() override {
Only "override", remove "virtual".

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
snapshot/api/module_annotations_win_test.cc:54: virtual void
WinMultiprocessChild() override {
Same.

https://codereview.chromium.org/1586433003/diff/100001/util/win/get_module_in...
File util/win/get_module_information.cc (right):

https://codereview.chromium.org/1586433003/diff/100001/util/win/get_module_in...
util/win/get_module_information.cc:26: GET_FUNCTION_REQUIRED(L"psapi.dll",
::GetModuleInformation);
I think you can probably remove the  :: on GetModuleInformation here.

https://codereview.chromium.org/1586433003/diff/100001/util/win/get_module_in...
File util/win/get_module_information.h (right):

https://codereview.chromium.org/1586433003/diff/100001/util/win/get_module_in...
util/win/get_module_information.h:25: //! \brief Proxy function for
GetModuleInformation.
Doxygen doesn't know about GetModuleInformation, so this should be
`GetModuleInformation()`.

[Patrick Monette, 2016-01-18 19:35:44]

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
File snapshot/api/module_annotations_win.h (right):

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
snapshot/api/module_annotations_win.h:30: //!     will be read.
On 2016/01/18 17:11:59, scottmg wrote:
> The test makes it clear, but note here that `module` should be as loaded in
the
> target process.

Done.

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
File snapshot/api/module_annotations_win_test.cc (right):

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
snapshot/api/module_annotations_win_test.cc:28: virtual void
WinMultiprocessParent() override {
On 2016/01/18 17:11:59, scottmg wrote:
> Only "override", remove "virtual".

Done.

https://codereview.chromium.org/1586433003/diff/100001/snapshot/api/module_an...
snapshot/api/module_annotations_win_test.cc:54: virtual void
WinMultiprocessChild() override {
On 2016/01/18 17:11:59, scottmg wrote:
> Same.

Done.

https://codereview.chromium.org/1586433003/diff/100001/util/win/get_module_in...
File util/win/get_module_information.cc (right):

https://codereview.chromium.org/1586433003/diff/100001/util/win/get_module_in...
util/win/get_module_information.cc:26: GET_FUNCTION_REQUIRED(L"psapi.dll",
::GetModuleInformation);
On 2016/01/18 17:11:59, scottmg wrote:
> I think you can probably remove the  :: on GetModuleInformation here.

Done.

https://codereview.chromium.org/1586433003/diff/100001/util/win/get_module_in...
File util/win/get_module_information.h (right):

https://codereview.chromium.org/1586433003/diff/100001/util/win/get_module_in...
util/win/get_module_information.h:25: //! \brief Proxy function for
GetModuleInformation.
On 2016/01/18 17:11:59, scottmg wrote:
> Doxygen doesn't know about GetModuleInformation, so this should be
> `GetModuleInformation()`.

Done.

[scottmg, 2016-01-18 19:43:21]

https://codereview.chromium.org/1586433003/diff/120001/util/win/get_module_in...
File util/win/get_module_information.h (right):

https://codereview.chromium.org/1586433003/diff/120001/util/win/get_module_in...
util/win/get_module_information.h:25: //! \brief Proxy function for
'GetModuleInformation()'.
These have to be ` not '.

[Mark Mentovai, 2016-01-19 21:35:22]

Note that this moved to and was committed at
https://chromium-review.googlesource.com/322550/.