Resolve initialize/destory order between ArcService and ArcSessionManager.

Resolve initialize/destory order between ArcService and ArcSessionManager.

This CL ensures that the ArcSessionManager is alive during the whole
each ArcService's life time, by merging two class initialization
list in ArcSessionManager and ArcServiceLauncher.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.

Committed: https://crrev.com/79a569780d9e7e234a0d6d8777b91db4f9b2b8d9
Cr-Commit-Position: refs/heads/master@{#434990}

[hidehiko, 2016-11-25 06:36:29]

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

[commit-bot: I haz the power, 2016-11-25 06:36:39]

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

[hidehiko, 2016-11-25 06:44:30]

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

[hidehiko, 2016-11-25 06:44:30]

PTAL.
lhchavez@: c/b/c/arc, components/arc
xiyuan@: c/b/c/login.

With this change, each ArcService implementation can assume that
ArcSessionManager is alive during its life-time, specifically on destruction for
graceful shutdown.

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1844: if
(arc::ArcBridgeService::GetEnabled(
Note: this is just redundant.
UserSessionManager::Shutdown() is called only from:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...

However, beforehand, ArcServiceLauncher::Shutdown() is invoked:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...

so ArcService instances are already destroyed here.
# Moreover, DCHECK should fail.

[commit-bot: I haz the power, 2016-11-25 07:29:15]

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

[commit-bot: I haz the power, 2016-11-25 07:29:16]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-11-28 22:17:41]

lgtm with nits.

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_service_launcher.cc (right):

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_service_launcher.cc:45: ArcServiceLauncher*
g_arc_service_launcher = nullptr;
Please mention who owns this weak pointer.

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_service_launcher.cc:55: if
(arc_service_manager_)
Shouldn't this be DCHECK(!arc_service_manager_); ? If we reach the destructor
without having already called Shutdown() it's too late.

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_service_launcher.cc:76: // TODO(hidehiko):
ArcSessionManager should be rather than an ArcService but
You can now remove this TODO :)

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_service_launcher.cc:145: // Destory in the
reverse order of the initialization.
nit: destroy.

https://codereview.chromium.org/2531543003/diff/1/components/arc/arc_service_...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2531543003/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.cc:70: icon_loader_ = nullptr;
now that I notice, can you move these two after L72? this is innocuous since
they're scoped_refptrs, but it's better to undo work in the reverse order in
which it is done.

[xiyuan, 2016-11-28 22:33:28]

lgtm

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1844: if
(arc::ArcBridgeService::GetEnabled(
On 2016/11/25 06:44:30, hidehiko wrote:
> Note: this is just redundant.
> UserSessionManager::Shutdown() is called only from:
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...
> 
> However, beforehand, ArcServiceLauncher::Shutdown() is invoked:
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...
> 
> so ArcService instances are already destroyed here.
> # Moreover, DCHECK should fail.

Acknowledged.

[hidehiko, 2016-11-29 14:14:12]

Thank you for review. Submitting.

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_service_launcher.cc (right):

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_service_launcher.cc:45: ArcServiceLauncher*
g_arc_service_launcher = nullptr;
On 2016/11/28 22:17:40, Luis Héctor Chávez wrote:
> Please mention who owns this weak pointer.

Done.

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_service_launcher.cc:55: if
(arc_service_manager_)
On 2016/11/28 22:17:40, Luis Héctor Chávez wrote:
> Shouldn't this be DCHECK(!arc_service_manager_); ? If we reach the destructor
> without having already called Shutdown() it's too late.

Oh, right. Done.

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_service_launcher.cc:76: // TODO(hidehiko):
ArcSessionManager should be rather than an ArcService but
On 2016/11/28 22:17:40, Luis Héctor Chávez wrote:
> You can now remove this TODO :)

Yes! Removed.

https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_service_launcher.cc:145: // Destory in the
reverse order of the initialization.
On 2016/11/28 22:17:40, Luis Héctor Chávez wrote:
> nit: destroy.

Done.

https://codereview.chromium.org/2531543003/diff/1/components/arc/arc_service_...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2531543003/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.cc:70: icon_loader_ = nullptr;
On 2016/11/28 22:17:40, Luis Héctor Chávez wrote:
> now that I notice, can you move these two after L72? this is innocuous since
> they're scoped_refptrs, but it's better to undo work in the reverse order in
> which it is done.

Hmm... To make this reverse order of the initialization, moving this into L72
looks not enough.

To focus on the ArcServiceLauncher work, let me work on this in a separate CL.

[hidehiko, 2016-11-29 14:14:20]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-11-29 14:14:21]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2531543003/#ps40001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-11-29 14:14:32]

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-11-29 15:15:08]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480428860978490,
"parent_rev": "43e55d5f4578531cc125f9319fa65e8b18866e44", "commit_rev":
"53af4499a966e235f6f724ac6f88a7f3288eaa4e"}

[commit-bot: I haz the power, 2016-11-29 15:15:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-29 15:18:16]

Description was changed from

==========
Resolve initialize/destory order between ArcService and ArcSessionManager.

This CL ensures that the ArcSessionManager is alive during the whole
each ArcService's life time, by merging two class initialization
list in ArcSessionManager and ArcServiceLauncher.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

to

==========
Resolve initialize/destory order between ArcService and ArcSessionManager.

This CL ensures that the ArcSessionManager is alive during the whole
each ArcService's life time, by merging two class initialization
list in ArcSessionManager and ArcServiceLauncher.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.

Committed: https://crrev.com/79a569780d9e7e234a0d6d8777b91db4f9b2b8d9
Cr-Commit-Position: refs/heads/master@{#434990}
==========

[commit-bot: I haz the power, 2016-11-29 15:18:17]

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