arc: Fix crash activating Play Store in deferred mode.

arc: Fix crash activating Play Store in deferred mode.

There are set of refactorings that makes using Play Store
deferred launcher unsafe. Activating Play Store shortcut
may cause replacing controller inline and this makes context
of current object is invalid after the call. This CL fixes
this issue by skipping access to object if Play Store was
actually launched.

TEST=Extended unit_tests
BUG=707895
Review-Url: https://codereview.chromium.org/2793223002
Cr-Commit-Position: refs/heads/master@{#461840}
Committed: https://chromium.googlesource.com/chromium/src/+/a6834c45baabad9b9f99d2750db94880147ccf38

[khmel, 2017-04-04 00:22:05]

khmel@chromium.org changed reviewers:
+ msw@chromium.org

[khmel, 2017-04-04 00:22:05]

Hi Mike,

PTAL
Thanks!

[msw, 2017-04-04 01:42:06]

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:30:
// Note, ArcAppLauncher may invoke arc::LaunchApp that in some cases creates
I don't quite understand this comment or why |playstore_launcher_| needs a
conditional assignment below... Can you explain the defect and fix a bit more?

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:31:
// deferred launch item which substitutes current controller. It is not safe
what is the "current controller"?

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:32:
// to access this object if Play Store was actually launched.
by "this object", do you mean this ArcPlaystoreShortcutLauncherItemController
instance?

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:4182:
ChromeLauncherController::set_instance_for_test(launcher_controller_.get());
This is not necessary

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:4192:
// Simulate click. This should register deferred controller.
nit: explain what deferred controller you mean more clearly, use a class name?

[khmel, 2017-04-04 16:52:26]

Thanks for your comments.

PTAL updated version.

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:30:
// Note, ArcAppLauncher may invoke arc::LaunchApp that in some cases creates
On 2017/04/04 01:42:05, msw wrote:
> I don't quite understand this comment or why |playstore_launcher_| needs a
> conditional assignment below... Can you explain the defect and fix a bit more?

Problem following. This is shortcut controller. User activates it. As result we
start an app. In our case this is Play Store app. Usually application starts
asynchronously. Once app started we remove (it gets deleted) shortcut controller
and replace it with app controller.
In case of deferred launch we have different timing. ArcAppLauncher is a helper
that waits until ARC app appears and launch it.
In case leading to crash, ARC app (Play Store) exists but not ready yet. That
means we are using deferred launch (spinning icon in shelf). Check for app
exists is made in ArcAppLauncher controller. That means this instance of
ArcPlaystoreShortcutLauncherItemController is deleted on exiting from 
ArcAppLauncher CTOR. That means access to playstore_launcher_ and 
GetAppMenuItems is invalid once this is deleled.

rephrased comment

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:31:
// deferred launch item which substitutes current controller. It is not safe
On 2017/04/04 01:42:05, msw wrote:
> what is the "current controller"?

Explained above

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:32:
// to access this object if Play Store was actually launched.
On 2017/04/04 01:42:05, msw wrote:
> by "this object", do you mean this ArcPlaystoreShortcutLauncherItemController
> instance?

yes

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:4182:
ChromeLauncherController::set_instance_for_test(launcher_controller_.get());
On 2017/04/04 01:42:05, msw wrote:
> This is not necessary

Done.

https://codereview.chromium.org/2793223002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:4192:
// Simulate click. This should register deferred controller.
On 2017/04/04 01:42:05, msw wrote:
> nit: explain what deferred controller you mean more clearly, use a class name?

Done.

[msw, 2017-04-04 19:30:30]

lgtm

https://codereview.chromium.org/2793223002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2793223002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:30:
std::unique_ptr<ArcAppLauncher> playstore_launcher =
Should this be skipped if |playstore_launcher_| is already not null?

https://codereview.chromium.org/2793223002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:34:
// ArcAppLauncher may launch Play Store in case it exists already. In this
Weird, so constructing the ArcAppLauncher may delete this object? Very weird.

[khmel, 2017-04-04 20:13:49]

Thanks for review!

https://codereview.chromium.org/2793223002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2793223002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:30:
std::unique_ptr<ArcAppLauncher> playstore_launcher =
On 2017/04/04 19:30:29, msw wrote:
> Should this be skipped if |playstore_launcher_| is already not null?

Makes sense, thanks! Ut can be potentially activated twice. We can skip
recreating if it was already scheduled.

https://codereview.chromium.org/2793223002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:34:
// ArcAppLauncher may launch Play Store in case it exists already. In this
On 2017/04/04 19:30:29, msw wrote:
> Weird, so constructing the ArcAppLauncher may delete this object? Very weird.

Agree, but from point of view of ArcAppLauncher this is correct operation. Chain
of such assumptions leads to this problem. I tried different ways solving this
at most simple is this one :(

[khmel, 2017-04-04 20:13:55]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-04-04 20:13:55]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2793223002/#ps60001
(title: "check if request was already scheduled")

[commit-bot: I haz the power, 2017-04-04 20:14:55]

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, 2017-04-04 21:14:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491336835468970,
"parent_rev": "f6b258ce5b3d4224eafd5fb73ed1b38ebf20df3c", "commit_rev":
"a6834c45baabad9b9f99d2750db94880147ccf38"}

[commit-bot: I haz the power, 2017-04-04 21:14:52]

Description was changed from

==========
arc: Fix crash activating Play Store in deferred mode.

There are set of refactorings that makes using Play Store
deferred launcher unsafe. Activating Play Store shortcut
may cause replacing controller inline and this makes context
of current object is invalid after the call. This CL fixes
this issue by skipping access to object if Play Store was
actually launched.

TEST=Extended unit_tests
BUG=707895
==========

to

==========
arc: Fix crash activating Play Store in deferred mode.

There are set of refactorings that makes using Play Store
deferred launcher unsafe. Activating Play Store shortcut
may cause replacing controller inline and this makes context
of current object is invalid after the call. This CL fixes
this issue by skipping access to object if Play Store was
actually launched.

TEST=Extended unit_tests
BUG=707895

Review-Url: https://codereview.chromium.org/2793223002
Cr-Commit-Position: refs/heads/master@{#461840}
Committed:
https://chromium.googlesource.com/chromium/src/+/a6834c45baabad9b9f99d2750db9...
==========

[commit-bot: I haz the power, 2017-04-04 21:14:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a6834c45baabad9b9f99d2750db9...