restore LOG_FATAL crash key on windows

restore LOG_FATAL crash key on windows

The method logging::SetLogMessageHandler saves callback in global
variable. It doesn't work since crashpad initialization moved
to chrome_elf.dll. That's why crash reports lost LOG_FATAL crash key.
DCHECK(false) << "This string won't be attached to crash reports on windows.";

R=rsesek@chromium.org,ananta@chromium.org,dcheng@chromium.org

[rkuksin, 2016-09-28 07:37:38]

Description was changed from

==========
restore LOG_FATAL crash key on windows

The method logging::SetLogMessageHandler saves callback in global
variable. It doesn't work since crashpad initialization moved
to chrome_elf.dll. That's why crash reports lost LOG_FATAL crash key.

R=rsesek@chromium.org,ananta@chromium.org,dcheng@chromium.org
==========

to

==========
restore LOG_FATAL crash key on windows

The method logging::SetLogMessageHandler saves callback in global
variable. It doesn't work since crashpad initialization moved
to chrome_elf.dll. That's why crash reports lost LOG_FATAL crash key.
DCHECK(false) << "This string won't be attached to crash reports on windows.";

R=rsesek@chromium.org,ananta@chromium.org,dcheng@chromium.org
==========

[dcheng, 2016-09-28 20:20:16]

Hmm, does it make sense for this initialization to be in //base? Why wouldn't it
just move to the appropriate main()?

[Robert Sesek, 2016-09-28 22:04:08]

This doesn't seem like the right approach. At the bottom of crashpad.cc are the
exports for use by the chrome.dll module. I think this function should be
reexported so that Chrome can call it.

[rkuksin, 2016-09-29 03:17:09]

On 2016/09/28 22:04:08, Robert Sesek wrote:
> This doesn't seem like the right approach. At the bottom of crashpad.cc are
the
> exports for use by the chrome.dll module. I think this function should be
> reexported so that Chrome can call it.

These exported functions are allready used here:
https://cs.chromium.org/chromium/src/chrome/common/child_process_logging_win....
Later, they are called from base::debug::SetCrashKeyValue.

It seems like a convinient way for setting crash keys from Chrome.

[rkuksin, 2016-09-29 03:28:04]

On 2016/09/28 20:20:16, dcheng wrote:
> Hmm, does it make sense for this initialization to be in //base? Why wouldn't
it
> just move to the appropriate main()?

I'm not sure that I understand you correctly.
I didn't put any initialization inside //base.
This patch only makes the LOG_FATAL crash key to be set
when LOG(FATAL) appears.

Could you please clarify what initialization are you talking about?

[Robert Sesek, 2016-09-29 22:43:14]

On 2016/09/29 03:17:09, rkuksin wrote:
> On 2016/09/28 22:04:08, Robert Sesek wrote:
> > This doesn't seem like the right approach. At the bottom of crashpad.cc are
> the
> > exports for use by the chrome.dll module. I think this function should be
> > reexported so that Chrome can call it.
> 
> These exported functions are allready used here:
>
https://cs.chromium.org/chromium/src/chrome/common/child_process_logging_win....
> Later, they are called from base::debug::SetCrashKeyValue.
> 
> It seems like a convinient way for setting crash keys from Chrome.

I think what both Daniel and I are getting at is that SetLogFatalCrashKey() does
not belong in base--it belongs in crashpad.cc. If you export that function from
the chrome_elf module, then chrome can use it as the log message handler
function.

[rkuksin, 2016-09-30 03:25:19]

On 2016/09/29 22:43:14, Robert Sesek wrote:
> On 2016/09/29 03:17:09, rkuksin wrote:
> > On 2016/09/28 22:04:08, Robert Sesek wrote:
> > > This doesn't seem like the right approach. At the bottom of crashpad.cc
are
> > the
> > > exports for use by the chrome.dll module. I think this function should be
> > > reexported so that Chrome can call it.
> > 
> > These exported functions are allready used here:
> >
>
https://cs.chromium.org/chromium/src/chrome/common/child_process_logging_win....
> > Later, they are called from base::debug::SetCrashKeyValue.
> > 
> > It seems like a convinient way for setting crash keys from Chrome.
> 
> I think what both Daniel and I are getting at is that SetLogFatalCrashKey()
does
> not belong in base--it belongs in crashpad.cc. If you export that function
from
> the chrome_elf module, then chrome can use it as the log message handler
> function.

What is the difference between SetLogFatalCrashKey and SetCrashKeyValueImpl
(already exported)?
Why to export this additional function?
Doesn't it look as a duplication?
base/crash_logging already sends crash keys to chrome_elf. Why not to use it
from base/logging?

[rkuksin, 2016-10-04 06:05:06]

There is another argument against using logging::SetLogMessageHandler.
We cannot use it for crash keys setting because it is used by
logging::LogEventProvider:
https://cs.chromium.org/chromium/src/base/logging_win.cc?q=logging_win&sq=pac...

[scottmg, 2016-10-05 22:06:20]

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

[scottmg, 2016-10-05 22:06:21]

(drive-by by request)

It doesn't seem too bad to me to have base/logging.h depend on
base/debug/crash_logging.h. I'm not familiar with the intricacies of base, is it
that we're trying to avoid some circular dependencies within base subfolders?

https://codereview.chromium.org/2377963002/diff/1/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2377963002/diff/1/base/logging.cc#newcode133
base/logging.cc:133: // Nacl targets miss base/debug/crash_logging.cc.
nit; "NaCl targets do not have ..."

https://codereview.chromium.org/2377963002/diff/1/base/logging.cc#newcode134
base/logging.cc:134: #if !defined(NACL_TC_REV)
OS_NACL.

https://codereview.chromium.org/2377963002/diff/1/base/logging.cc#newcode148
base/logging.cc:148: base::debug::SetCrashKeyValue("LOG_FATAL", message);
This takes a StringPiece so no need for the Sprintfing? In fact, I would just
inline

#if !defined(OS_NACL)
if (severity_ == LOG_FATAL)
  base::debug::SetCrashKeyValue("LOG_FATAL", StringPiece(...)) 
#endif  // OS_NACL

below instead.

https://codereview.chromium.org/2377963002/diff/1/components/crash/content/ap...
File components/crash/content/app/crashpad.cc (left):

https://codereview.chromium.org/2377963002/diff/1/components/crash/content/ap...
components/crash/content/app/crashpad.cc:167:
logging::SetLogMessageHandler(LogMessageHandler);
Does this do anything here for logging that does actually happen in chrome_elf?
I'm not sure without digging into it.

[rkuksin, 2016-10-06 05:53:14]

On 2016/10/05 22:06:21, scottmg wrote:
> I'm not familiar with the intricacies of base, is it
> that we're trying to avoid some circular dependencies within base subfolders?

It seems, such dependencies are widespread.
For example, "base/debug/debugger.cc" inlcudes "base/logging.h" and
"base/logging.cc" includes "base/debug/debugger.h"

This patch adds similar relation.

[rkuksin, 2016-10-06 08:50:33]

https://codereview.chromium.org/2377963002/diff/1/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2377963002/diff/1/base/logging.cc#newcode133
base/logging.cc:133: // Nacl targets miss base/debug/crash_logging.cc.
On 2016/10/05 22:06:21, scottmg wrote:
> nit; "NaCl targets do not have ..."

Done.

https://codereview.chromium.org/2377963002/diff/1/base/logging.cc#newcode134
base/logging.cc:134: #if !defined(NACL_TC_REV)
On 2016/10/05 22:06:21, scottmg wrote:
> OS_NACL.

Done.

https://codereview.chromium.org/2377963002/diff/1/base/logging.cc#newcode148
base/logging.cc:148: base::debug::SetCrashKeyValue("LOG_FATAL", message);
On 2016/10/05 22:06:21, scottmg wrote:
> This takes a StringPiece so no need for the Sprintfing? In fact, I would just
> inline
> 
> #if !defined(OS_NACL)
> if (severity_ == LOG_FATAL)
>   base::debug::SetCrashKeyValue("LOG_FATAL", StringPiece(...)) 
> #endif  // OS_NACL
> 
> below instead.

Done.

https://codereview.chromium.org/2377963002/diff/1/components/crash/content/ap...
File components/crash/content/app/crashpad.cc (left):

https://codereview.chromium.org/2377963002/diff/1/components/crash/content/ap...
components/crash/content/app/crashpad.cc:167:
logging::SetLogMessageHandler(LogMessageHandler);
On 2016/10/05 22:06:21, scottmg wrote:
> Does this do anything here for logging that does actually happen in
chrome_elf?
> I'm not sure without digging into it.

base::debug::SetCrashKeyReportingFunctions subtitutes this (line 156).

[dcheng, 2016-10-06 08:52:40]

https://codereview.chromium.org/2377963002/diff/20001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2377963002/diff/20001/base/logging.cc#newcode544
base/logging.cc:544: if (severity_ == LOG_FATAL) {
This still needs to protect against reentrancy, unless I'm missing something.

[rkuksin, 2016-10-07 07:44:12]

https://codereview.chromium.org/2377963002/diff/20001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2377963002/diff/20001/base/logging.cc#newcode544
base/logging.cc:544: if (severity_ == LOG_FATAL) {
On 2016/10/06 08:52:40, dcheng wrote:
> This still needs to protect against reentrancy, unless I'm missing something.

Done.

[dcheng, 2016-10-07 21:35:01]

//base LGTM

[rkuksin, 2016-10-08 13:02:33]

The CQ bit was checked by rkuksin@yandex-team.ru

[commit-bot: I haz the power, 2016-10-08 13:02:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-08 13:11:32]

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

[commit-bot: I haz the power, 2016-10-08 13:11:34]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rkuksin, 2016-10-11 03:25:47]

Hi!
I still need approvement for these files:
    blimp/engine/app/blimp_engine_crash_keys.cc
    chrome/app/chrome_crash_reporter_client_win.cc
    chrome/common/crash_keys.cc
    components/crash/content/app/crashpad.cc

Is there anything else I should do in this issue?

[rkuksin, 2016-10-17 02:40:51]

Review please. Need lgtm.

[rkuksin, 2016-10-20 10:57:04]

On 2016/10/20 10:56:35, rkuksin wrote:

PTAL

[scottmg, 2016-10-20 15:48:46]

lgtm for crashpad.cc if Robert thinks it's OK too.

rkuksin: in general, if you're looking for lgtm's on specific files, you need to
call out who should look at what, because when there's multiple reviewers people
aren't going to hunt around to figure out what they might be OWNERS for.

[Robert Sesek, 2016-10-21 15:38:00]

I still don't agree that this should be in base, but dcheng is OWNER so lgtm.

https://codereview.chromium.org/2377963002/diff/60001/chrome/common/crash_key...
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/2377963002/diff/60001/chrome/common/crash_key...
chrome/common/crash_keys.cc:106: { kShutdownType, kSmallSize },
Move the key to here, since it's not a temporary one.

[dcheng, 2016-10-21 15:52:44]

On 2016/10/21 15:38:00, Robert Sesek wrote:
> I still don't agree that this should be in base, but dcheng is OWNER so lgtm.

Hmm... so when I originally lgtmed this CL, I was assuming that other code in
//base made use of crash keys. But I didn't verify, and you're right: there's no
precedent for it.

So now I'm starting to think maybe it shouldn't go in base after all. Sorry =/

> 
>
https://codereview.chromium.org/2377963002/diff/60001/chrome/common/crash_key...
> File chrome/common/crash_keys.cc (right):
> 
>
https://codereview.chromium.org/2377963002/diff/60001/chrome/common/crash_key...
> chrome/common/crash_keys.cc:106: { kShutdownType, kSmallSize },
> Move the key to here, since it's not a temporary one.

[rkuksin, 2016-10-24 09:08:28]

dcheng, PTAL
I added additional message handler to base::logging. Chrome sets it when
registering crash keys functions.

Does it look acceptable for you?

https://codereview.chromium.org/2377963002/diff/60001/chrome/common/crash_key...
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/2377963002/diff/60001/chrome/common/crash_key...
chrome/common/crash_keys.cc:106: { kShutdownType, kSmallSize },
On 2016/10/21 15:38:00, Robert Sesek wrote:
> Move the key to here, since it's not a temporary one.

Done.

[dcheng, 2016-10-27 21:52:09]

I think part of rsesek's feedback is that the crash key itself shouldn't be
specified by //base, it should be specified by the things using //base.

It's also not clear to me why we need a new handler: in theory, nothing prevents
us from chaining log message handlers (remoting does this). And even if we
don't, there's not that many places that use log message handlers (outside of
test code, the only other usage I see is in GPU).

[rkuksin, 2016-10-28 04:39:52]

On 2016/10/27 21:52:09, dcheng wrote:
> I think part of rsesek's feedback is that the crash key itself shouldn't be
> specified by //base, it should be specified by the things using //base.

Done

> It's also not clear to me why we need a new handler: in theory, nothing
prevents
> us from chaining log message handlers (remoting does this). And even if we
> don't, there's not that many places that use log message handlers (outside of
> test code, the only other usage I see is in GPU).

What about this place:
https://cs.chromium.org/chromium/src/base/logging_win.cc?q=logging_win.cc&sq=...
Chrome calls it after crashpad and child_process_logging initialization. So it
always resets the log message handler.

[dcheng, 2016-11-11 09:26:14]

So I'm wondering if we should just natively support multiple logging handlers.
It makes this a bit more complex, but since it seems like it's really easy to
make conflicting logging handlers, that seems like the better path forward.

Probably, we can do that in a separate patch, and then rebase this on top of
that. WDYT?

[rkuksin, 2016-11-11 10:14:18]

OK

[Robert Sesek, 2016-11-14 16:13:44]

On 2016/11/11 09:26:14, dcheng wrote:
> So I'm wondering if we should just natively support multiple logging handlers.
> It makes this a bit more complex, but since it seems like it's really easy to
> make conflicting logging handlers, that seems like the better path forward.
> 
> Probably, we can do that in a separate patch, and then rebase this on top of
> that. WDYT?

Someone seems to be working on this already:
https://codereview.chromium.org/2034393004/