Set dump_without_crashing_function_ in chrome.dll

Set dump_without_crashing_function_ in chrome.dll with value of exported chrome!DumpProcessWithoutCrash.

BUG=391974
TEST=chrome://crashdump and verify crash dump appears and tab does not crash
TBR=darin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284448

[Will Harris, 2014-07-07 23:53:57]

not sure I am setting this in the right place, please advise.

[Will Harris, 2014-07-07 23:59:33]

On 2014/07/07 23:53:57, Will Harris wrote:
> not sure I am setting this in the right place, please advise.

actually I'm thinking a better place for this code would be in
breakpad_win.cc:InitCrashReporter where it calls
base::debug::SetDumpWithoutCrashingFunction(&DumpProcessWithoutCrash) - just
pass the function exported from chrome.exe instead of &DumpProcessWithoutCrash -
WDYT?

[rvargas (doing something else), 2014-07-08 01:35:14]

On 2014/07/07 23:59:33, Will Harris wrote:
> On 2014/07/07 23:53:57, Will Harris wrote:
> > not sure I am setting this in the right place, please advise.
> 
> actually I'm thinking a better place for this code would be in
> breakpad_win.cc:InitCrashReporter where it calls
> base::debug::SetDumpWithoutCrashingFunction(&DumpProcessWithoutCrash) - just
> pass the function exported from chrome.exe instead of &DumpProcessWithoutCrash
-
> WDYT?

That would not help. breakpad_win.cc:InitCrashReporter() runs from the exe,
before the dll is loaded and that's why it is getting (correctly) the address
from the exe.

I'm on the fence about calling SetDumpWithoutCrashingFunction from two places
(exe and dll). Maybe we should remove the call from
breakpad_win.cc:InitCrashReporter, and add a comment in ChromeMain stating that
the variable should be set from the dll (pointing to the exe) and not from the
exe.

There may be an issue in having a content constant that doesn't do anything
without the code that lives in chrome.

[Will Harris, 2014-07-08 04:00:46]

On 2014/07/08 01:35:14, rvargas wrote:
> On 2014/07/07 23:59:33, Will Harris wrote:
> > On 2014/07/07 23:53:57, Will Harris wrote:
> > > not sure I am setting this in the right place, please advise.
> > 
> > actually I'm thinking a better place for this code would be in
> > breakpad_win.cc:InitCrashReporter where it calls
> > base::debug::SetDumpWithoutCrashingFunction(&DumpProcessWithoutCrash) - just
> > pass the function exported from chrome.exe instead of
&DumpProcessWithoutCrash
> -
> > WDYT?
> 
> That would not help. breakpad_win.cc:InitCrashReporter() runs from the exe,
> before the dll is loaded and that's why it is getting (correctly) the address
> from the exe.
> 
> I'm on the fence about calling SetDumpWithoutCrashingFunction from two places
> (exe and dll). Maybe we should remove the call from
> breakpad_win.cc:InitCrashReporter, and add a comment in ChromeMain stating
that
> the variable should be set from the dll (pointing to the exe) and not from the
> exe.
> 
> There may be an issue in having a content constant that doesn't do anything
> without the code that lives in chrome.

I don't think DumpWithoutCrashing is called from anywhere in chrome.exe so
removing the call from InitCrashReporter and moving it to ChromeMain sounds
like a good idea.  I also don't like the idea of it being in two places.

I could hard code "chrome.exe" instead of using kBrowserProcessExecutableName
or I could mine the loaded modules for chrome.exe to pull the symbol.

[rvargas (doing something else), 2014-07-08 19:39:12]

On 2014/07/08 04:00:46, Will Harris wrote:
> On 2014/07/08 01:35:14, rvargas wrote:
> > On 2014/07/07 23:59:33, Will Harris wrote:
> > > On 2014/07/07 23:53:57, Will Harris wrote:
> > > > not sure I am setting this in the right place, please advise.
> > > 
> > > actually I'm thinking a better place for this code would be in
> > > breakpad_win.cc:InitCrashReporter where it calls
> > > base::debug::SetDumpWithoutCrashingFunction(&DumpProcessWithoutCrash) -
just
> > > pass the function exported from chrome.exe instead of
> &DumpProcessWithoutCrash
> > -
> > > WDYT?
> > 
> > That would not help. breakpad_win.cc:InitCrashReporter() runs from the exe,
> > before the dll is loaded and that's why it is getting (correctly) the
address
> > from the exe.
> > 
> > I'm on the fence about calling SetDumpWithoutCrashingFunction from two
places
> > (exe and dll). Maybe we should remove the call from
> > breakpad_win.cc:InitCrashReporter, and add a comment in ChromeMain stating
> that
> > the variable should be set from the dll (pointing to the exe) and not from
the
> > exe.
> > 
> > There may be an issue in having a content constant that doesn't do anything
> > without the code that lives in chrome.
> 
> I don't think DumpWithoutCrashing is called from anywhere in chrome.exe so
> removing the call from InitCrashReporter and moving it to ChromeMain sounds
> like a good idea.  I also don't like the idea of it being in two places.
> 
> I could hard code "chrome.exe" instead of using kBrowserProcessExecutableName
> or I could mine the loaded modules for chrome.exe to pull the symbol.

I think there was some confusion with my comment. chrome_main looks fine as is
(modulo comment). I was talking about the concept of a kChromeUIDumpURL that
doesn't do anything for other content embedders unless they duplicate the code
that we are adding to chrome/app. That may be OK though (the code being in base
assumes that it may do nothing unless someone performs some setup)

[Will Harris, 2014-07-16 20:11:03]

On 2014/07/08 19:39:12, rvargas_Out_until_7-23 wrote:
> On 2014/07/08 04:00:46, Will Harris wrote:
> > On 2014/07/08 01:35:14, rvargas wrote:
> > > On 2014/07/07 23:59:33, Will Harris wrote:
> > > > On 2014/07/07 23:53:57, Will Harris wrote:
> > > > > not sure I am setting this in the right place, please advise.
> > > > 
> > > > actually I'm thinking a better place for this code would be in
> > > > breakpad_win.cc:InitCrashReporter where it calls
> > > > base::debug::SetDumpWithoutCrashingFunction(&DumpProcessWithoutCrash) -
> just
> > > > pass the function exported from chrome.exe instead of
> > &DumpProcessWithoutCrash
> > > -
> > > > WDYT?
> > > 
> > > That would not help. breakpad_win.cc:InitCrashReporter() runs from the
exe,
> > > before the dll is loaded and that's why it is getting (correctly) the
> address
> > > from the exe.
> > > 
> > > I'm on the fence about calling SetDumpWithoutCrashingFunction from two
> places
> > > (exe and dll). Maybe we should remove the call from
> > > breakpad_win.cc:InitCrashReporter, and add a comment in ChromeMain stating
> > that
> > > the variable should be set from the dll (pointing to the exe) and not from
> the
> > > exe.
> > > 
> > > There may be an issue in having a content constant that doesn't do
anything
> > > without the code that lives in chrome.
> > 
> > I don't think DumpWithoutCrashing is called from anywhere in chrome.exe so
> > removing the call from InitCrashReporter and moving it to ChromeMain sounds
> > like a good idea.  I also don't like the idea of it being in two places.
> > 
> > I could hard code "chrome.exe" instead of using
kBrowserProcessExecutableName
> > or I could mine the loaded modules for chrome.exe to pull the symbol.
> 
> I think there was some confusion with my comment. chrome_main looks fine as is
> (modulo comment). I was talking about the concept of a kChromeUIDumpURL that
> doesn't do anything for other content embedders unless they duplicate the code
> that we are adding to chrome/app. That may be OK though (the code being in
base
> assumes that it may do nothing unless someone performs some setup)

rvargas is out at the moment - darin@ jochen@ can you take a look at this latest
CL?  This is also blocking crbug.com/391165 for more context.

[jochen (gone - plz use gerrit), 2014-07-17 07:18:18]

On 2014/07/16 at 20:11:03, wfh wrote:
> On 2014/07/08 19:39:12, rvargas_Out_until_7-23 wrote:
> > On 2014/07/08 04:00:46, Will Harris wrote:
> > > On 2014/07/08 01:35:14, rvargas wrote:
> > > > On 2014/07/07 23:59:33, Will Harris wrote:
> > > > > On 2014/07/07 23:53:57, Will Harris wrote:
> > > > > > not sure I am setting this in the right place, please advise.
> > > > > 
> > > > > actually I'm thinking a better place for this code would be in
> > > > > breakpad_win.cc:InitCrashReporter where it calls
> > > > > base::debug::SetDumpWithoutCrashingFunction(&DumpProcessWithoutCrash)
-
> > just
> > > > > pass the function exported from chrome.exe instead of
> > > &DumpProcessWithoutCrash
> > > > -
> > > > > WDYT?
> > > > 
> > > > That would not help. breakpad_win.cc:InitCrashReporter() runs from the
exe,
> > > > before the dll is loaded and that's why it is getting (correctly) the
> > address
> > > > from the exe.
> > > > 
> > > > I'm on the fence about calling SetDumpWithoutCrashingFunction from two
> > places
> > > > (exe and dll). Maybe we should remove the call from
> > > > breakpad_win.cc:InitCrashReporter, and add a comment in ChromeMain
stating
> > > that
> > > > the variable should be set from the dll (pointing to the exe) and not
from
> > the
> > > > exe.
> > > > 
> > > > There may be an issue in having a content constant that doesn't do
anything
> > > > without the code that lives in chrome.
> > > 
> > > I don't think DumpWithoutCrashing is called from anywhere in chrome.exe so
> > > removing the call from InitCrashReporter and moving it to ChromeMain
sounds
> > > like a good idea.  I also don't like the idea of it being in two places.
> > > 
> > > I could hard code "chrome.exe" instead of using
kBrowserProcessExecutableName
> > > or I could mine the loaded modules for chrome.exe to pull the symbol.
> > 
> > I think there was some confusion with my comment. chrome_main looks fine as
is
> > (modulo comment). I was talking about the concept of a kChromeUIDumpURL that
> > doesn't do anything for other content embedders unless they duplicate the
code
> > that we are adding to chrome/app. That may be OK though (the code being in
base
> > assumes that it may do nothing unless someone performs some setup)
> 
> rvargas is out at the moment - darin@ jochen@ can you take a look at this
latest CL?  This is also blocking crbug.com/391165 for more context.

can you verify that this creates a crash dump in content_shell as well?
http://www.chromium.org/developers/testing/webkit-layout-tests/using-breakpad...
has the instructions how to use breakpad with content shell

[jochen (gone - plz use gerrit), 2014-07-17 07:28:16]

wfh explained the details over chat. The issue is that base is linked both in
the exe and the dll, so we have two versions of all base/ statics :(

we could just revert the change that moved DumpWithoutCrashing into base/ as
it's no longer used outside of chrome/

or, if you prefer this versions then lgtm

[Will Harris, 2014-07-21 16:07:41]

On 2014/07/17 07:28:16, jochen wrote:
> wfh explained the details over chat. The issue is that base is linked both in
> the exe and the dll, so we have two versions of all base/ statics :(
> 
> we could just revert the change that moved DumpWithoutCrashing into base/ as
> it's no longer used outside of chrome/
> 
> or, if you prefer this versions then lgtm

TBRing darin@ for the comment change in base/debug

[Will Harris, 2014-07-21 16:07:44]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2014-07-21 16:09:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wfh@chromium.org/371073003/40001

[commit-bot: I haz the power, 2014-07-21 17:14:01]

Change committed as 284448

[darin (slow to review), 2014-07-21 20:29:33]

LGTM for base/debug