Issue 2436763004: More graceful shutdown for ArcSession.

hidehiko

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

4 years, 2 months ago (2016-10-20 09:29:37 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2436763004/1

4 years, 2 months ago (2016-10-20 09:29:45 UTC) #2

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-20 10:01:40 UTC) #3

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-20 10:01:41 UTC) #4

hidehiko

hidehiko@chromium.org changed reviewers: + bartfab@chromium.org, lhchavez@chromium.org

4 years, 2 months ago (2016-10-20 10:09:08 UTC) #5

hidehiko

PTAL. lhchavez@: all. bartfab@: chrome/browser/policy/policy_browsertest.cc as an OWNER. Luis, this is the CL to follow ...

4 years, 2 months ago (2016-10-20 10:09:08 UTC) #6

bartfab (slow)

policy_browsertest.cc LGTM https://codereview.chromium.org/2436763004/diff/1/chrome/browser/policy/policy_browsertest.cc File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2436763004/diff/1/chrome/browser/policy/policy_browsertest.cc#newcode4193 chrome/browser/policy/policy_browsertest.cc:4193: // Here inject FakeArcSession so blocking task ...

4 years, 2 months ago (2016-10-20 16:16:24 UTC) #7

Luis Héctor Chávez

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_bridge_service.h File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_bridge_service.h#newcode117 components/arc/arc_bridge_service.h:117: virtual void OnShutdown() = 0; Can you add a ...

4 years, 2 months ago (2016-10-21 00:43:42 UTC) #8

hidehiko

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

4 years, 2 months ago (2016-10-21 04:27:13 UTC) #9

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2436763004/40001

4 years, 2 months ago (2016-10-21 04:27:30 UTC) #10

hidehiko

Thank you for review. PTAL. https://codereview.chromium.org/2436763004/diff/1/chrome/browser/policy/policy_browsertest.cc File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2436763004/diff/1/chrome/browser/policy/policy_browsertest.cc#newcode4193 chrome/browser/policy/policy_browsertest.cc:4193: // Here inject FakeArcSession ...

4 years, 2 months ago (2016-10-21 04:34:04 UTC) #11

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-21 05:19:04 UTC) #12

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-21 05:19:05 UTC) #13

Luis Héctor Chávez

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_bridge_service.h File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_bridge_service.h#newcode117 components/arc/arc_bridge_service.h:117: virtual void OnShutdown() = 0; On 2016/10/21 04:34:03, hidehiko ...

4 years, 2 months ago (2016-10-21 06:34:20 UTC) #14

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.h:117: virtual void OnShutdown() = 0;
On 2016/10/21 04:34:03, hidehiko wrote:
> On 2016/10/21 00:43:41, Luis Héctor Chávez wrote:
> > Can you add a simple unit test that uses this? I'd like to exercise the
DCHECK
> > on the destructor a bit more.
> 
> Added. But which DCHECK did you mean?
> 
> If the one in ArcSession, unfortunately current we have no good way to test
it.
> We need more refactoring for testing. I'm happy to do that, if it looks good
to
> you, but let me work on it in a separate CL considering the change size.

The current unit test here looks good enough for the purpose of this change. If
there is a way to add better testing in a different change, I'm all up for it.

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session.cc
File components/arc/arc_session.cc (right):

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session....
components/arc/arc_session.cc:602: // After Chrome's shutdown, SessionManager
shuts down the ARC instance,
On 2016/10/21 04:34:03, hidehiko wrote:
> On 2016/10/21 00:43:41, Luis Héctor Chávez wrote:
> > I'd still want this code to be robust by itself and not depend on
> SessionManager
> > automatically cleaning stuff up. Can you still call StopArcInstance() if the
> > state indicates that the container is running? But please conserve the
comment
> > below that the UI thread is gone so OnStopped must be explicitly called.
> 
> Could you tell me what more robustness do you want to ensure here?
> Or, do you think we cannot rely on SessionManager's behavior?
> 
> CC'ed hashimoto@, the DBus expert. He may give us more advice.

We can rely on SessionManager's _current_ behavior (since both the explicit
shutting down of the container and the browser exiting code paths have the same
container-terminating code), but I'd rather be explicit if we can, in case we
need to change its behavior. e.g. at some point there were plans of giving
Android a bit more time to shutdown, so that code might diverge slightly in the
future.

The only thing we cannot rely on is having the callback dispatched due to the
lack of a message loop, but the outgoing call should be able to get through
without any issues.

hashimoto

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

4 years, 2 months ago (2016-10-21 07:20:26 UTC) #15

hashimoto

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session.cc File components/arc/arc_session.cc (right): https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session.cc#newcode602 components/arc/arc_session.cc:602: // After Chrome's shutdown, SessionManager shuts down the ARC ...

4 years, 2 months ago (2016-10-21 07:20:27 UTC) #16

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session.cc
File components/arc/arc_session.cc (right):

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session....
components/arc/arc_session.cc:602: // After Chrome's shutdown, SessionManager
shuts down the ARC instance,
On 2016/10/21 06:34:20, Luis Héctor Chávez wrote:
> On 2016/10/21 04:34:03, hidehiko wrote:
> > On 2016/10/21 00:43:41, Luis Héctor Chávez wrote:
> > > I'd still want this code to be robust by itself and not depend on
> > SessionManager
> > > automatically cleaning stuff up. Can you still call StopArcInstance() if
the
> > > state indicates that the container is running? But please conserve the
> comment
> > > below that the UI thread is gone so OnStopped must be explicitly called.
> > 
> > Could you tell me what more robustness do you want to ensure here?
> > Or, do you think we cannot rely on SessionManager's behavior?
> > 
> > CC'ed hashimoto@, the DBus expert. He may give us more advice.
> 
> We can rely on SessionManager's _current_ behavior (since both the explicit
> shutting down of the container and the browser exiting code paths have the
same
> container-terminating code), but I'd rather be explicit if we can, in case we
> need to change its behavior. e.g. at some point there were plans of giving
> Android a bit more time to shutdown, so that code might diverge slightly in
the
> future.
> 
> The only thing we cannot rely on is having the callback dispatched due to the
> lack of a message loop, but the outgoing call should be able to get through
> without any issues.
IIUC unclean shutdown is also covered by SessionManagerService::HandleExit.
(https://chromium.googlesource.com/chromiumos/platform2/+/master/login_manager...)
I think SessionManager must continue shutting down the container whenever Chrome
dies, because Chrome can die in ways other than clean shutdown (e.g. crash, or
getting killed by SessionManager), but the container must be killed before
Chrome gets restarted.

Also, SessionManagerService::HandleExit() gives the container 3 seconds to
shutdown, while SessionManagerImpl::StopArcInstance() only gives it 1 second.
Although I don't think this difference was introduced intentionally, IIUC
calling StopArcInstance during Chrome shutdown results in unclean container
shutdown happening more frequently.

Luis Héctor Chávez

lgtm. as we discussed offline, please add the explicit shutdown call if possible. https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session.cc File ...

4 years, 2 months ago (2016-10-21 07:53:43 UTC) #17

lgtm. as we discussed offline, please add the explicit shutdown call if
possible.

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session.cc
File components/arc/arc_session.cc (right):

https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session....
components/arc/arc_session.cc:602: // After Chrome's shutdown, SessionManager
shuts down the ARC instance,
On 2016/10/21 07:20:27, hashimoto wrote:
> On 2016/10/21 06:34:20, Luis Héctor Chávez wrote:
> > On 2016/10/21 04:34:03, hidehiko wrote:
> > > On 2016/10/21 00:43:41, Luis Héctor Chávez wrote:
> > > > I'd still want this code to be robust by itself and not depend on
> > > SessionManager
> > > > automatically cleaning stuff up. Can you still call StopArcInstance() if
> the
> > > > state indicates that the container is running? But please conserve the
> > comment
> > > > below that the UI thread is gone so OnStopped must be explicitly called.
> > > 
> > > Could you tell me what more robustness do you want to ensure here?
> > > Or, do you think we cannot rely on SessionManager's behavior?
> > > 
> > > CC'ed hashimoto@, the DBus expert. He may give us more advice.
> > 
> > We can rely on SessionManager's _current_ behavior (since both the explicit
> > shutting down of the container and the browser exiting code paths have the
> same
> > container-terminating code), but I'd rather be explicit if we can, in case
we
> > need to change its behavior. e.g. at some point there were plans of giving
> > Android a bit more time to shutdown, so that code might diverge slightly in
> the
> > future.
> > 
> > The only thing we cannot rely on is having the callback dispatched due to
the
> > lack of a message loop, but the outgoing call should be able to get through
> > without any issues.
> IIUC unclean shutdown is also covered by SessionManagerService::HandleExit.
>
(https://chromium.googlesource.com/chromiumos/platform2/+/master/login_manager...)
> I think SessionManager must continue shutting down the container whenever
Chrome
> dies, because Chrome can die in ways other than clean shutdown (e.g. crash, or
> getting killed by SessionManager), but the container must be killed before
> Chrome gets restarted.
> 
> Also, SessionManagerService::HandleExit() gives the container 3 seconds to
> shutdown, while SessionManagerImpl::StopArcInstance() only gives it 1 second.
> Although I don't think this difference was introduced intentionally, IIUC
> calling StopArcInstance during Chrome shutdown results in unclean container
> shutdown happening more frequently.

Oh that was unintentional :/ we should swap the timeouts in session_manager, so
that the clean exit is 3s and the non-clean shutdown is just 1.

hidehiko

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

4 years, 2 months ago (2016-10-21 07:54:30 UTC) #18

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2436763004/60001

4 years, 2 months ago (2016-10-21 07:54:47 UTC) #19

hidehiko

On 2016/10/21 07:53:43, Luis Héctor Chávez wrote: > lgtm. as we discussed offline, please add ...

4 years, 2 months ago (2016-10-21 07:57:34 UTC) #20

On 2016/10/21 07:53:43, Luis Héctor Chávez wrote:
> lgtm. as we discussed offline, please add the explicit shutdown call if
> possible.
> 
>
https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session.cc
> File components/arc/arc_session.cc (right):
> 
>
https://codereview.chromium.org/2436763004/diff/1/components/arc/arc_session....
> components/arc/arc_session.cc:602: // After Chrome's shutdown, SessionManager
> shuts down the ARC instance,
> On 2016/10/21 07:20:27, hashimoto wrote:
> > On 2016/10/21 06:34:20, Luis Héctor Chávez wrote:
> > > On 2016/10/21 04:34:03, hidehiko wrote:
> > > > On 2016/10/21 00:43:41, Luis Héctor Chávez wrote:
> > > > > I'd still want this code to be robust by itself and not depend on
> > > > SessionManager
> > > > > automatically cleaning stuff up. Can you still call StopArcInstance()
if
> > the
> > > > > state indicates that the container is running? But please conserve the
> > > comment
> > > > > below that the UI thread is gone so OnStopped must be explicitly
called.
> > > > 
> > > > Could you tell me what more robustness do you want to ensure here?
> > > > Or, do you think we cannot rely on SessionManager's behavior?
> > > > 
> > > > CC'ed hashimoto@, the DBus expert. He may give us more advice.
> > > 
> > > We can rely on SessionManager's _current_ behavior (since both the
explicit
> > > shutting down of the container and the browser exiting code paths have the
> > same
> > > container-terminating code), but I'd rather be explicit if we can, in case
> we
> > > need to change its behavior. e.g. at some point there were plans of giving
> > > Android a bit more time to shutdown, so that code might diverge slightly
in
> > the
> > > future.
> > > 
> > > The only thing we cannot rely on is having the callback dispatched due to
> the
> > > lack of a message loop, but the outgoing call should be able to get
through
> > > without any issues.
> > IIUC unclean shutdown is also covered by SessionManagerService::HandleExit.
> >
>
(https://chromium.googlesource.com/chromiumos/platform2/+/master/login_manager...)
> > I think SessionManager must continue shutting down the container whenever
> Chrome
> > dies, because Chrome can die in ways other than clean shutdown (e.g. crash,
or
> > getting killed by SessionManager), but the container must be killed before
> > Chrome gets restarted.
> > 
> > Also, SessionManagerService::HandleExit() gives the container 3 seconds to
> > shutdown, while SessionManagerImpl::StopArcInstance() only gives it 1
second.
> > Although I don't think this difference was introduced intentionally, IIUC
> > calling StopArcInstance during Chrome shutdown results in unclean container
> > shutdown happening more frequently.
> 
> Oh that was unintentional :/ we should swap the timeouts in session_manager,
so
> that the clean exit is 3s and the non-clean shutdown is just 1.

Yes, then it makes sense to me. Added StopArcInstance(). Will land, when trybots
are finished.

Thank you for review.

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-21 08:50:43 UTC) #21

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-21 08:50:44 UTC) #22

hidehiko

The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, lhchavez@chromium.org Link to ...

4 years, 2 months ago (2016-10-21 09:55:20 UTC) #24

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2436763004/60001

4 years, 2 months ago (2016-10-21 09:55:31 UTC) #25

commit-bot: I haz the power

Description was changed from ========== More graceful shutdown for ArcSession. Currently, ArcSession uses the same ...

4 years, 2 months ago (2016-10-21 13:28:28 UTC) #27

commit-bot: I haz the power

4 years, 2 months ago (2016-10-21 13:28:29 UTC) #28

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/1362287708e9a559efbe66b4a137150c6aef7348
Cr-Commit-Position: refs/heads/master@{#426760}

Issue 2436763004: More graceful shutdown for ArcSession. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Address comments. #

Patch Set 3 : Rebase #

Patch Set 4 : Add StopArcInstance #

Messages