Introduce the ability to wait for the watcher process to initialize.

Introduce the ability to wait for the watcher process to initialize.

BUG=

Committed: https://crrev.com/c57091e4a94ef5cf881424d2a3449a19fa97f251
Cr-Commit-Position: refs/heads/master@{#314563}

[erikwright (departed), 2015-01-28 20:45:49]

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

[erikwright (departed), 2015-01-28 20:45:50]

siggi: PTAL. This will be used to block on the establishment of the Kasko RPC
service.

[Sigurður Ásgeirsson, 2015-01-29 16:59:57]

nice - I have but nits.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_unittest_win.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_unittest_win.cc:58: ::CreateEvent(NULL, FALSE,
FALSE, kExitEventName));
I think this will create flakes on parallel tests. Better to inherit the handle
or provide a unique name with a flag or environment variable, or by parent
process pid, or some such.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_unittest_win.cc:65: ::CreateEvent(NULL, FALSE,
FALSE, kInitializeEventName));
ditto

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:15: :
command_line_generator_(command_line_generator) {
wonky indent?

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:22: SECURITY_ATTRIBUTES
on_initialized_event_attributes = {
nit: worth providing running commentary here. Inheritable handles are
uncommon...

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:41: bool
ChromeWatcherClient::WaitForInitialization() {
nit: this assumes single-threaded use. Maybe mention as much in the header - or
perhaps this doesn't warrant comment as it's the default for Chrome?

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:89: process_.Terminate(exit_code);
interesting detail - I don't think this'll change the exit code of an already
exited process?

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_win.h (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.h:23: // is fully initialized. The second
HANDLE is the process that the watcher
note that both handles must be inheritable?
Maybe also worth a note somewhere on the necessary lifetime of these handles, as
those are all rather unusual details?

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.h:46: // has not already exited. Blocks
until the process has exited and returns its
This is hopefully for testing only?

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
File chrome/chrome_watcher/chrome_watcher_main.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
chrome/chrome_watcher/chrome_watcher_main.cc:45: // signaled from the background
thread immediately before blocking on the exit
nit: the background thread is implementation details - maybe phrase this more
generically?

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
chrome/chrome_watcher/chrome_watcher_main.cc:114:
base::Passed(&on_initialized_event)))) {
ugh - this syntax irks me, but I guess that's just me ...

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
chrome/chrome_watcher/chrome_watcher_main.cc:146: // Signal our client now that
RPC is initialized and we have cleared all of
RPC?

[erikwright (departed), 2015-01-30 20:39:57]

PTAL.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_unittest_win.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_unittest_win.cc:58: ::CreateEvent(NULL, FALSE,
FALSE, kExitEventName));
On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> I think this will create flakes on parallel tests. Better to inherit the
handle
> or provide a unique name with a flag or environment variable, or by parent
> process pid, or some such.

Done.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_unittest_win.cc:65: ::CreateEvent(NULL, FALSE,
FALSE, kInitializeEventName));
On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> ditto

Done.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:15: :
command_line_generator_(command_line_generator) {
On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> wonky indent?

This is what clang format does.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:22: SECURITY_ATTRIBUTES
on_initialized_event_attributes = {
On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> nit: worth providing running commentary here. Inheritable handles are
> uncommon...

Done.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:41: bool
ChromeWatcherClient::WaitForInitialization() {
On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> nit: this assumes single-threaded use. Maybe mention as much in the header -
or
> perhaps this doesn't warrant comment as it's the default for Chrome?

This is the default for Chrome.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:89: process_.Terminate(exit_code);
On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> interesting detail - I don't think this'll change the exit code of an already
> exited process?

Correct. That's why we return the actual exit code afterwards.

The tests do rely on this to assert that the mock watcher process had (or had
not) already exited.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_win.h (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.h:23: // is fully initialized. The second
HANDLE is the process that the watcher
On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> note that both handles must be inheritable?
> Maybe also worth a note somewhere on the necessary lifetime of these handles,
as
> those are all rather unusual details?

Given that the command-line generator is not responsible for the inheritance I'm
not sure it's relevant. I added a brief comment here anyway.

I'm not sure what lifetime issues you're referring to. Again it's not relevant
to the generator (I think it's self-evident that the HANDLEs will still be valid
at the time the generated cmdline is actually executed, but even still, not the
generator's problem).

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.h:46: // has not already exited. Blocks
until the process has exited and returns its
On 2015/01/29 16:59:57, Sigurður Ásgeirsson wrote:
> This is hopefully for testing only?

Yes, although I can't think of a good reason to label it as such. In my opinion,
a ForTesting label is appropriate when the use of the method somehow violates
the contract or encapsulation of the class and thus must be prevented. While
there is no existing production use-case for this method, there would be no harm
in invoking it if one were to materialize.

For example, it could conceivably make sense to call it if the user disabled
crash reporting after the browser had been launched.

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
File chrome/chrome_watcher/chrome_watcher_main.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
chrome/chrome_watcher/chrome_watcher_main.cc:45: // signaled from the background
thread immediately before blocking on the exit
On 2015/01/29 16:59:57, Sigurður Ásgeirsson wrote:
> nit: the background thread is implementation details - maybe phrase this more
> generically?

I have made an attempt. Note that the code as it stands makes no attempt at
encapsulation. The client is required to understand exactly what StartWatching
does, that it does so asynchronously, that it Quits |run_loop| at certain
points, etc.

So it's a bit difficult to extend that behaviour with some semblance of
encapsulation.

That being said, I considered extracting this class and found that it was
trivial enough, mainly consisting of configuring a bunch of disparate and
unrelated components, that the refactoring was without merit.

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
chrome/chrome_watcher/chrome_watcher_main.cc:114:
base::Passed(&on_initialized_event)))) {
On 2015/01/29 16:59:57, Sigurður Ásgeirsson wrote:
> ugh - this syntax irks me, but I guess that's just me ...

You mean base::Passed(&movable)? I replaced it with an alternative syntax that
might be less jarring.

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
chrome/chrome_watcher/chrome_watcher_main.cc:146: // Signal our client now that
RPC is initialized and we have cleared all of
On 2015/01/29 16:59:57, Sigurður Ásgeirsson wrote:
> RPC?

Done.

[erikwright (departed), 2015-02-02 15:53:54]

ping.

[Sigurður Ásgeirsson, 2015-02-02 16:59:20]

nice!

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:89: process_.Terminate(exit_code);
On 2015/01/30 20:39:57, erikwright wrote:
> On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> > interesting detail - I don't think this'll change the exit code of an
already
> > exited process?
> 
> Correct. That's why we return the actual exit code afterwards.
> 
> The tests do rely on this to assert that the mock watcher process had (or had
> not) already exited.

In that case the wait above is redundant - no?

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_win.h (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.h:46: // has not already exited. Blocks
until the process has exited and returns its
> For example, it could conceivably make sense to call it if the user disabled
> crash reporting after the browser had been launched.

Not the way it's currently implemented.
TerminateProcess should never be used, any more than WriteProcessMemory (read:
used only when absolutely necessary :). It would be appropriate to expose a
"please shutdown in an orderly fashion" interface, though.

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
File chrome/chrome_watcher/chrome_watcher_main.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
chrome/chrome_watcher/chrome_watcher_main.cc:114:
base::Passed(&on_initialized_event)))) {
On 2015/01/30 20:39:57, erikwright wrote:
> On 2015/01/29 16:59:57, Sigurður Ásgeirsson wrote:
> > ugh - this syntax irks me, but I guess that's just me ...
> 
> You mean base::Passed(&movable)? I replaced it with an alternative syntax that
> might be less jarring.

It's probably just me - but base::Passed(&movable) looks to me like passing a
pointer to a local across threads (despite knowing that it doesn't).

https://codereview.chromium.org/886613002/diff/20001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/20001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:50: bool
ChromeWatcherClient::WaitForInitialization() {
I'm not sure it's safe and sane to expose this thread unsafe interface to this
class now that the wait is deferred to the time of crash.

Consider where two threads in the client crash simultaneously?

Also maybe you want this to be idempotent - which probably wants another name
like Ensure* or some such?

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

On 2015/02/02 16:59:20, Sigurður Ásgeirsson wrote:
> nice!
> 
>
https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
> File chrome/app/chrome_watcher_client_win.cc (right):
> 
>
https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
> chrome/app/chrome_watcher_client_win.cc:89: process_.Terminate(exit_code);
> On 2015/01/30 20:39:57, erikwright wrote:
> > On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> > > interesting detail - I don't think this'll change the exit code of an
> already
> > > exited process?
> > 
> > Correct. That's why we return the actual exit code afterwards.
> > 
> > The tests do rely on this to assert that the mock watcher process had (or
had
> > not) already exited.
> 
> In that case the wait above is redundant - no?
> 
>
https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
> File chrome/app/chrome_watcher_client_win.h (right):
> 
>
https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
> chrome/app/chrome_watcher_client_win.h:46: // has not already exited. Blocks
> until the process has exited and returns its
> > For example, it could conceivably make sense to call it if the user disabled
> > crash reporting after the browser had been launched.
> 
> Not the way it's currently implemented.
> TerminateProcess should never be used, any more than WriteProcessMemory (read:
> used only when absolutely necessary :). It would be appropriate to expose a
> "please shutdown in an orderly fashion" interface, though.
> 
>
https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
> File chrome/chrome_watcher/chrome_watcher_main.cc (right):
> 
>
https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
> chrome/chrome_watcher/chrome_watcher_main.cc:114:
> base::Passed(&on_initialized_event)))) {
> On 2015/01/30 20:39:57, erikwright wrote:
> > On 2015/01/29 16:59:57, Sigurður Ásgeirsson wrote:
> > > ugh - this syntax irks me, but I guess that's just me ...
> > 
> > You mean base::Passed(&movable)? I replaced it with an alternative syntax
that
> > might be less jarring.
> 
> It's probably just me - but base::Passed(&movable) looks to me like passing a
> pointer to a local across threads (despite knowing that it doesn't).
> 
>
https://codereview.chromium.org/886613002/diff/20001/chrome/app/chrome_watche...
> File chrome/app/chrome_watcher_client_win.cc (right):
> 
>
https://codereview.chromium.org/886613002/diff/20001/chrome/app/chrome_watche...
> chrome/app/chrome_watcher_client_win.cc:50: bool
> ChromeWatcherClient::WaitForInitialization() {
> I'm not sure it's safe and sane to expose this thread unsafe interface to this
> class now that the wait is deferred to the time of crash.
> 
> Consider where two threads in the client crash simultaneously?
> 
> Also maybe you want this to be idempotent - which probably wants another name
> like Ensure* or some such?

PTAL.

[erikwright (departed), 2015-02-03 20:05:35]

Sorry, forgot to send the comments.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.cc:89: process_.Terminate(exit_code);
On 2015/02/02 16:59:20, Sigurður Ásgeirsson wrote:
> On 2015/01/30 20:39:57, erikwright wrote:
> > On 2015/01/29 16:59:56, Sigurður Ásgeirsson wrote:
> > > interesting detail - I don't think this'll change the exit code of an
> already
> > > exited process?
> > 
> > Correct. That's why we return the actual exit code afterwards.
> > 
> > The tests do rely on this to assert that the mock watcher process had (or
had
> > not) already exited.
> 
> In that case the wait above is redundant - no?

Correct.

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
File chrome/app/chrome_watcher_client_win.h (right):

https://codereview.chromium.org/886613002/diff/1/chrome/app/chrome_watcher_cl...
chrome/app/chrome_watcher_client_win.h:46: // has not already exited. Blocks
until the process has exited and returns its
On 2015/02/02 16:59:20, Sigurður Ásgeirsson wrote:
> > For example, it could conceivably make sense to call it if the user disabled
> > crash reporting after the browser had been launched.
> 
> Not the way it's currently implemented.
> TerminateProcess should never be used, any more than WriteProcessMemory (read:
> used only when absolutely necessary :). It would be appropriate to expose a
> "please shutdown in an orderly fashion" interface, though.
> 

How about this? It's up to the client to ask the "watcher" to shut down, but we
provide a way to wait on it. This seems to respect encapsulation.

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
File chrome/chrome_watcher/chrome_watcher_main.cc (right):

https://codereview.chromium.org/886613002/diff/1/chrome/chrome_watcher/chrome...
chrome/chrome_watcher/chrome_watcher_main.cc:114:
base::Passed(&on_initialized_event)))) {
On 2015/02/02 16:59:20, Sigurður Ásgeirsson wrote:
> On 2015/01/30 20:39:57, erikwright wrote:
> > On 2015/01/29 16:59:57, Sigurður Ásgeirsson wrote:
> > > ugh - this syntax irks me, but I guess that's just me ...
> > 
> > You mean base::Passed(&movable)? I replaced it with an alternative syntax
that
> > might be less jarring.
> 
> It's probably just me - but base::Passed(&movable) looks to me like passing a
> pointer to a local across threads (despite knowing that it doesn't).

Acknowledged.

https://codereview.chromium.org/886613002/diff/20001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/20001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:50: bool
ChromeWatcherClient::WaitForInitialization() {
On 2015/02/02 16:59:20, Sigurður Ásgeirsson wrote:
> I'm not sure it's safe and sane to expose this thread unsafe interface to this
> class now that the wait is deferred to the time of crash.
> 
> Consider where two threads in the client crash simultaneously?
> 
> Also maybe you want this to be idempotent - which probably wants another name
> like Ensure* or some such?

Both good points. Done.

[Sigurður Ásgeirsson, 2015-02-03 20:28:05]

lgtm - sweet!

https://codereview.chromium.org/886613002/diff/40001/components/browser_watch...
File components/browser_watcher/watcher_client_win.h (right):

https://codereview.chromium.org/886613002/diff/40001/components/browser_watch...
components/browser_watcher/watcher_client_win.h:35: void
AddInheritedHandle(HANDLE handle);
nit: add some docs?

[erikwright (departed), 2015-02-03 20:28:06]

Sorry, it appears I forgot to compile this. Fixed to make it work.

[Sigurður Ásgeirsson, 2015-02-03 20:30:05]

still lgtm

[erikwright (departed), 2015-02-03 20:35:38]

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

[erikwright (departed), 2015-02-03 20:37:50]

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

[erikwright (departed), 2015-02-03 20:37:50]

grt: PTAL.

Note that in the end we will not wait for the watcher to initialize at startup,
but, rather, immediately before reporting a crash. That's in a separate CL.

[grt (UTC plus 2), 2015-02-03 21:20:46]

mostly nits.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:31: PLOG(ERROR) << "Failed to create an
event.";
should any of these be debug-only? are they needed in release builds?

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:53: LOG(ERROR) << "EnsureInitialized
called after a failed LaunchWatcher "
is this documenting the API contract? why not just DCHECK(process_.IsValid());?

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:83: LOG(ERROR) << "WaitForExit called
after a failed LaunchWatcher call, or "
same comment as above

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:94: LOG(ERROR) <<
"WaitForExitWithTimeout called after a failed LaunchWatcher "
and here

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_client_win.h (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.h:56: bool WaitForExitWithTimeout(const
base::TimeDelta& timeout, int* exit_code);
i think TimeDelta is ordinarily passed by value (it's an int64_t).

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_command_line_win.cc (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.cc:21: void
AppendHandleSwitch(base::CommandLine* command_line,
base::CommandLine* command_line should be last since it's an in/out param

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.cc:31: unsigned switch_uint = 0;
"unsigned" -> "unsigned int"

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.cc:35: return NULL;
nullptr

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.cc:91: if
(!on_initialized_event->IsValid() || !parent_process->IsValid()) {
this function doesn't ensure that the two ScopedHandles provided by the caller
are not valid upon entry, so it isn't safe to rely on them being valid only if
set on line 75 and 87.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_command_line_win.h (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:10: #include
"base/win/scoped_handle.h"
can ScopedHandle be forward decl'd now?

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:19: // |on_initialized_event| when
its initialization is completed.
nit: "is complete" or "has completed"

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:22: HANDLE on_initialized_event,
based on the comment above, i'd swap on_initialized_event and parent_process.
makes more sense to me that way for some reason.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:32: base::win::ScopedHandle*
on_initialized_event,
i'd swap them here, too

[erikwright (departed), 2015-02-03 21:26:02]

grt: queries to you.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_command_line_win.h (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:22: HANDLE on_initialized_event,
On 2015/02/03 21:20:45, grt wrote:
> based on the comment above, i'd swap on_initialized_event and parent_process.
> makes more sense to me that way for some reason.

We end up binding this to |chrome_exe|, and later to |on_initialized_event|, to
generate a Callback that only takes a single HANDLE.

If I reorder the parameters I'll need at least one "jump function" to swap the
order.

Let me know if you think it's warranted.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:32: base::win::ScopedHandle*
on_initialized_event,
On 2015/02/03 21:20:45, grt wrote:
> i'd swap them here, too

Ordered for consistency with the above.

If you confirm the request to swap there I'll do it here too.

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

grt: PTAL. Another query, and changes according to your comments.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:31: PLOG(ERROR) << "Failed to create an
event.";
On 2015/02/03 21:20:44, grt wrote:
> should any of these be debug-only? are they needed in release builds?

Good question. I don't know. I googled for a policy on this and couldn't find
anything.

Certainly at least some of them could be useful in diagnosing failure in the
field using Sawbuck.

This one is exceptionally unlikely, so on the one hand it might as well be DPLOG
and on the other hand it's irrelevant since it will presumably never be reached.

WDYT?

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:53: LOG(ERROR) << "EnsureInitialized
called after a failed LaunchWatcher "
On 2015/02/03 21:20:44, grt wrote:
> is this documenting the API contract? why not just
DCHECK(process_.IsValid());?

In fact, it seems reasonable to call this even if it was never launched (i.e. to
save the caller the need to maintain that state separately).

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:83: LOG(ERROR) << "WaitForExit called
after a failed LaunchWatcher call, or "
On 2015/02/03 21:20:44, grt wrote:
> same comment as above

Done.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:94: LOG(ERROR) <<
"WaitForExitWithTimeout called after a failed LaunchWatcher "
On 2015/02/03 21:20:44, grt wrote:
> and here

Done.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_command_line_win.cc (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.cc:21: void
AppendHandleSwitch(base::CommandLine* command_line,
On 2015/02/03 21:20:45, grt wrote:
> base::CommandLine* command_line should be last since it's an in/out param

Done.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.cc:31: unsigned switch_uint = 0;
On 2015/02/03 21:20:45, grt wrote:
> "unsigned" -> "unsigned int"

Done.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.cc:35: return NULL;
On 2015/02/03 21:20:45, grt wrote:
> nullptr

Done.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.cc:91: if
(!on_initialized_event->IsValid() || !parent_process->IsValid()) {
On 2015/02/03 21:20:45, grt wrote:
> this function doesn't ensure that the two ScopedHandles provided by the caller
> are not valid upon entry, so it isn't safe to rely on them being valid only if
> set on line 75 and 87.

Added explicit Close calls as an expedient and consistent solution.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_command_line_win.h (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:10: #include
"base/win/scoped_handle.h"
On 2015/02/03 21:20:45, grt wrote:
> can ScopedHandle be forward decl'd now?

It's a typedef, so no.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:19: // |on_initialized_event| when
its initialization is completed.
On 2015/02/03 21:20:45, grt wrote:
> nit: "is complete" or "has completed"

Done.

[grt (UTC plus 2), 2015-02-04 03:04:48]

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:31: PLOG(ERROR) << "Failed to create an
event.";
On 2015/02/03 21:56:47, erikwright wrote:
> On 2015/02/03 21:20:44, grt wrote:
> > should any of these be debug-only? are they needed in release builds?
> 
> Good question. I don't know. I googled for a policy on this and couldn't find
> anything.
> 
> Certainly at least some of them could be useful in diagnosing failure in the
> field using Sawbuck.
> 
> This one is exceptionally unlikely, so on the one hand it might as well be
DPLOG
> and on the other hand it's irrelevant since it will presumably never be
reached.
> 
> WDYT?

Every LOG adds weight. If it will never be reached, it shouldn't be there.
Here's the "never use LOG" thread:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/nAiKeh-zLws/6wljl....
I still consider the installer a special case for the reasons I stated in the
thread. Outside of the installer, I think it's hard to make a case for LOG. On
the one hand, I love logging. I really, really love logging. On the other hand,
if every developer thought that their special snowflake required extensive
logging, Chrome would be huge and the logs would be impenetrable, so we might as
well not have logging and ask for repro cases rather than asking for logs.
You're welcome to disagree, but please scrutinize each LOG and decide whether or
not it'll ever happen, and whether or not you're ever likely to learn something
if it does happen. I'm also of the mind that we don't need verbose messages in
non-debug LOG statements since the emitted message will contain file+line, and
we have the source. Put the description in a comment rather than in a string
that gets built into chrome.dll. It's entirely possible that there is no
Chromium consensus around this, however, so take this as just like my opinion,
man.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_client_win.h (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.h:56: bool WaitForExitWithTimeout(const
base::TimeDelta& timeout, int* exit_code);
On 2015/02/03 21:20:45, grt wrote:
> i think TimeDelta is ordinarily passed by value (it's an int64_t).

my possibly flawed codesearch for uses of base::TimeDelta in .h files with and
without & also indicates that it's generally passed by value.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_command_line_win.h (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:10: #include
"base/win/scoped_handle.h"
On 2015/02/03 21:56:47, erikwright wrote:
> On 2015/02/03 21:20:45, grt wrote:
> > can ScopedHandle be forward decl'd now?
> 
> It's a typedef, so no.

Acknowledged.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:22: HANDLE on_initialized_event,
On 2015/02/03 21:26:02, erikwright wrote:
> On 2015/02/03 21:20:45, grt wrote:
> > based on the comment above, i'd swap on_initialized_event and
parent_process.
> > makes more sense to me that way for some reason.
> 
> We end up binding this to |chrome_exe|, and later to |on_initialized_event|,
to
> generate a Callback that only takes a single HANDLE.
> 
> If I reorder the parameters I'll need at least one "jump function" to swap the
> order.
> 
> Let me know if you think it's warranted.

It took me a really long time to understand what you meant. Is it possible to
make it clear in the comments in either
ChromeWatcherClient::CommandLineGenerator or here that this is meant to be a
WatcherClient::CommandLineGenerator with an extra arg? I can't think of anything
graceful at the moment. My apologies for asking you to do something vague
without a concrete suggestion. :-(

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
File chrome/app/chrome_watcher_client_unittest_win.cc (right):

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_unittest_win.cc:136: bool
WaitForResultWithTimeout(const base::TimeDelta& timeout, bool* result) {
TimeDelta by value

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_unittest_win.cc:198: bool SignalExit() { return
FALSE != ::SetEvent(exit_event_.Get()); }
i think any of the following are preferable since Chromium style doesn't believe
in putting the constant first in "if (foo == 5)" scenarios:
  return ::SetEvent(exit_event_.Get());
  return ::SetEvent(exit_event_.Get()) != FALSE;
  return !!::SetEvent(exit_event_.Get());

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_unittest_win.cc:202: bool SignalInitialize() {
here and elsewhere

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_win.cc:52: if (!process_.IsValid()) {
nit: no braces

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_win.cc:83: return
process_.WaitForExit(exit_code);
nit (here and below):
  return process_.IsValid() && ...;

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
File chrome/app/chrome_watcher_command_line_unittest_win.cc (right):

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_command_line_unittest_win.cc:16: base::ProcessHandle
current = nullptr;
  // Ownership of these handles is passed to the ScopedHandles by
InterpretChromeWatcherCommandLine.

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

grt: PTAL.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.cc:31: PLOG(ERROR) << "Failed to create an
event.";
On 2015/02/04 03:04:47, grt wrote:
> On 2015/02/03 21:56:47, erikwright wrote:
> > On 2015/02/03 21:20:44, grt wrote:
> > > should any of these be debug-only? are they needed in release builds?
> > 
> > Good question. I don't know. I googled for a policy on this and couldn't
find
> > anything.
> > 
> > Certainly at least some of them could be useful in diagnosing failure in the
> > field using Sawbuck.
> > 
> > This one is exceptionally unlikely, so on the one hand it might as well be
> DPLOG
> > and on the other hand it's irrelevant since it will presumably never be
> reached.
> > 
> > WDYT?
> 
> Every LOG adds weight. If it will never be reached, it shouldn't be there.
> Here's the "never use LOG" thread:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/nAiKeh-zLws/6wljl....
> I still consider the installer a special case for the reasons I stated in the
> thread. Outside of the installer, I think it's hard to make a case for LOG. On
> the one hand, I love logging. I really, really love logging. On the other
hand,
> if every developer thought that their special snowflake required extensive
> logging, Chrome would be huge and the logs would be impenetrable, so we might
as
> well not have logging and ask for repro cases rather than asking for logs.
> You're welcome to disagree, but please scrutinize each LOG and decide whether
or
> not it'll ever happen, and whether or not you're ever likely to learn
something
> if it does happen. I'm also of the mind that we don't need verbose messages in
> non-debug LOG statements since the emitted message will contain file+line, and
> we have the source. Put the description in a comment rather than in a string
> that gets built into chrome.dll. It's entirely possible that there is no
> Chromium consensus around this, however, so take this as just like my opinion,
> man.

Thanks for filling me in on the philosophy. It seems very sensible.

I kept one log message that I thought was key (that the watcher failed to
launch). All of the others I converted to debug-only.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_client_win.h (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_client_win.h:56: bool WaitForExitWithTimeout(const
base::TimeDelta& timeout, int* exit_code);
On 2015/02/04 03:04:47, grt wrote:
> On 2015/02/03 21:20:45, grt wrote:
> > i think TimeDelta is ordinarily passed by value (it's an int64_t).
> 
> my possibly flawed codesearch for uses of base::TimeDelta in .h files with and
> without & also indicates that it's generally passed by value.

Sorry, just overlooked this the first time.

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
File chrome/app/chrome_watcher_command_line_win.h (right):

https://codereview.chromium.org/886613002/diff/80001/chrome/app/chrome_watche...
chrome/app/chrome_watcher_command_line_win.h:22: HANDLE on_initialized_event,
On 2015/02/04 03:04:47, grt wrote:
> On 2015/02/03 21:26:02, erikwright wrote:
> > On 2015/02/03 21:20:45, grt wrote:
> > > based on the comment above, i'd swap on_initialized_event and
> parent_process.
> > > makes more sense to me that way for some reason.
> > 
> > We end up binding this to |chrome_exe|, and later to |on_initialized_event|,
> to
> > generate a Callback that only takes a single HANDLE.
> > 
> > If I reorder the parameters I'll need at least one "jump function" to swap
the
> > order.
> > 
> > Let me know if you think it's warranted.
> 
> It took me a really long time to understand what you meant. Is it possible to
> make it clear in the comments in either
> ChromeWatcherClient::CommandLineGenerator or here that this is meant to be a
> WatcherClient::CommandLineGenerator with an extra arg? I can't think of
anything
> graceful at the moment. My apologies for asking you to do something vague
> without a concrete suggestion. :-(

I reversed the order to make the public contract intuitive at the expense of a
minor detour in the implementation. I think that's the right choice.

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
File chrome/app/chrome_watcher_client_unittest_win.cc (right):

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_unittest_win.cc:136: bool
WaitForResultWithTimeout(const base::TimeDelta& timeout, bool* result) {
On 2015/02/04 03:04:47, grt wrote:
> TimeDelta by value

Done.

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_unittest_win.cc:198: bool SignalExit() { return
FALSE != ::SetEvent(exit_event_.Get()); }
On 2015/02/04 03:04:47, grt wrote:
> i think any of the following are preferable since Chromium style doesn't
believe
> in putting the constant first in "if (foo == 5)" scenarios:
>   return ::SetEvent(exit_event_.Get());
>   return ::SetEvent(exit_event_.Get()) != FALSE;
>   return !!::SetEvent(exit_event_.Get());

Done.

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_unittest_win.cc:202: bool SignalInitialize() {
On 2015/02/04 03:04:47, grt wrote:
> here and elsewhere

I didn't see any other elsewheres besides these two. Let me know if I've
overlooked something.

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
File chrome/app/chrome_watcher_client_win.cc (right):

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_win.cc:52: if (!process_.IsValid()) {
On 2015/02/04 03:04:48, grt wrote:
> nit: no braces

Done.

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_client_win.cc:83: return
process_.WaitForExit(exit_code);
On 2015/02/04 03:04:47, grt wrote:
> nit (here and below):
>   return process_.IsValid() && ...;

Done.

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
File chrome/app/chrome_watcher_command_line_unittest_win.cc (right):

https://codereview.chromium.org/886613002/diff/120001/chrome/app/chrome_watch...
chrome/app/chrome_watcher_command_line_unittest_win.cc:16: base::ProcessHandle
current = nullptr;
On 2015/02/04 03:04:48, grt wrote:
>   // Ownership of these handles is passed to the ScopedHandles by
> InterpretChromeWatcherCommandLine.

Done.

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

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

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

jochen: PTAL for build.gn

[jochen (gone - plz use gerrit), 2015-02-04 12:52:36]

lgtm

[grt (UTC plus 2), 2015-02-04 14:11:47]

lgtm

[erikwright (departed), 2015-02-04 14:22:44]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2015-02-04 14:23:16]

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

[commit-bot: I haz the power, 2015-02-04 15:19:38]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-04 15:21:10]

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