win: Move Crashpad all into chrome.exe

win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=cpu@chromium.org, mark@chromium.org
BUG=546288, 561653, 564328
Committed: https://chromium.googlesource.com/chromium/src/+/cae6b0ec5d499bb49dc0591087b8bc16b4ae339a

[scottmg, 2015-11-26 00:45:59]

Description was changed from

==========
win: Move Crashpad all into chrome.exe

R=mark@chromium.org
BUG=546288
==========

to

==========
win: Move Crashpad all into chrome.exe

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

[scottmg, 2015-11-26 00:45:59]

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

[scottmg, 2015-11-26 00:47:44]

Description was changed from

==========
win: Move Crashpad all into chrome.exe

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

to

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain.

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

[scottmg, 2015-11-26 00:50:31]

Description was changed from

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain.

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

to

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc the get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization earlier.

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

[scottmg, 2015-11-26 00:50:46]

Description was changed from

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc the get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization earlier.

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

to

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc the get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

[scottmg, 2015-11-26 00:52:27]

Patchset #1 (id:1) has been deleted

[scottmg, 2015-11-26 00:52:31]

Patchset #1 (id:20001) has been deleted

[scottmg, 2015-11-26 00:52:35]

Patchset #1 (id:40001) has been deleted

[scottmg, 2015-11-26 00:52:50]

Patchset #1 (id:60001) has been deleted

[scottmg, 2015-11-26 00:54:48]

Should have just done this originally, much better to have it all done up front.

(No need to review until after Thanksgiving if you're already gone for the
weekend.)

[scottmg, 2015-11-27 17:11:50]

Description was changed from

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc the get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

to

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

[cpu_(ooo_6.6-7.5), 2015-11-30 23:47:40]

looks like somebody reverted crashpad?

[Mark Mentovai, 2015-12-01 00:03:06]

Yeah, Scott did! But it’ll hopefully reland soon…

I’m treating this one as on ice until Crashpad does land in Chrome and sticks.

[scottmg, 2015-12-01 01:36:28]

Description was changed from

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=mark@chromium.org, cpu@chromium.org
BUG=546288
==========

to

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=mark@chromium.org, cpu@chromium.org
BUG=546288, 561653
==========

[scottmg, 2015-12-01 01:37:07]

On 2015/12/01 00:03:06, Mark Mentovai wrote:
> Yeah, Scott did! But it’ll hopefully reland soon…
> 
> I’m treating this one as on ice until Crashpad does land in Chrome and sticks.

Sorry, should have mentioned this was back to pending.

Optimistic that Crashpad will stick this again, I've rebased. So, it's safe to
look at this again.

[scottmg, 2015-12-01 01:37:16]

On 2015/12/01 00:03:06, Mark Mentovai wrote:
> Yeah, Scott did! But it’ll hopefully reland soon…
> 
> I’m treating this one as on ice until Crashpad does land in Chrome and sticks.

Sorry, should have mentioned this was back to pending.

Optimistic that Crashpad will stick this time, I've rebased. So, it's safe to
look at this again.

[scottmg, 2015-12-02 02:59:14]

Description was changed from

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=mark@chromium.org, cpu@chromium.org
BUG=546288, 561653
==========

to

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=mark@chromium.org, cpu@chromium.org
BUG=546288, 561653, 564328
==========

[Mark Mentovai, 2015-12-02 03:05:07]

Handler startup and registration happen almost as early as possible now. Great!

I don’t see any magic for making crash keys (module annotations) work. Is that
stuff just available to the dll because it’s in the exe? (It’s fine that the
annotations will wind up in the exe instead of the dll, I’m just asking because
I don’t see where this happens.)

https://codereview.chromium.org/1481703002/diff/180001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1481703002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:193: // on Windows.
Worth associating a bug?

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
File chrome/browser/crash_upload_list_crashpad.cc (right):

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_crashpad.cc:18: void GetUploadedReportsThunk(
:/

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_crashpad.cc:20: static
GetUploadedReportsPointer get_uploaded_reports = nullptr;
Too bad we don’t have GET_FUNCTION() here.

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_crashpad.cc:51:
GetUploadedReportsThunk(&uploaded_reports);
Maybe this ought to be done in crash_reporter::GetUplaodedReports so that the
knowledge of this split doesn’t have to happen in crash_upload_list_crashpad.

[scottmg, 2015-12-02 03:19:19]

Patchset #7 (id:200001) has been deleted

[scottmg, 2015-12-02 05:33:42]

On 2015/12/02 03:05:07, Mark Mentovai wrote:
> Handler startup and registration happen almost as early as possible now.
Great!
> 
> I don’t see any magic for making crash keys (module annotations) work. Is that
> stuff just available to the dll because it’s in the exe? (It’s fine that the
> annotations will wind up in the exe instead of the dll, I’m just asking
because
> I don’t see where this happens.)

Do you mean setting them? Each process will go through the call to
base::debug::SetCrashKeyReportingFunctions() in InitializeCrashpad(), and
base/debug/crash_logging goes through those functions to set the keys.

[scottmg, 2015-12-02 05:35:01]

Still fiddling with initialization order in the test binaries unfortunately.

https://codereview.chromium.org/1481703002/diff/180001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1481703002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:193: // on Windows.
On 2015/12/02 03:05:07, Mark Mentovai wrote:
> Worth associating a bug?

Done.

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
File chrome/browser/crash_upload_list_crashpad.cc (right):

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_crashpad.cc:18: void GetUploadedReportsThunk(
On 2015/12/02 03:05:07, Mark Mentovai wrote:
> :/

Yeah, it's icky. But not unprecedented as I learned today, sadly.

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_crashpad.cc:20: static
GetUploadedReportsPointer get_uploaded_reports = nullptr;
On 2015/12/02 03:05:07, Mark Mentovai wrote:
> Too bad we don’t have GET_FUNCTION() here.

Yeah, we should probably add that to base.

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_crashpad.cc:51:
GetUploadedReportsThunk(&uploaded_reports);
On 2015/12/02 03:05:07, Mark Mentovai wrote:
> Maybe this ought to be done in crash_reporter::GetUplaodedReports so that the
> knowledge of this split doesn’t have to happen in crash_upload_list_crashpad.

I thought it was a bit better here because then at least both pieces of the
split are in chrome/. Otherwise, components/crash/ needs to knows chrome/app,
which seems like it shouldn't. But it's pretty artificial either way, so I'll
switch if you think in crash_reporter is better.

[Mark Mentovai, 2015-12-02 14:59:23]

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
File chrome/browser/crash_upload_list_crashpad.cc (right):

https://codereview.chromium.org/1481703002/diff/180001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_crashpad.cc:51:
GetUploadedReportsThunk(&uploaded_reports);
scottmg wrote:
> I thought it was a bit better here because then at least both pieces of the
> split are in chrome/. Otherwise, components/crash/ needs to knows chrome/app,
> which seems like it shouldn't. But it's pretty artificial either way, so I'll
> switch if you think in crash_reporter is better.

I guess it is better out here. Who’s to say that non-Chrome clients of the
component will want to split things up in the same way?

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:167:
std::vector<crash_reporter::UploadedReport>* uploaded_reports) {
Is there also precedent for moving C++ types across module boundaries? Do
chrome.exe and chrome.dll share the same C++ standard library/CRT?

[cpu_(ooo_6.6-7.5), 2015-12-02 19:12:26]

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:138:
DCHECK_EQ(command_line.GetSwitchValueASCII(switches::kProcessType),
I don't know if we can use dchecks here since the logger stuff is initialized in
the dll. Or rather there was a conflict with the two loggers. I asked Bruce to
look into this so he might have the best guess at what is going to happen.

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:157: argv_as_utf8[argv.size()] = nullptr;
the above reads funny, as if you are zeroing one-past the end ... are you?

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:167:
std::vector<crash_reporter::UploadedReport>* uploaded_reports) {
On 2015/12/02 14:59:23, Mark Mentovai wrote:
> Is there also precedent for moving C++ types across module boundaries? Do
> chrome.exe and chrome.dll share the same C++ standard library/CRT?

This is somewhat safe as long as there is no exe and dll mismatch, that is
chrome N exe loading chrome N+1 dll, however I believe that issue still exists
on the field. There is even a chromecash/ bucket for it.  

Chrome exe and chrome dll share the sanbox C++ interface but that is pure
virtual, which will be also the safest choice here

class UploadedReports {
   virtual X* GetNext() = 0;
   virtual void Reset() = 0;
};

Something like that could work, assuming that we only want to read reports.
Mutating the std vector will be a no-no across binaries.

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:193: // on Windows.
https://code.google.com/p/chromium/issues/detail?id=564398.
yeah, once you start using the path providers you pretty much stuck with the
AEM. I don't imagine is worth fighting that fight.

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:215:
content::InitializeSandboxInfo(&sandbox_info);
this is what I was talking about sharing c++ across modules.

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:319: ClearDidRun(dll_path);
can you place a breakpoint here? we want to make sure we only do this for the
process that loads chrome.dll, for browser.

[scottmg, 2015-12-02 21:28:46]

Patchset #11 (id:300001) has been deleted

[scottmg, 2015-12-02 21:57:30]

Sorry for the churn, but I think it's better to try to avoid the AtExitManager
and PathService. WDYT?

I was a little concerned that there might be some cross-module problem with the
crash keys as Mark mentioned, but crash/32b5d04728d4fbb1 was uploaded with this
code and the "Fields" looks OK to me.

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:138:
DCHECK_EQ(command_line.GetSwitchValueASCII(switches::kProcessType),
On 2015/12/02 19:12:26, cpu wrote:
> I don't know if we can use dchecks here since the logger stuff is initialized
in
> the dll. Or rather there was a conflict with the two loggers. I asked Bruce to
> look into this so he might have the best guess at what is going to happen.

Oh good point. It's definitely not initialized at this point, so I guess it'll
do something weird if it fails. It's only checking the code immediately before
this function was called, so I just removed it.

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:157: argv_as_utf8[argv.size()] = nullptr;
On 2015/12/02 19:12:26, cpu wrote:
> the above reads funny, as if you are zeroing one-past the end ... are you?

Yeah, it looks funny, but I think it's OK. It's allocated one larger on line
150. The getopt library that we use in crashpad isn't so awesome and seems to
rely on the argv[argc] = null.

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:167:
std::vector<crash_reporter::UploadedReport>* uploaded_reports) {
On 2015/12/02 19:12:26, cpu wrote:
> On 2015/12/02 14:59:23, Mark Mentovai wrote:
> > Is there also precedent for moving C++ types across module boundaries? Do
> > chrome.exe and chrome.dll share the same C++ standard library/CRT?
> 
> This is somewhat safe as long as there is no exe and dll mismatch, that is
> chrome N exe loading chrome N+1 dll, however I believe that issue still exists
> on the field. There is even a chromecash/ bucket for it.  
> 
> Chrome exe and chrome dll share the sanbox C++ interface but that is pure
> virtual, which will be also the safest choice here
> 
> class UploadedReports {
>    virtual X* GetNext() = 0;
>    virtual void Reset() = 0;
> };
> 
> Something like that could work, assuming that we only want to read reports.
> Mutating the std vector will be a no-no across binaries.
> 

Since I need to store the returned list in the exe then anyway, I just had the
helper function return a pointer and a count.

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:193: // on Windows.
https://code.google.com/p/chromium/issues/detail?id=564398.
On 2015/12/02 19:12:26, cpu wrote:
> yeah, once you start using the path providers you pretty much stuck with the
> AEM. I don't imagine is worth fighting that fight.

The whole thing is kind of a pain. I'm not really sure why PathService needs a
LazyInstance, or why that needs an AtExitManager anyway (couldn't it just leak
the path data?)

But it was also a pain to get the delicate initialization order right in all the
different test binaries (different for chrome vs. content), so I went for it,
since it's also a bit less code running here before we get registered.

We end up needing %LOCALAPPDATA%, the exe path, module path, and %PROGRAMFILES%
so I made standalone functions for those in base_paths_win.

Too ugly or OK?

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:215:
content::InitializeSandboxInfo(&sandbox_info);
On 2015/12/02 19:12:26, cpu wrote:
> this is what I was talking about sharing c++ across modules.

Thanks, copied something like that.

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:319: ClearDidRun(dll_path);
On 2015/12/02 19:12:26, cpu wrote:
> can you place a breakpoint here? we want to make sure we only do this for the
> process that loads chrome.dll, for browser.

This function gets called for most processes (other than the renderers which get
otherwise terminated). But it doesn't end up hitting the ClearDidRun code
because RESULT_CODE_NORMAL_EXIT_CANCEL is not returned.

[Mark Mentovai, 2015-12-02 22:01:04]

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:138:
DCHECK_EQ(command_line.GetSwitchValueASCII(switches::kProcessType),
scottmg wrote:
> Oh good point. It's definitely not initialized at this point, so I guess it'll
> do something weird if it fails. It's only checking the code immediately before
> this function was called, so I just removed it.

There are DCHECKs that might get hit in HandlerMain() too, though. Uh-oh.

[scottmg, 2015-12-02 22:28:28]

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1481703002/diff/240001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:138:
DCHECK_EQ(command_line.GetSwitchValueASCII(switches::kProcessType),
On 2015/12/02 22:01:04, Mark Mentovai wrote:
> scottmg wrote:
> > Oh good point. It's definitely not initialized at this point, so I guess
it'll
> > do something weird if it fails. It's only checking the code immediately
before
> > this function was called, so I just removed it.
> 
> There are DCHECKs that might get hit in HandlerMain() too, though. Uh-oh.

In practice in a debug build on Windows, it logs to stderr and debug.log and
then __debugbreak()s. So it's a little confusing because you never see a window,
but not too bad. So I could probably put the DCHECK back in here, but it also
wasn't that useful.

[Mark Mentovai, 2015-12-02 22:47:12]

Overall this LGTM now.

https://codereview.chromium.org/1481703002/diff/320001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1481703002/diff/320001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:287: if
(GetAlternativeCrashDumpLocation(crash_dir))
This slurps a path out of BREAKPAD_DUMP_LOCATION. What uses that? Is it
expecting the directory to have a Breakpad-type structure?

https://codereview.chromium.org/1481703002/diff/320001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:294: result =
result.Append(chrome::kUserDataDirname);
Need to manually look for --user-data-dir too?

https://codereview.chromium.org/1481703002/diff/320001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:299: if (!base::PathExists(result) &&
!base::CreateDirectory(result))
The Crashpad handler doesn’t care if you start it without this existing. You can
take at least this blocking operation out of the startup path, although really
that just moves it into handler startup, which we do block on. But there’s no
reason to do it twice.

Dunno if there’s something else in Chrome that wants to see this directory, but
there shouldn’t be.

https://codereview.chromium.org/1481703002/diff/320001/chrome/browser/crash_u...
File chrome/browser/crash_upload_list_crashpad.cc (right):

https://codereview.chromium.org/1481703002/diff/320001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_crashpad.cc:21: static
GetUploadedReportsPointer get_uploaded_reports = nullptr;
Can you do the static GetUploadedReportsPointer x = [](){} thing so that it’ll
only try to initialize this the first time through? No reason to try again if it
fails.

[scottmg, 2015-12-03 01:40:15]

Thanks!

In the continuing saga of the Last Few Tests, I'm going around in circles. e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...
uses PathService coupled with registry blocking to serve as a way to mock out
the various system-installed vs. user-installed cases.

Looking at PathService,
https://code.google.com/p/chromium/codesearch#chromium/src/base/path_service....
I don't think there's a good reason for it to need AtExitManager. The destructor
doesn't seem to be doing anything useful, so it could just as well be a ::Leaky,
which means it wouldn't need to register for destruction.

So, I thiiiink I'll go back to PathService, but GetCrashDumpLocation() will only
use the paths that are supplied by base_paths_win, because those are statically
initialized, so we don't need to finesse where content::,
chrome::RegisterPathProviders(), etc. are called.

Le sigh.

https://codereview.chromium.org/1481703002/diff/320001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1481703002/diff/320001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:287: if
(GetAlternativeCrashDumpLocation(crash_dir))
On 2015/12/02 22:47:12, Mark Mentovai wrote:
> This slurps a path out of BREAKPAD_DUMP_LOCATION. What uses that? Is it
> expecting the directory to have a Breakpad-type structure?

The only place I could find on Windows is
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
but now that I look deeper, it's actually launching crash_service/Breakpad, so I
probably shouldn't honour this, and it'll need more porting anyway.

https://codereview.chromium.org/1481703002/diff/320001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:294: result =
result.Append(chrome::kUserDataDirname);
On 2015/12/02 22:47:12, Mark Mentovai wrote:
> Need to manually look for --user-data-dir too?

This is copied from
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...
which didn't respect that.

https://codereview.chromium.org/1481703002/diff/320001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:299: if (!base::PathExists(result) &&
!base::CreateDirectory(result))
On 2015/12/02 22:47:12, Mark Mentovai wrote:
> The Crashpad handler doesn’t care if you start it without this existing. You
can
> take at least this blocking operation out of the startup path, although really
> that just moves it into handler startup, which we do block on. But there’s no
> reason to do it twice.
> 
> Dunno if there’s something else in Chrome that wants to see this directory,
but
> there shouldn’t be.

Done.

https://codereview.chromium.org/1481703002/diff/320001/chrome/browser/crash_u...
File chrome/browser/crash_upload_list_crashpad.cc (right):

https://codereview.chromium.org/1481703002/diff/320001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_crashpad.cc:21: static
GetUploadedReportsPointer get_uploaded_reports = nullptr;
On 2015/12/02 22:47:12, Mark Mentovai wrote:
> Can you do the static GetUploadedReportsPointer x = [](){} thing so that it’ll
> only try to initialize this the first time through? No reason to try again if
it
> fails.

Done.

[scottmg, 2015-12-03 05:08:02]

Should have followed that thought about PathService/AtExitManager earlier. Much
less fiddling with base_paths, and everything else keeps using PathService as
usual.

[Mark Mentovai, 2015-12-03 15:11:32]

https://codereview.chromium.org/1481703002/diff/420001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1481703002/diff/420001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:284: // the chrome's PathService
entries (for DIR_CRASH_DUMPS) being available on
Lose “the”.

https://codereview.chromium.org/1481703002/diff/420001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:287: if
(!PathService::Get(base::DIR_LOCAL_APP_DATA, &result))
Following up on the --user-data-dir discussion:

The DIR_CRASH_DUMPS getter comes with this comment:

>       // The crash reports are always stored relative to the default user data
>       // directory.  This avoids the problem of having to re-initialize the
>       // exception handler after parsing command line options, which may
>       // override the location of the app's profile directory.

which becomes moot if you honor --user-data-dir here.

You can leave it as-is for now and we can save the change for a follow-up fix,
but now that we’ve got a distinct but equivalent way to get DIR_CRASH_DUMPS, the
rationale for not respecting --user-data-dir is shaky.

My main concern is that Chrome treats the consent bit as per-user-data-dir, but
if multiple user-data-dirs share the same Crashpad settings database, things can
easily get out of sync.

It looks like we have this problem on Mac now too, so maybe it’s best to save it
for a follow-up fix after all.

[scottmg, 2015-12-03 18:14:51]

https://codereview.chromium.org/1481703002/diff/420001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1481703002/diff/420001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:284: // the chrome's PathService
entries (for DIR_CRASH_DUMPS) being available on
On 2015/12/03 15:11:32, Mark Mentovai wrote:
> Lose “the”.

Done.

https://codereview.chromium.org/1481703002/diff/420001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:287: if
(!PathService::Get(base::DIR_LOCAL_APP_DATA, &result))
On 2015/12/03 15:11:32, Mark Mentovai wrote:
> Following up on the --user-data-dir discussion:
> 
> The DIR_CRASH_DUMPS getter comes with this comment:
> 
> >       // The crash reports are always stored relative to the default user
data
> >       // directory.  This avoids the problem of having to re-initialize the
> >       // exception handler after parsing command line options, which may
> >       // override the location of the app's profile directory.
> 
> which becomes moot if you honor --user-data-dir here.
> 
> You can leave it as-is for now and we can save the change for a follow-up fix,
> but now that we’ve got a distinct but equivalent way to get DIR_CRASH_DUMPS,
the
> rationale for not respecting --user-data-dir is shaky.
> 
> My main concern is that Chrome treats the consent bit as per-user-data-dir,
but
> if multiple user-data-dirs share the same Crashpad settings database, things
can
> easily get out of sync.
> 
> It looks like we have this problem on Mac now too, so maybe it’s best to save
it
> for a follow-up fix after all.

Yeah, that's a good point. I filed crbug.com/565466, and tagged this location
and DIR_CRASH_DUMPS with the bug, but didn't change anything for now.

I don't think anything outside of //components/crash actually needs
DIR_CRASH_DUMPS when using Crashpad, so for Mac and Win we could just make the
canonical location GetCrashDumpLocation() and not support DIR_CRASH_DUMPS at
all.

[Mark Mentovai, 2015-12-03 20:35:43]

This just keeps getting better and better, but the LGTM still stands.

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:292: // http://crbug.com/565466.
Wrong bug number (466 v. 446). Also, you should use https for links when it
works, line 285 too.

https://codereview.chromium.org/1481703002/diff/440001/chrome/common/chrome_p...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/1481703002/diff/440001/chrome/common/chrome_p...
chrome/common/chrome_paths.cc:211: // http://crbug.com/565466.
This one too.

[cpu_(ooo_6.6-7.5), 2015-12-03 20:41:08]

https://codereview.chromium.org/1481703002/diff/440001/base/path_service.cc
File base/path_service.cc (left):

https://codereview.chromium.org/1481703002/diff/440001/base/path_service.cc#o...
base/path_service.cc:139: };
go ahead and land this /base change in a separate CL with tbr me now.

[scottmg, 2015-12-03 20:52:52]

Thanks

https://codereview.chromium.org/1481703002/diff/440001/base/path_service.cc
File base/path_service.cc (left):

https://codereview.chromium.org/1481703002/diff/440001/base/path_service.cc#o...
base/path_service.cc:139: };
On 2015/12/03 20:41:08, cpu wrote:
> go ahead and land this /base change in a separate CL with tbr me now.
> 

Done: https://codereview.chromium.org/1487213004

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:292: // http://crbug.com/565466.
On 2015/12/03 20:35:42, Mark Mentovai wrote:
> Wrong bug number (466 v. 446). Also, you should use https for links when it
> works, line 285 too.

Oops. I swear https didn't work with crbug.com before, but it seems to now. Done
and done.

https://codereview.chromium.org/1481703002/diff/440001/chrome/common/chrome_p...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/1481703002/diff/440001/chrome/common/chrome_p...
chrome/common/chrome_paths.cc:211: // http://crbug.com/565466.
On 2015/12/03 20:35:42, Mark Mentovai wrote:
> This one too.

Done.

[cpu_(ooo_6.6-7.5), 2015-12-03 20:53:27]

back when I was one of about 3 peeps poking chrome.exe code I had strict rules
about avoiding most of base/ including pathservice etc. Then I relented about
running experiments there which open the floodgates. Frankly at this point
either way, whatever has better chance of being correct and not exploding in
some configuration.

Anyhow lgtm on my side.

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:289: BrowserDistribution* dist =
BrowserDistribution::GetDistribution();
no need for delete dist; ?

also, are you trying to do the same as chrome/installer/util/helper.cc ?

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:327: ChromeCrashReporterClient
chrome_crash_client;
does removing the #include break this deviant ... err I mean variant?

[scottmg, 2015-12-03 21:06:23]

Thanks!

(Rebased on https://codereview.chromium.org/1487213004/).

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client.cc:289: BrowserDistribution* dist =
BrowserDistribution::GetDistribution();
On 2015/12/03 20:53:27, cpu wrote:
> no need for delete dist; ?
> 

The code's a bit weird, but I don't think so.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...
It makes it into a global and then expects that to stick around.

> also, are you trying to do the same as chrome/installer/util/helper.cc ?

I didn't even see that one! This is an inline implementation of
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro....

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1481703002/diff/440001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:327: ChromeCrashReporterClient
chrome_crash_client;
On 2015/12/03 20:53:27, cpu wrote:
> does removing the #include break this deviant ... err I mean variant?

Ah, good spotting! Thanks, done.

The good thing is that it has no CQ bot or main waterfall bot, so someone just
panics later when they try to ship a SyzyAsan canary.

[scottmg, 2015-12-03 21:09:43]

On 2015/12/03 20:53:27, cpu wrote:
> back when I was one of about 3 peeps poking chrome.exe code I had strict rules
> about avoiding most of base/ including pathservice etc. Then I relented about
> running experiments there which open the floodgates. Frankly at this point
> either way, whatever has better chance of being correct and not exploding in
> some configuration.

Yeah, I feel like PathService captures well why people say "globals are a bad
idea". If it was just utility functions like call-a-Windows-API it would be ok,
but now it has all kinds of subtle ordering, with values computed in complicated
ways, set from all over the place, sp it's kind of confused. Oh well.

> 
> Anyhow lgtm on my side.
> 
>
https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
> File chrome/app/chrome_crash_reporter_client.cc (right):
> 
>
https://codereview.chromium.org/1481703002/diff/440001/chrome/app/chrome_cras...
> chrome/app/chrome_crash_reporter_client.cc:289: BrowserDistribution* dist =
> BrowserDistribution::GetDistribution();
> no need for delete dist; ?
> 
> also, are you trying to do the same as chrome/installer/util/helper.cc ?
> 
>
https://codereview.chromium.org/1481703002/diff/440001/chrome/app/main_dll_lo...
> File chrome/app/main_dll_loader_win.cc (right):
> 
>
https://codereview.chromium.org/1481703002/diff/440001/chrome/app/main_dll_lo...
> chrome/app/main_dll_loader_win.cc:327: ChromeCrashReporterClient
> chrome_crash_client;
> does removing the #include break this deviant ... err I mean variant?

[scottmg, 2015-12-03 23:16:35]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2015-12-03 23:16:36]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/1481703002/#ps480001
(title: "fix and rebase")

[commit-bot: I haz the power, 2015-12-03 23:17:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1481703002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1481703002/480001

[scottmg, 2015-12-04 00:02:44]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2015-12-04 00:02:51]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-12-04 00:09:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1481703002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1481703002/480001

[scottmg, 2015-12-04 00:10:17]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2015-12-04 00:10:28]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-12-04 00:14:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1481703002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1481703002/480001

[scottmg, 2015-12-04 00:35:40]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2015-12-04 00:35:49]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-12-04 00:40:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1481703002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1481703002/480001

[commit-bot: I haz the power, 2015-12-04 01:23:36]

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

[commit-bot: I haz the power, 2015-12-04 01:23:37]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[scottmg, 2015-12-04 01:27:53]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-12-04 01:28:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1481703002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1481703002/480001

[scottmg, 2015-12-04 03:17:25]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2015-12-04 03:17:32]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-12-04 03:19:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1481703002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1481703002/480001

[commit-bot: I haz the power, 2015-12-04 03:26:19]

Description was changed from

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=mark@chromium.org, cpu@chromium.org
BUG=546288, 561653, 564328
==========

to

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=cpu@chromium.org, mark@chromium.org
BUG=546288, 561653, 564328

Committed: https://crrev.com/cae6b0ec5d499bb49dc0591087b8bc16b4ae339a
Cr-Commit-Position: refs/heads/master@{#363097}
==========

[commit-bot: I haz the power, 2015-12-04 03:26:20]

Patchset 19 (id:??) landed as
https://crrev.com/cae6b0ec5d499bb49dc0591087b8bc16b4ae339a
Cr-Commit-Position: refs/heads/master@{#363097}

[scottmg, 2015-12-04 03:26:52]

Description was changed from

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=cpu@chromium.org, mark@chromium.org
BUG=546288, 561653, 564328

Committed: https://crrev.com/cae6b0ec5d499bb49dc0591087b8bc16b4ae339a
Cr-Commit-Position: refs/heads/master@{#363097}
==========

to

==========
win: Move Crashpad all into chrome.exe

This initializes Crashpad much earlier at the beginning of WinMain,
which captures early crashes about as well as we can. It also removes
all Crashpad code from chrome.dll and chrome_child.dll.

The cost for this is one thunk function that's looked up in
crash_upload_list_crashpad.cc to get back into chrome.exe (for its
global ChromeCrashReporterClient). That part is a little icky, but I
think it's worthwhile to get initialization before anything in the DLL
is run.

R=cpu@chromium.org, mark@chromium.org
BUG=546288, 561653, 564328

Committed:
https://chromium.googlesource.com/chromium/src/+/cae6b0ec5d499bb49dc0591087b8...
==========

[scottmg, 2015-12-04 03:26:53]

Committed patchset #19 (id:480001) manually as
cae6b0ec5d499bb49dc0591087b8bc16b4ae339a (presubmit successful).