chromeos: Remove dbus::FileDescriptor from SessionManagerClient

chromeos: Remove dbus::FileDescriptor from SessionManagerClient

Replace dbus::FileDescriptor with base::ScopedFD.
Move socket related code to ChromeRestartRequest to make SessionManagerClient a simple D-Bus binding class.

BUG=621841
TEST="Browse as Guest" on login screen works.

Committed: https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5
Cr-Commit-Position: refs/heads/master@{#418194}

[hashimoto, 2016-09-06 06:34:23]

Description was changed from

==========
chromeos: Remove dbus::FileDescriptor from SessionManagerClient

BUG=
==========

to

==========
chromeos: Remove dbus::FileDescriptor from SessionManagerClient

Replace dbus::FileDescriptor with base::ScopedFD.
Move socket related code to ChromeRestartRequest to make SessionManagerClient a
simple D-Bus binding class.

BUG=621841
TEST="Browse as Guest" on login screen works.
==========

[hashimoto, 2016-09-06 06:37:30]

hashimoto@chromium.org changed reviewers:
+ satorux@chromium.org, stevenjb@chromium.org

[hashimoto, 2016-09-06 06:37:31]

cc: achuith in case this change conflicts with your work on crbug.com/631640

[hashimoto, 2016-09-06 06:37:40]

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

[commit-bot: I haz the power, 2016-09-06 06:37:55]

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-09-06 07:14:55]

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

[commit-bot: I haz the power, 2016-09-06 07:14:56]

Dry run: This issue passed the CQ dry run.

[satorux1, 2016-09-06 07:31:58]

satorux@chromium.org changed reviewers:
+ derat@chromium.org

[satorux1, 2016-09-06 07:31:58]

Moving socket related code to ChromeRestartRequest SGTM. +derat who'd be
familiar with the session manager business.

[Daniel Erat, 2016-09-06 15:11:06]

lgtm

(i'm not really all that familiar with session_manager stuff; i'm just one of
the owners of the code now. :-P)

[stevenjb, 2016-09-06 15:59:28]

https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/chrome_restart_request.cc:329: if
(socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) {
I'm not familiar with socketpair(), is this safe to run on the UI thread?

https://codereview.chromium.org/2310823003/diff/20001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.cc (left):

https://codereview.chromium.org/2310823003/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.cc:151: FROM_HERE,
create_credentials_conduit_closure,
It looks like before we were calling socketpair() from a worker pool thread,
which seems more correct?

[achuithb, 2016-09-06 16:11:31]

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

[achuithb, 2016-09-06 16:11:32]

+1 to Steven's concern about socket_pair. Otherwise this should be fine as far
as 631640 goes.

[hashimoto, 2016-09-07 06:05:46]

https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/chrome_restart_request.cc:293:
g_browser_process->EndSession();
FYI: BrowserProcessImpl::EndSession() may block the UI thread for at most 10
seconds to write preferences and the local state.
https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl.cc?l...

https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/chrome_restart_request.cc:329: if
(socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) {
On 2016/09/06 15:59:28, stevenjb wrote:
> I'm not familiar with socketpair(), is this safe to run on the UI thread?

AssertIOAllowed fails on the UI thread, and it's meant to avoid blocking the UI
thread waiting for a disk which is usually much slower than CPU and RAM.
IIUC using socketpair() here is OK because:
 1. socketpair() shouldn't result in disk IO.
 2. Here we're about to kill this process to start a new Chrome process. So
having janky UI here shouldn't matter much. At least,
ChromeRestartRequest::Start() is already calling BrowserProcessImpl::EndSession
on the UI thread.

WDYT?

[stevenjb, 2016-09-08 16:40:11]

https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/chrome_restart_request.cc:329: if
(socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) {
On 2016/09/07 06:05:46, hashimoto wrote:
> On 2016/09/06 15:59:28, stevenjb wrote:
> > I'm not familiar with socketpair(), is this safe to run on the UI thread?
> 
> AssertIOAllowed fails on the UI thread, and it's meant to avoid blocking the
UI
> thread waiting for a disk which is usually much slower than CPU and RAM.
> IIUC using socketpair() here is OK because:
>  1. socketpair() shouldn't result in disk IO.
>  2. Here we're about to kill this process to start a new Chrome process. So
> having janky UI here shouldn't matter much. At least,
> ChromeRestartRequest::Start() is already calling
BrowserProcessImpl::EndSession
> on the UI thread.
> 
> WDYT?

AssertIOAllowed will fail in places where we added an assert. If socketpair() is
a system call then it won't have an assert wrapper, will it? (I'm not at all
familiar with it).

socketpair() *shouldn't* result in disk IO or *won't*? If it *won't* than I am
OK with it here, but if we are uncertain then in principal I think we should use
a worker thread.

That said, if we are only ever calling this from code where we might be blocking
on the UI thread anyway, then this is probably fine, but we should have a
comment explaining that it is OK.

[hashimoto, 2016-09-09 03:41:46]

On 2016/09/08 16:40:11, stevenjb wrote:
>
https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/chrome_restart_request.cc (right):
> 
>
https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/chrome_restart_request.cc:329: if
> (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) {
> On 2016/09/07 06:05:46, hashimoto wrote:
> > On 2016/09/06 15:59:28, stevenjb wrote:
> > > I'm not familiar with socketpair(), is this safe to run on the UI thread?
> > 
> > AssertIOAllowed fails on the UI thread, and it's meant to avoid blocking the
> UI
> > thread waiting for a disk which is usually much slower than CPU and RAM.
> > IIUC using socketpair() here is OK because:
> >  1. socketpair() shouldn't result in disk IO.
> >  2. Here we're about to kill this process to start a new Chrome process. So
> > having janky UI here shouldn't matter much. At least,
> > ChromeRestartRequest::Start() is already calling
> BrowserProcessImpl::EndSession
> > on the UI thread.
> > 
> > WDYT?
> 
> AssertIOAllowed will fail in places where we added an assert. If socketpair()
is
> a system call then it won't have an assert wrapper, will it? (I'm not at all
> familiar with it).
Sorry for being unclear.
What I meant was that if we were to add a wrapper function for socketpair(),
there wouldn't be AssertIOAllowed().

> 
> socketpair() *shouldn't* result in disk IO or *won't*? If it *won't* than I am
> OK with it here, but if we are uncertain then in principal I think we should
use
> a worker thread.
It won't. The existing code was posting a task to a worker because it was
calling dbus::FileDescriptor::CheckValidity() which has AssertIOAllowed().
> 
> That said, if we are only ever calling this from code where we might be
blocking
> on the UI thread anyway, then this is probably fine, but we should have a
> comment explaining that it is OK.

[stevenjb, 2016-09-12 16:14:27]

On 2016/09/09 03:41:46, hashimoto wrote:
> On 2016/09/08 16:40:11, stevenjb wrote:
> >
>
https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
> > File chrome/browser/chromeos/login/chrome_restart_request.cc (right):
> > 
> >
>
https://codereview.chromium.org/2310823003/diff/20001/chrome/browser/chromeos...
> > chrome/browser/chromeos/login/chrome_restart_request.cc:329: if
> > (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) {
> > On 2016/09/07 06:05:46, hashimoto wrote:
> > > On 2016/09/06 15:59:28, stevenjb wrote:
> > > > I'm not familiar with socketpair(), is this safe to run on the UI
thread?
> > > 
> > > AssertIOAllowed fails on the UI thread, and it's meant to avoid blocking
the
> > UI
> > > thread waiting for a disk which is usually much slower than CPU and RAM.
> > > IIUC using socketpair() here is OK because:
> > >  1. socketpair() shouldn't result in disk IO.
> > >  2. Here we're about to kill this process to start a new Chrome process.
So
> > > having janky UI here shouldn't matter much. At least,
> > > ChromeRestartRequest::Start() is already calling
> > BrowserProcessImpl::EndSession
> > > on the UI thread.
> > > 
> > > WDYT?
> > 
> > AssertIOAllowed will fail in places where we added an assert. If
socketpair()
> is
> > a system call then it won't have an assert wrapper, will it? (I'm not at all
> > familiar with it).
> Sorry for being unclear.
> What I meant was that if we were to add a wrapper function for socketpair(),
> there wouldn't be AssertIOAllowed().
> 
> > 
> > socketpair() *shouldn't* result in disk IO or *won't*? If it *won't* than I
am
> > OK with it here, but if we are uncertain then in principal I think we should
> use
> > a worker thread.
> It won't. The existing code was posting a task to a worker because it was
> calling dbus::FileDescriptor::CheckValidity() which has AssertIOAllowed().
> > 
> > That said, if we are only ever calling this from code where we might be
> blocking
> > on the UI thread anyway, then this is probably fine, but we should have a
> > comment explaining that it is OK.

If it won't block or we don't care that is fine, but this is not clear by
inspection, we need to add a comment.

[stevenjb, 2016-09-12 16:15:19]

Also, lgtm with a comment added above the call to socketpair() explaining that
it is OK to call that from the UI thread.

[achuithb, 2016-09-12 18:17:42]

On 2016/09/12 16:15:19, stevenjb wrote:
> Also, lgtm with a comment added above the call to socketpair() explaining that
> it is OK to call that from the UI thread.

lgtm

[hashimoto, 2016-09-13 07:27:27]

On 2016/09/12 18:17:42, achuithb wrote:
> On 2016/09/12 16:15:19, stevenjb wrote:
> > Also, lgtm with a comment added above the call to socketpair() explaining
that
> > it is OK to call that from the UI thread.
Done.
> 
> lgtm

[hashimoto, 2016-09-13 07:27:32]

The CQ bit was checked by hashimoto@chromium.org

[hashimoto, 2016-09-13 07:27:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, derat@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2310823003/#ps40001
(title: "Add a comment")

[commit-bot: I haz the power, 2016-09-13 07:27:47]

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-09-13 08:17:54]

Description was changed from

==========
chromeos: Remove dbus::FileDescriptor from SessionManagerClient

Replace dbus::FileDescriptor with base::ScopedFD.
Move socket related code to ChromeRestartRequest to make SessionManagerClient a
simple D-Bus binding class.

BUG=621841
TEST="Browse as Guest" on login screen works.
==========

to

==========
chromeos: Remove dbus::FileDescriptor from SessionManagerClient

Replace dbus::FileDescriptor with base::ScopedFD.
Move socket related code to ChromeRestartRequest to make SessionManagerClient a
simple D-Bus binding class.

BUG=621841
TEST="Browse as Guest" on login screen works.
==========

[commit-bot: I haz the power, 2016-09-13 08:17:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-13 08:19:55]

Description was changed from

==========
chromeos: Remove dbus::FileDescriptor from SessionManagerClient

Replace dbus::FileDescriptor with base::ScopedFD.
Move socket related code to ChromeRestartRequest to make SessionManagerClient a
simple D-Bus binding class.

BUG=621841
TEST="Browse as Guest" on login screen works.
==========

to

==========
chromeos: Remove dbus::FileDescriptor from SessionManagerClient

Replace dbus::FileDescriptor with base::ScopedFD.
Move socket related code to ChromeRestartRequest to make SessionManagerClient a
simple D-Bus binding class.

BUG=621841
TEST="Browse as Guest" on login screen works.

Committed: https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5
Cr-Commit-Position: refs/heads/master@{#418194}
==========

[commit-bot: I haz the power, 2016-09-13 08:19:56]

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