Fixing an AttachmentBroker problem which broke the windows host

Fixing an AttachmentBroker problem which broke the windows host

A previous change did some cleanup in the remoting codebase however it caused
a crash when we tried to spin up the desktop process:
https://codereview.chromium.org/1903663004/

The issue was that the code in the ipc_utils_win file attempted to register a
broker but did not check to see if one was registered first.  The
DesktopProcess class did register a broker, but did so after this utility
function was called.

BUG=607313
Committed: https://crrev.com/3d4a0c92b0079c32e777933a52b6d8eaf32d0c6e
Cr-Commit-Position: refs/heads/master@{#390653}

[joedow, 2016-04-28 20:33:11]

joedow@chromium.org changed reviewers:
+ erikchen@chromium.org

[joedow, 2016-04-28 20:33:12]

Hi Erik,

Can you take a look at this change involving AttachmentBroker usage in remoting?
 Your recent change caused a regression which I've fixed but would like you to
take a look to make sure I didn't regress anything in your change.

Thanks!
Joe

[erikchen, 2016-04-28 23:12:51]

https://codereview.chromium.org/1925263002/diff/20001/remoting/host/ipc_util_...
File remoting/host/ipc_util_win.cc (right):

https://codereview.chromium.org/1925263002/diff/20001/remoting/host/ipc_util_...
remoting/host/ipc_util_win.cc:64: LOG(WARNING) << "No AttachmentBroker
registered.";
Every process should have a broker. When does this LOG statement get emitted?

[erikchen, 2016-04-28 23:13:05]

note that this will also have to be merged to M51.

[joedow, 2016-04-28 23:20:50]

https://codereview.chromium.org/1925263002/diff/20001/remoting/host/ipc_util_...
File remoting/host/ipc_util_win.cc (right):

https://codereview.chromium.org/1925263002/diff/20001/remoting/host/ipc_util_...
remoting/host/ipc_util_win.cc:64: LOG(WARNING) << "No AttachmentBroker
registered.";
On 2016/04/28 23:12:50, erikchen wrote:
> Every process should have a broker. When does this LOG statement get emitted?

It occurred after you made your last change and I had to spend more time than I
would have liked tracking it down.  With my change I suppose now the message
will fail (which should be logged) instead of crashing.  I like having the
warning because in our case, we were spinning up a process on a TS session and
there was no debug output as to why the connection was failing.  This is a big
clue which should save them time.

I'd like to leave it as this would have saved me time tracking this issue down.

[erikchen, 2016-04-28 23:24:54]

https://codereview.chromium.org/1925263002/diff/20001/remoting/host/ipc_util_...
File remoting/host/ipc_util_win.cc (right):

https://codereview.chromium.org/1925263002/diff/20001/remoting/host/ipc_util_...
remoting/host/ipc_util_win.cc:64: LOG(WARNING) << "No AttachmentBroker
registered.";
On 2016/04/28 23:20:49, joedow wrote:
> On 2016/04/28 23:12:50, erikchen wrote:
> > Every process should have a broker. When does this LOG statement get
emitted?
> 
> It occurred after you made your last change and I had to spend more time than
I
> would have liked tracking it down.  With my change I suppose now the message
> will fail (which should be logged) instead of crashing.  I like having the
> warning because in our case, we were spinning up a process on a TS session and
> there was no debug output as to why the connection was failing.  This is a big
> clue which should save them time.
> 
> I'd like to leave it as this would have saved me time tracking this issue
down.

Would making this a DCHECK(broker) or even a CHECK(broker) satisfy your
constraint for making future problems easy to debug? The presence of a
conditional/log statement implies that this is a valid state for the system
[which it isn't].

[joedow, 2016-04-28 23:39:26]

https://codereview.chromium.org/1925263002/diff/20001/remoting/host/ipc_util_...
File remoting/host/ipc_util_win.cc (right):

https://codereview.chromium.org/1925263002/diff/20001/remoting/host/ipc_util_...
remoting/host/ipc_util_win.cc:64: LOG(WARNING) << "No AttachmentBroker
registered.";
On 2016/04/28 23:24:54, erikchen wrote:
> On 2016/04/28 23:20:49, joedow wrote:
> > On 2016/04/28 23:12:50, erikchen wrote:
> > > Every process should have a broker. When does this LOG statement get
> emitted?
> > 
> > It occurred after you made your last change and I had to spend more time
than
> I
> > would have liked tracking it down.  With my change I suppose now the message
> > will fail (which should be logged) instead of crashing.  I like having the
> > warning because in our case, we were spinning up a process on a TS session
and
> > there was no debug output as to why the connection was failing.  This is a
big
> > clue which should save them time.
> > 
> > I'd like to leave it as this would have saved me time tracking this issue
> down.
> 
> Would making this a DCHECK(broker) or even a CHECK(broker) satisfy your
> constraint for making future problems easy to debug? The presence of a
> conditional/log statement implies that this is a valid state for the system
> [which it isn't].

DCHECK sounds legit.

[erikchen, 2016-04-28 23:40:51]

lgtm

[joedow, 2016-04-28 23:57:21]

The CQ bit was checked by joedow@chromium.org

[commit-bot: I haz the power, 2016-04-28 23:57:47]

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

[commit-bot: I haz the power, 2016-04-29 01:26:34]

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

[commit-bot: I haz the power, 2016-04-29 01:26:35]

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

[joedow, 2016-04-29 15:14:42]

The CQ bit was checked by joedow@chromium.org

[commit-bot: I haz the power, 2016-04-29 15:15:03]

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

[commit-bot: I haz the power, 2016-04-29 15:19:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-30 17:25:57]

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

[commit-bot: I haz the power, 2016-04-30 18:00:43]

Description was changed from

==========
Fixing an AttachmentBroker problem which broke the windows host

A previous change did some cleanup in the remoting codebase however it caused
a crash when we tried to spin up the desktop process:
https://codereview.chromium.org/1903663004/

The issue was that the code in the ipc_utils_win file attempted to register a
broker but did not check to see if one was registered first.  The
DesktopProcess class did register a broker, but did so after this utility
function was called.

BUG=607313
==========

to

==========
Fixing an AttachmentBroker problem which broke the windows host

A previous change did some cleanup in the remoting codebase however it caused
a crash when we tried to spin up the desktop process:
https://codereview.chromium.org/1903663004/

The issue was that the code in the ipc_utils_win file attempted to register a
broker but did not check to see if one was registered first.  The
DesktopProcess class did register a broker, but did so after this utility
function was called.

BUG=607313

Committed: https://crrev.com/3d4a0c92b0079c32e777933a52b6d8eaf32d0c6e
Cr-Commit-Position: refs/heads/master@{#390653}
==========