remote-debugging-port for guest sessions.

remote-debugging-port for guest sessions.

--remote-debugging-port flag is lost when guest sessions are started on
chromeos.

BUG=None

Committed: https://crrev.com/7268c3d1f3f991740f536d9680426251392b98e8
Cr-Commit-Position: refs/heads/master@{#405633}

[achuithb, 2016-07-13 16:46:51]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[achuithb, 2016-07-13 16:47:07]

achuith@chromium.org changed reviewers:
+ emaxx@chromium.org

[achuithb, 2016-07-13 16:47:07]

Max, please take a look

[commit-bot: I haz the power, 2016-07-13 16:47:31]

Dry run: 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-07-13 17:28:41]

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

[commit-bot: I haz the power, 2016-07-13 17:28:42]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-07-13 18:25:40]

https://codereview.chromium.org/2141303003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/2141303003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/chrome_restart_request.cc:141:
::switches::kRemoteDebuggingPort,
I'm wondering whether a race between the current Chrome process and the newly
started Chrome process would become possible.
Because probably the dev tools would just fail to start listening on a port if
it's already in use.

And, if this really may happen, I'm not sure how to fix this properly. I don't
see in the RestartChrome function any currently existing place for putting the
necessary cleanup _before_ launching the new process.

[achuithb, 2016-07-14 09:21:14]

https://codereview.chromium.org/2141303003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/2141303003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/chrome_restart_request.cc:141:
::switches::kRemoteDebuggingPort,
On 2016/07/13 18:25:40, emaxx wrote:
> I'm wondering whether a race between the current Chrome process and the newly
> started Chrome process would become possible.
> Because probably the dev tools would just fail to start listening on a port if
> it's already in use.

It shouldn't be in use though right? I'm assuming chrome shutdown properly
closes the socket. Also, this seems to be no different than restarting the
browser using the same port, which is something I do frequently without seeing
any problems.


> And, if this really may happen, I'm not sure how to fix this properly. I don't
> see in the RestartChrome function any currently existing place for putting the
> necessary cleanup _before_ launching the new process.

In practice, this seems to work ok in my testing. 

--remote-debugging-port is only used for testing, and this change is chromeos
specific, so even if there are some cases where this doesn't work perfectly,
it's still an improvement for debugging guest sessions.

The teardown sequence closes the remote debugging server here:
https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl.cc?l...

I'm not able to find the exact location where the socket is closed but I could
look into it if you have a strong concern that it is not cleanly closed.

[emaxx, 2016-07-14 18:27:03]

lgtm

https://codereview.chromium.org/2141303003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/2141303003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/chrome_restart_request.cc:141:
::switches::kRemoteDebuggingPort,
On 2016/07/14 09:21:14, achuithb wrote:
> On 2016/07/13 18:25:40, emaxx wrote:
> > I'm wondering whether a race between the current Chrome process and the
newly
> > started Chrome process would become possible.
> > Because probably the dev tools would just fail to start listening on a port
if
> > it's already in use.
> 
> It shouldn't be in use though right? I'm assuming chrome shutdown properly
> closes the socket. Also, this seems to be no different than restarting the
> browser using the same port, which is something I do frequently without seeing
> any problems.
> 
> 
> > And, if this really may happen, I'm not sure how to fix this properly. I
don't
> > see in the RestartChrome function any currently existing place for putting
the
> > necessary cleanup _before_ launching the new process.
> 
> In practice, this seems to work ok in my testing. 
> 
> --remote-debugging-port is only used for testing, and this change is chromeos
> specific, so even if there are some cases where this doesn't work perfectly,
> it's still an improvement for debugging guest sessions.
> 
> The teardown sequence closes the remote debugging server here:
>
https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl.cc?l...
> 
> I'm not able to find the exact location where the socket is closed but I could
> look into it if you have a strong concern that it is not cleanly closed.

OK, I see: as this code (the GetOffTheRecordCommandLine method) is actually
relevant only for Chrome OS, we have only two cases to consider: the real Chrome
OS and the development "chromeos=1" build.
In the former case, seems that the session manager actually waits until the old
browser process is terminated until opening the new one (so there is no race
condition).
As for the latter case, I'm still thinking there's some probability of having
issues.
But I don't want to block the CL just because of some potential (and not really
harmful) problem in the dev environment.

[achuithb, 2016-07-14 22:45:48]

On 2016/07/14 18:27:03, emaxx wrote:
> lgtm
> 
>
https://codereview.chromium.org/2141303003/diff/1/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/chrome_restart_request.cc (right):
> 
>
https://codereview.chromium.org/2141303003/diff/1/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/chrome_restart_request.cc:141:
> ::switches::kRemoteDebuggingPort,
> On 2016/07/14 09:21:14, achuithb wrote:
> > On 2016/07/13 18:25:40, emaxx wrote:
> > > I'm wondering whether a race between the current Chrome process and the
> newly
> > > started Chrome process would become possible.
> > > Because probably the dev tools would just fail to start listening on a
port
> if
> > > it's already in use.
> > 
> > It shouldn't be in use though right? I'm assuming chrome shutdown properly
> > closes the socket. Also, this seems to be no different than restarting the
> > browser using the same port, which is something I do frequently without
seeing
> > any problems.
> > 
> > 
> > > And, if this really may happen, I'm not sure how to fix this properly. I
> don't
> > > see in the RestartChrome function any currently existing place for putting
> the
> > > necessary cleanup _before_ launching the new process.
> > 
> > In practice, this seems to work ok in my testing. 
> > 
> > --remote-debugging-port is only used for testing, and this change is
chromeos
> > specific, so even if there are some cases where this doesn't work perfectly,
> > it's still an improvement for debugging guest sessions.
> > 
> > The teardown sequence closes the remote debugging server here:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl.cc?l...
> > 
> > I'm not able to find the exact location where the socket is closed but I
could
> > look into it if you have a strong concern that it is not cleanly closed.
> 
> OK, I see: as this code (the GetOffTheRecordCommandLine method) is actually
> relevant only for Chrome OS, we have only two cases to consider: the real
Chrome
> OS and the development "chromeos=1" build.
> In the former case, seems that the session manager actually waits until the
old
> browser process is terminated until opening the new one (so there is no race
> condition).
> As for the latter case, I'm still thinking there's some probability of having
> issues.
> But I don't want to block the CL just because of some potential (and not
really
> harmful) problem in the dev environment.

Thanks!

[achuithb, 2016-07-14 22:45:59]

The CQ bit was checked by achuith@chromium.org

[commit-bot: I haz the power, 2016-07-14 22:46:45]

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-07-14 23:48:47]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-14 23:48:49]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 23:51:05]

Description was changed from

==========
remote-debugging-port for guest sessions.

--remote-debugging-port flag is lost when guest sessions are started on
chromeos.

BUG=None
==========

to

==========
remote-debugging-port for guest sessions.

--remote-debugging-port flag is lost when guest sessions are started on
chromeos.

BUG=None

Committed: https://crrev.com/7268c3d1f3f991740f536d9680426251392b98e8
Cr-Commit-Position: refs/heads/master@{#405633}
==========

[commit-bot: I haz the power, 2016-07-14 23:51:06]

Patchset 1 (id:??) landed as
https://crrev.com/7268c3d1f3f991740f536d9680426251392b98e8
Cr-Commit-Position: refs/heads/master@{#405633}

[achuithb, 2016-07-27 00:17:15]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2189503002/ by achuith@chromium.org.

The reason for reverting is: Speculative revert for crbug.com/631640.