Instantiate a Kasko reporter inside the Chrome watcher process, when SyzyASAN instrumented.

Instantiate a Kasko reporter inside the Chrome watcher process, when SyzyASAN instrumented.

BUG=460512
Committed: https://crrev.com/d2349e504d60c9fd0b99420a10e8a83df0f7d8fe
Cr-Commit-Position: refs/heads/master@{#317385}

[erikwright (departed), 2015-02-04 16:24:50]

erikwright@chromium.org changed reviewers:
+ grt@chromium.org, siggi@chromium.org

[erikwright (departed), 2015-02-04 16:24:50]

grt: PTAL at least for DEPS and chrome/app. Your excellent feedback also welcome
on the rest.
siggi: This is the Reporter-half of the CL you already reviewed here:
http://crrev.com/875053003/ . Please restamp.

[grt (UTC plus 2), 2015-02-04 16:42:51]

https://codereview.chromium.org/901673002/diff/1/chrome/app/client_util.cc
File chrome/app/client_util.cc (right):

https://codereview.chromium.org/901673002/diff/1/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:192: base::FilePath browser_data_directory;
what do you think about making a new chrome path for this? we already have
DIR_CRASH_DUMPS, so why not DIR_KASKO or somesuch? you could then perhaps use
GetDefaultUserDataDirectory in some way.

my reason for suggesting this is that i think it's better to have the sort of
logic you have here consolidated either in a path provider somewhere or in the
installer somewhere.

[erikwright (departed), 2015-02-04 18:25:54]

PTAL.

[grt (UTC plus 2), 2015-02-04 18:50:36]

chrome/app lgtm

https://codereview.chromium.org/901673002/diff/20001/chrome/common/chrome_pat...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/901673002/diff/20001/chrome/common/chrome_pat...
chrome/common/chrome_paths.cc:228: #endif
      break;
#endif

[Sigurður Ásgeirsson, 2015-02-04 19:51:31]

https://codereview.chromium.org/901673002/diff/20001/chrome/common/chrome_pat...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/901673002/diff/20001/chrome/common/chrome_pat...
chrome/common/chrome_paths.cc:228: #endif
On 2015/02/04 18:50:36, grt wrote:
>       break;
> #endif

what he said :)

[erikwright (departed), 2015-02-04 20:43:42]

New patchsets have been uploaded after l-g-t-m from grt@chromium.org

[erikwright (departed), 2015-02-04 20:45:35]

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

[erikwright (departed), 2015-02-04 20:45:35]

cpu: PTAL for chrome/common. This defines a directory within user-data-dir where
the watcher can store data (in particular SyzyASAN reports for upload via
Kasko).

[Sigurður Ásgeirsson, 2015-02-04 21:15:47]

lgtm

[cpu_(ooo_6.6-7.5), 2015-02-05 03:16:27]

lgtm

[erikwright (departed), 2015-02-05 15:37:01]

New patchsets have been uploaded after l-g-t-m from
siggi@chromium.org,cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2015-02-05 18:46:43]

https://codereview.chromium.org/901673002/diff/50001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/901673002/diff/50001/chrome/chrome_exe.gypi#n...
chrome/chrome_exe.gypi:718: '<(PRODUCT_DIR)/kasko.dll.pdb',
wait, what is this part?

[erikwright (departed), 2015-02-05 18:48:26]

https://codereview.chromium.org/901673002/diff/50001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/901673002/diff/50001/chrome/chrome_exe.gypi#n...
chrome/chrome_exe.gypi:718: '<(PRODUCT_DIR)/kasko.dll.pdb',
On 2015/02/05 18:46:43, cpu wrote:
> wait, what is this part? 

The kasko build artifacts are retrieved via DEPS to thirdparty/kasko.

In this CL we link to them from chrome_watcher.dll.

In order to run the watcher process on a dev machine those DLLs need to be in
out/Debug (for example).

[erikwright (departed), 2015-02-05 18:50:37]

grt: In a follow-up CL you asked me to use PathService to get the exe path.
Since the code in question originated in this CL I pulled that forward.

Also, introducing the new watcher data path in chrome_paths.h requires a call to
chrome::RegisterPathProvider. That call happens in chrome_main_delegate for the
browser process. I added an invocation in the watcher process code path.

[erikwright (departed), 2015-02-05 18:51:05]

cpu: I'll hold this until I have further confirmation from you.

[cpu_(ooo_6.6-7.5), 2015-02-05 19:08:24]

not lgtm. lets go back to the email thread.

I want to be perfectly clear that we are not shipping 3 different ways to do
crash reporting in chrome.

[grt (UTC plus 2), 2015-02-05 19:45:29]

https://codereview.chromium.org/901673002/diff/70001/chrome/chrome_watcher/ch...
File chrome/chrome_watcher/chrome_watcher.gypi (right):

https://codereview.chromium.org/901673002/diff/70001/chrome/chrome_watcher/ch...
chrome/chrome_watcher/chrome_watcher.gypi:58: 'chrome_watcher_main.cc',
this file includes syzygy/kasko/api/reporter.h, yet i don't see a dependency on
syzygy/kasko stuff down on lines 62-65. what's up with that?

https://codereview.chromium.org/901673002/diff/70001/chrome/chrome_watcher/ch...
File chrome/chrome_watcher/chrome_watcher_main.cc (right):

https://codereview.chromium.org/901673002/diff/70001/chrome/chrome_watcher/ch...
chrome/chrome_watcher/chrome_watcher_main.cc:30: #include
"syzygy/kasko/api/reporter.h"
#ifdef SYZYASAN around this?

[cpu_(ooo_6.6-7.5), 2015-02-10 22:02:40]

ok, back to lgtm, but grt question still stands.

[erikwright (departed), 2015-02-11 14:10:09]

New patchsets have been uploaded after l-g-t-m from cpu@chromium.org

[erikwright (departed), 2015-02-11 14:48:20]

grt: The dependency was not really set up properly. chrome_watcher was relying
on the DLLs being there, but it was chrome_exe that was meeting this
requirement. In fact, as of this CL, chrome_exe does not depend on kasko.dll
itself.

So I rejigged the dependencies a bit. Please re-review the .gyp(i?) files.

[grt (UTC plus 2), 2015-02-12 03:11:09]

ah, i think i see what's going on. i'm not sure i'm GYPish enough to say that
this is the right wayTM. also, please make corresponding BUILD.gn changes.

[cpu_(ooo_6.6-7.5), 2015-02-12 03:17:02]

+1 on the gn build. I didn't realize until today that gn is now building
chrome.exe

[erikwright (departed), 2015-02-12 19:05:59]

erikwright@chromium.org changed reviewers:
+ thakis@chromium.org

[erikwright (departed), 2015-02-12 19:06:00]

Nico, are you a good candidate to review changes to .gyp, .gypi, and .gn files?

My intent in this CL is as follows:

(1) There is already, in third_party/kasko/, kasko.dll, kasko.dll.pdb and
kasko.dll.lib .
(2) I introduce a dependency from chrome_watcher
(chrome/chrome_watcher/chrome_watcher.gypi, which is included by chrome.gyp)
onto kasko.dll. This requires adding third_party/kasko to the library search
path and adding an "additional dependency" on kasko.dll.lib.
(3) In order to make that dependency available at runtime, I need to have
kasko.dll (and, for good measure, its pdb) in the out/XXX directory.

Eventually (follow-up CL) there will be other dependencies on kasko.dll, so I
want to extract all of the steps into something simple (this pseudo target,
'kasko_dll') which will handle all of those steps.

Is the approach taken in this CL reasonable?

Note that all of this only happens if SyzyASAN is enabled. Since this is not
supported by GN, the majority of this logic is not replicated in the .gn file,
for now.

[erikwright (departed), 2015-02-19 18:09:53]

thakis: PTAL.

[Nico, 2015-02-19 20:31:34]

What's kasko?

Should this have a tracking bug?

[Nico, 2015-02-19 20:33:32]

Other than that, the gyp / gn changes lgtm.

[grt (UTC plus 2), 2015-02-19 21:00:18]

lgtm

[erikwright (departed), 2015-02-19 21:02:56]

On 2015/02/19 20:31:34, Nico wrote:
> What's kasko?
> 
> Should this have a tracking bug?

Launch bug: https://code.google.com/p/chromium/issues/detail?id=460204

Kasko is a crash reporting tool that will be used to send enhanced SyzyASAN
reports, and possibly also to do some other specialized crash reporting (OOP
hang detection from the Chrome Watcher process).

[Nico, 2015-02-19 21:17:17]

On 2015/02/19 21:02:56, erikwright wrote:
> On 2015/02/19 20:31:34, Nico wrote:
> > What's kasko?
> > 
> > Should this have a tracking bug?
> 
> Launch bug: https://code.google.com/p/chromium/issues/detail?id=460204

I mean a tracking bug that'll appear in the BUG= line of code changes (it sounds
like there'll be other changes related to this; things spanning multiple CLs
should have a tracking bug).

Having a launch bug is a good idea too though.

> Kasko is a crash reporting tool that will be used to send enhanced SyzyASAN
> reports, and possibly also to do some other specialized crash reporting (OOP
> hang detection from the Chrome Watcher process).

How does it relate to crashpad?

[Nico, 2015-02-19 21:19:14]

On 2015/02/19 21:17:17, Nico wrote:
> On 2015/02/19 21:02:56, erikwright wrote:
> > On 2015/02/19 20:31:34, Nico wrote:
> > > What's kasko?
> > > 
> > > Should this have a tracking bug?
> > 
> > Launch bug: https://code.google.com/p/chromium/issues/detail?id=460204
> 
> I mean a tracking bug that'll appear in the BUG= line of code changes (it
sounds
> like there'll be other changes related to this; things spanning multiple CLs
> should have a tracking bug).
> 
> Having a launch bug is a good idea too though.
> 
> > Kasko is a crash reporting tool that will be used to send enhanced SyzyASAN
> > reports, and possibly also to do some other specialized crash reporting (OOP
> > hang detection from the Chrome Watcher process).
> 
> How does it relate to crashpad?

Oh, this is answered in the design doc linked to from the launch bug. I'm
surprised we're adding another crash reporter (at 0.3MB no less!).

[erikwright (departed), 2015-02-19 21:41:43]

A good question, which is answered here:

https://docs.google.com/a/google.com/document/d/1lF3nwY3Lp1zAz_kBRI4tmirB7IuJ...

I will create a tracking bug before landing.

On Thu, Feb 19, 2015 at 4:17 PM, <thakis@chromium.org> wrote:

> On 2015/02/19 21:02:56, erikwright wrote:
>
>> On 2015/02/19 20:31:34, Nico wrote:
>> > What's kasko?
>> >
>> > Should this have a tracking bug?
>>
>
>  Launch bug: https://code.google.com/p/chromium/issues/detail?id=460204
>>
>
> I mean a tracking bug that'll appear in the BUG= line of code changes (it
> sounds
> like there'll be other changes related to this; things spanning multiple
> CLs
> should have a tracking bug).
>
> Having a launch bug is a good idea too though.
>
>  Kasko is a crash reporting tool that will be used to send enhanced
>> SyzyASAN
>> reports, and possibly also to do some other specialized crash reporting
>> (OOP
>> hang detection from the Chrome Watcher process).
>>
>
> How does it relate to crashpad?
>
> https://codereview.chromium.org/901673002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikwright (departed), 2015-02-19 21:43:25]

Only in SyzyASAN builds, and after consideration of other alternatives.
This will never ship concurrently with Crashpad.

On Thu, Feb 19, 2015 at 4:41 PM, Erik Wright <erikwright@chromium.org>
wrote:

> A good question, which is answered here:
>
>
>
https://docs.google.com/a/google.com/document/d/1lF3nwY3Lp1zAz_kBRI4tmirB7IuJ...
>
> I will create a tracking bug before landing.
>
> On Thu, Feb 19, 2015 at 4:17 PM, <thakis@chromium.org> wrote:
>
>> On 2015/02/19 21:02:56, erikwright wrote:
>>
>>> On 2015/02/19 20:31:34, Nico wrote:
>>> > What's kasko?
>>> >
>>> > Should this have a tracking bug?
>>>
>>
>>  Launch bug: https://code.google.com/p/chromium/issues/detail?id=460204
>>>
>>
>> I mean a tracking bug that'll appear in the BUG= line of code changes (it
>> sounds
>> like there'll be other changes related to this; things spanning multiple
>> CLs
>> should have a tracking bug).
>>
>> Having a launch bug is a good idea too though.
>>
>>  Kasko is a crash reporting tool that will be used to send enhanced
>>> SyzyASAN
>>> reports, and possibly also to do some other specialized crash reporting
>>> (OOP
>>> hang detection from the Chrome Watcher process).
>>>
>>
>> How does it relate to crashpad?
>>
>> https://codereview.chromium.org/901673002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikwright (departed), 2015-02-20 17:04:47]

New patchsets have been uploaded after l-g-t-m from
thakis@chromium.org,grt@chromium.org

[erikwright (departed), 2015-02-20 17:55:13]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2015-02-20 17:56:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/901673002/190001

[commit-bot: I haz the power, 2015-02-20 18:39:52]

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

[commit-bot: I haz the power, 2015-02-20 18:39:54]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[erikwright (departed), 2015-02-20 19:23:58]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2015-02-20 19:25:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/901673002/210001

[commit-bot: I haz the power, 2015-02-20 20:22:44]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2015-02-20 20:23:44]

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