Report active extensions in crash reports (windows only).

Report active extensions in crash reports. This only implements Windows right now. Mac and linux will be separate CLs.

"Active" is overloaded to mean different things depending on
the process type:

- browser: all enabled extensions
- renderer: unique set of extensions from all user scripts
- extension: extensions running in the process

BUG=27169
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33255

[Aaron Boodman, 2009-11-26 01:03:39]

http://codereview.chromium.org/437078/diff/1/2
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/437078/diff/1/2#newcode86
chrome/app/breakpad_win.cc:86: google_breakpad::CustomInfoEntry
extension1(L"extension-1", L"");
I realize this is ghetto. Following convention of existing code, plus could not
think of a much better way to do it because of the fixed layout in memory.
Suggestions welcome.

[Erik does not do reviews, 2009-11-28 15:49:58]

http://codereview.chromium.org/437078/diff/1/2
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/437078/diff/1/2#newcode132
chrome/app/breakpad_win.cc:132: (*g_extension_ids)[i] = entries[13 + i].value;
it would be nice if you could pull these numbers (8, 5, 4, 13) out into
something less fragile.

http://codereview.chromium.org/437078/diff/1/2#newcode164
chrome/app/breakpad_win.cc:164: (*g_extension_ids)[i] = entries[7 + i].value;
same here

http://codereview.chromium.org/437078/diff/1/6
File chrome/common/child_process_logging_linux.cc (right):

http://codereview.chromium.org/437078/diff/1/6#newcode36
chrome/common/child_process_logging_linux.cc:36: NOTIMPLEMENTED();
add TODO(port)

I think you should just remove the NOTIMPLEMENTED() here.  Otherwise, Linux
debug builds with extensions will always crash, right?

http://codereview.chromium.org/437078/diff/1/7
File chrome/common/child_process_logging_mac.mm (right):

http://codereview.chromium.org/437078/diff/1/7#newcode96
chrome/common/child_process_logging_mac.mm:96: NOTIMPLEMENTED();
same here

http://codereview.chromium.org/437078/diff/1/8
File chrome/common/child_process_logging_win.cc (right):

http://codereview.chromium.org/437078/diff/1/8#newcode80
chrome/common/child_process_logging_win.cc:80: const int kMaxExtensions = 10;
Is there some way you could share the constant between here and breakpad_win.cc?

http://codereview.chromium.org/437078/diff/1/10
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/437078/diff/1/10#newcode123
chrome/renderer/user_script_slave.cc:123: // n^2, but meh, it's a small list.
nit: newline above comment

[Aaron Boodman, 2009-11-29 02:16:10]

Ok, I have reworked the way breakpad_win.cc works to be more amenable to my
change. I think it makes the existing code  nicer too.

It is important to not break this and I do not think it has much test coverage,
so can you take a close look?

http://codereview.chromium.org/437078/diff/1/2
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/437078/diff/1/2#newcode132
chrome/app/breakpad_win.cc:132: (*g_extension_ids)[i] = entries[13 + i].value;
On 2009/11/28 15:49:58, Erik Kay wrote:
> it would be nice if you could pull these numbers (8, 5, 4, 13) out into
> something less fragile.

Done.

http://codereview.chromium.org/437078/diff/1/2#newcode164
chrome/app/breakpad_win.cc:164: (*g_extension_ids)[i] = entries[7 + i].value;
On 2009/11/28 15:49:58, Erik Kay wrote:
> same here

Done.

http://codereview.chromium.org/437078/diff/1/6
File chrome/common/child_process_logging_linux.cc (right):

http://codereview.chromium.org/437078/diff/1/6#newcode36
chrome/common/child_process_logging_linux.cc:36: NOTIMPLEMENTED();
On 2009/11/28 15:49:58, Erik Kay wrote:
> add TODO(port)
> 
> I think you should just remove the NOTIMPLEMENTED() here.  Otherwise, Linux
> debug builds with extensions will always crash, right?

I thought that NOTIMPLEMENTED() was just a printline, not a crash, but the code
where it is declared is complicated, so I changed this to be sure.

http://codereview.chromium.org/437078/diff/1/8
File chrome/common/child_process_logging_win.cc (right):

http://codereview.chromium.org/437078/diff/1/8#newcode80
chrome/common/child_process_logging_win.cc:80: const int kMaxExtensions = 10;
On 2009/11/28 15:49:58, Erik Kay wrote:
> Is there some way you could share the constant between here and
breakpad_win.cc?

Done.

http://codereview.chromium.org/437078/diff/1/10
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/437078/diff/1/10#newcode123
chrome/renderer/user_script_slave.cc:123: // n^2, but meh, it's a small list.
On 2009/11/28 15:49:58, Erik Kay wrote:
> nit: newline above comment

Done.

[Erik does not do reviews, 2009-11-30 06:41:37]

http://codereview.chromium.org/437078/diff/4002/3005
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/437078/diff/4002/3005#newcode38
chrome/app/breakpad_win.cc:38: std::vector<wchar_t*>* g_url_chunks = NULL;
since |entries| is now a vector itself, just make it a global and store offsets
rather than managing extra vectors.

http://codereview.chromium.org/437078/diff/4002/3005#newcode77
chrome/app/breakpad_win.cc:77: const int kNumCustomInfoEntries = 17;
this should be something like kMiscEntriesCount + kMaxReportedActiveExtensions +
kMaxUrlChunks;

http://codereview.chromium.org/437078/diff/4002/3005#newcode115
chrome/app/breakpad_win.cc:115: // Browser-specific entries.
maybe this else clause should be if (type == "browser").  I think the
command-line args aren't that interesting for the other process types.  this is
also what the comment seems to be implying.

http://codereview.chromium.org/437078/diff/4002/3005#newcode135
chrome/app/breakpad_win.cc:135: DCHECK(entries.size() <= entries.capacity());
capacity() is always >= size(), so this will never fail.  You need to compare
against kNumCustomInfoEntries.

http://codereview.chromium.org/437078/diff/4002/3015
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/437078/diff/4002/3015#newcode127
chrome/renderer/user_script_slave.cc:127: scripts_[i]->extension_id()) ==
extension_ids.end()) {
good catch

[Erik does not do reviews, 2009-11-30 06:47:04]

sorry I didn't get this review to you earlier.  I spaced about the 3am
thing.  I think the code as-is is fine and these changes can be made in a
follow-on.

Erik


On Sun, Nov 29, 2009 at 10:41 PM, <erikkay@chromium.org> wrote:

>
> http://codereview.chromium.org/437078/diff/4002/3005
>
> File chrome/app/breakpad_win.cc (right):
>
> http://codereview.chromium.org/437078/diff/4002/3005#newcode38
> chrome/app/breakpad_win.cc:38: std::vector<wchar_t*>* g_url_chunks =
> NULL;
> since |entries| is now a vector itself, just make it a global and store
> offsets rather than managing extra vectors.
>
> http://codereview.chromium.org/437078/diff/4002/3005#newcode77
> chrome/app/breakpad_win.cc:77: const int kNumCustomInfoEntries = 17;
> this should be something like kMiscEntriesCount +
> kMaxReportedActiveExtensions + kMaxUrlChunks;
>
> http://codereview.chromium.org/437078/diff/4002/3005#newcode115
> chrome/app/breakpad_win.cc:115: // Browser-specific entries.
> maybe this else clause should be if (type == "browser").  I think the
> command-line args aren't that interesting for the other process types.
> this is also what the comment seems to be implying.
>
> http://codereview.chromium.org/437078/diff/4002/3005#newcode135
> chrome/app/breakpad_win.cc:135: DCHECK(entries.size() <=
> entries.capacity());
> capacity() is always >= size(), so this will never fail.  You need to
> compare against kNumCustomInfoEntries.
>
> http://codereview.chromium.org/437078/diff/4002/3015
>
> File chrome/renderer/user_script_slave.cc (right):
>
> http://codereview.chromium.org/437078/diff/4002/3015#newcode127
> chrome/renderer/user_script_slave.cc:127: scripts_[i]->extension_id())
> == extension_ids.end()) {
> good catch
>
>
> http://codereview.chromium.org/437078
>