arc: Fix crash on Arc window close.

arc: Fix crash on Arc window close.

BUG=638261
BUG=b/30817552
TEST=Manually on device. No more crashes are observed.

Committed: https://crrev.com/323b4a4792b0c6eb7e51d2aee66773378229e486
Cr-Commit-Position: refs/heads/master@{#412390}

[khmel, 2016-08-15 21:55:07]

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

[khmel, 2016-08-15 21:55:08]

Hi Stefan,

This fixes regression caused by https://codereview.chromium.org/2243573002.

Before we did not associate arc app window with a native window. CL above adds
this support. But for arc this is done deferred. So I added correct support.

PTAL

[Mr4D (OOO till 08-26), 2016-08-15 22:09:39]

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

https://codereview.chromium.org/2251493002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:364:
app_window->controller()->OnWindowAttached(app_window);
Hmm... I have the feeling that this gets probably caused by some kind of race
condition / call ordering? If so we might re-attach the window to the controller
and later on no one might ever remove it again?

[khmel, 2016-08-15 23:05:00]

PTAL

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

https://codereview.chromium.org/2251493002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:364:
app_window->controller()->OnWindowAttached(app_window);
On 2016/08/15 22:09:39, Mr4D wrote:
> Hmm... I have the feeling that this gets probably caused by some kind of race
> condition / call ordering? If so we might re-attach the window to the
controller
> and later on no one might ever remove it again?

Yes, we have race condition here because we use different protocols: mojom to
dispatch task info (onTaskCreated) and wayland to handle windows. Ideal solution
would be probably switch to one protocol (wayland) but this is quite big task. 
Alternative solution is to create controller when we have all required
information (arc window and app info). At this point we can associate native
window on controller during the creation and we don't need to modify base class.

WDYT?

[Mr4D (OOO till 08-26), 2016-08-15 23:33:56]

skuhne@chromium.org changed reviewers:
+ oshima@chromium.org

[Mr4D (OOO till 08-26), 2016-08-15 23:33:57]

I understand what you are trying to do and basically think it should do the
trick, but I would like to have oshima to have another look at it since this
appears to add more complexity then I expected.

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

https://codereview.chromium.org/2251493002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:353: //
because we want to update the orientation after exo send
Unrelated: Not sure I understand this. who is sending which layout / orientation
change and why?

[khmel, 2016-08-15 23:43:57]

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

[khmel, 2016-08-15 23:43:58]

Thanks Stefan,

Also adding David, he might want to take a look as well.

[reveman, 2016-08-15 23:47:04]

lgtm

[oshima, 2016-08-16 00:46:59]

can you update the bug description exactly how the crash was happening? (my
guess is that visibility change somehow happens after taskis destroyed?)

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

https://codereview.chromium.org/2251493002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:353: //
because we want to update the orientation after exo send
On 2016/08/15 23:33:56, Mr4D wrote:
> Unrelated: Not sure I understand this. who is sending which layout /
orientation
> change and why?

When the device enters into tablet mode, this class updates the device
orientation to use the requested orientation. However, it has to happen after
the android side enters tablet mode, which is handled in exo. This is to make
sure that EXO first receives it.

https://codereview.chromium.org/2251493002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h
(right):

https://codereview.chromium.org/2251493002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h:94: };
move this to .cc file.

[Mr4D (OOO till 08-26), 2016-08-16 14:15:27]

Sorry - I didn't mark it explicitly, but this lgtm for me, but please wait for
Oshima's comments.

[khmel, 2016-08-16 14:58:02]

Thank you for your reviews and comments.

I've updated bug with more detailed info and moved TaskInfo to .cc

PTAL

https://codereview.chromium.org/2251493002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h
(right):

https://codereview.chromium.org/2251493002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h:94: };
On 2016/08/16 00:46:59, oshima wrote:
> move this to .cc file.

Done.

[yoshiki, 2016-08-16 16:19:54]

Yury, did you forget to add task_info.cc/.h?

[khmel, 2016-08-16 16:21:24]

On 2016/08/16 16:19:54, yoshiki wrote:
> Yury, did you forget to add task_info.cc/.h?

Hi Yoshiki,

It is declared at arc_app_window_launcher_controller.cc:
ArcAppWindowLauncherController::TaskInfo

[yoshiki, 2016-08-16 16:21:35]

On 2016/08/16 16:19:54, yoshiki wrote:
> Yury, did you forget to add task_info.cc/.h?

Sorry, I was wrong. Please ignore my comment.

[khmel, 2016-08-16 16:24:43]

Description was changed from

==========
arc: Fix crash on Arc window close.

BUG=b/30817552
TEST=Manually on device. No more crashes are observed.
==========

to

==========
arc: Fix crash on Arc window close.

BUG=638261
BUG=b/30817552
TEST=Manually on device. No more crashes are observed.
==========

[oshima, 2016-08-16 18:03:11]

lgtm

https://codereview.chromium.org/2251493002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2251493002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:399:
task_id_to_task_info_.erase(task_id);
future improve suggestion: Consider using the state (like TASK_CREATED,
WINDOW_CREATED, RUNNING etc) rather than relying on the data availability on
multiple structure.

It's easier to read & understand, and it's easier to catch / avoid impossible
state. (and I think you can just have one map from task_id to all info).

[khmel, 2016-08-16 18:06:36]

https://codereview.chromium.org/2251493002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2251493002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:399:
task_id_to_task_info_.erase(task_id);
On 2016/08/16 18:03:11, oshima wrote:
> future improve suggestion: Consider using the state (like TASK_CREATED,
> WINDOW_CREATED, RUNNING etc) rather than relying on the data availability on
> multiple structure.
> 
> It's easier to read & understand, and it's easier to catch / avoid impossible
> state. (and I think you can just have one map from task_id to all info).

I agree with you, but from other side we might want to refactor this to use
wayland protocol only and this code will be deprecated. In any way I will keep
in mind you suggestion while refactoring this.

[khmel, 2016-08-16 18:06:40]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-08-16 18:06:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/2251493002/#ps60001
(title: "moved TaskInfo")

[commit-bot: I haz the power, 2016-08-16 18:07:12]

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-08-16 18:45:26]

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

[commit-bot: I haz the power, 2016-08-16 18:45:27]

Exceeded global retry quota

[khmel, 2016-08-16 23:00:10]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-08-16 23:00:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, oshima@chromium.org, skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/2251493002/#ps80001
(title: "unit_tests fixed")

[commit-bot: I haz the power, 2016-08-16 23:00:53]

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-08-17 00:08:14]

Description was changed from

==========
arc: Fix crash on Arc window close.

BUG=638261
BUG=b/30817552
TEST=Manually on device. No more crashes are observed.
==========

to

==========
arc: Fix crash on Arc window close.

BUG=638261
BUG=b/30817552
TEST=Manually on device. No more crashes are observed.
==========

[commit-bot: I haz the power, 2016-08-17 00:08:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-17 00:11:32]

Description was changed from

==========
arc: Fix crash on Arc window close.

BUG=638261
BUG=b/30817552
TEST=Manually on device. No more crashes are observed.
==========

to

==========
arc: Fix crash on Arc window close.

BUG=638261
BUG=b/30817552
TEST=Manually on device. No more crashes are observed.

Committed: https://crrev.com/323b4a4792b0c6eb7e51d2aee66773378229e486
Cr-Commit-Position: refs/heads/master@{#412390}
==========

[commit-bot: I haz the power, 2016-08-17 00:11:34]

Patchset 5 (id:??) landed as
https://crrev.com/323b4a4792b0c6eb7e51d2aee66773378229e486
Cr-Commit-Position: refs/heads/master@{#412390}