arc: Fix UpdatePinnedState crash

arc: Fix UpdatePinnedState crash

Transient notifications get into a situation that the item is destroyed
via mojo before its surface gets mapped via wayland. Check |item_|
before UpdatePinnedState() call in SetSurface to fix the crash.

BUG=647829
TBR=yoshiki@chromium.org

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

[xiyuan, 2016-09-16 22:27:36]

xiyuan@chromium.org changed reviewers:
+ yoshiki@chromium.org

[xiyuan, 2016-09-16 22:27:52]

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

[commit-bot: I haz the power, 2016-09-16 22:28:18]

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-16 23:23:16]

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

[commit-bot: I haz the power, 2016-09-16 23:23:17]

Dry run: This issue passed the CQ dry run.

[zel, 2016-09-19 16:36:55]

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

[zel, 2016-09-19 16:36:55]

lgtm

[xiyuan, 2016-09-19 17:31:39]

Description was changed from

==========
arc: Fix UpdatePinnedState crash

Transient notifications get into a situation that the item is destroyed
via mojo before its surface gets mapped via wayland. Check |item_|
before UpdatePinnedState() call in SetSurface to fix the crash.

BUG=647829
==========

to

==========
arc: Fix UpdatePinnedState crash

Transient notifications get into a situation that the item is destroyed
via mojo before its surface gets mapped via wayland. Check |item_|
before UpdatePinnedState() call in SetSurface to fix the crash.

BUG=647829
TBR=yoshiki@chromium.org
==========

[xiyuan, 2016-09-19 17:31:55]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2016-09-19 17:32:22]

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-19 18:18:22]

Description was changed from

==========
arc: Fix UpdatePinnedState crash

Transient notifications get into a situation that the item is destroyed
via mojo before its surface gets mapped via wayland. Check |item_|
before UpdatePinnedState() call in SetSurface to fix the crash.

BUG=647829
TBR=yoshiki@chromium.org
==========

to

==========
arc: Fix UpdatePinnedState crash

Transient notifications get into a situation that the item is destroyed
via mojo before its surface gets mapped via wayland. Check |item_|
before UpdatePinnedState() call in SetSurface to fix the crash.

BUG=647829
TBR=yoshiki@chromium.org
==========

[commit-bot: I haz the power, 2016-09-19 18:18:23]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-19 18:21:18]

Description was changed from

==========
arc: Fix UpdatePinnedState crash

Transient notifications get into a situation that the item is destroyed
via mojo before its surface gets mapped via wayland. Check |item_|
before UpdatePinnedState() call in SetSurface to fix the crash.

BUG=647829
TBR=yoshiki@chromium.org
==========

to

==========
arc: Fix UpdatePinnedState crash

Transient notifications get into a situation that the item is destroyed
via mojo before its surface gets mapped via wayland. Check |item_|
before UpdatePinnedState() call in SetSurface to fix the crash.

BUG=647829
TBR=yoshiki@chromium.org

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

[commit-bot: I haz the power, 2016-09-19 18:21:19]

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

[yoshiki, 2016-09-20 03:43:06]

https://codereview.chromium.org/2345373002/diff/1/ui/arc/notification/arc_cus...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2345373002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:233: if (item_)
The patch itself lgtm, but why item is null here?

It means surface is changed after OnItemDestroying, but isn't it weird?

[xiyuan, 2016-09-20 15:58:16]

https://codereview.chromium.org/2345373002/diff/1/ui/arc/notification/arc_cus...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2345373002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:233: if (item_)
On 2016/09/20 03:43:06, yoshiki wrote:
> The patch itself lgtm, but why item is null here?
> 
> It means surface is changed after OnItemDestroying, but isn't it weird?

There is a racing between ArcNotificationService and wayland on notification
close. |item_| is set to null when a notification is removed and mojo call is
made. However, the window closing is async.  Even though we close the window
before we do mojo call in ArcNotificationService. It is still possible that the
notification surface could exist and update via wayland for a short while. Hence
the crash.