arc: Fix race condition in applying modal state.

arc: Fix race condition in applying modal state.

There is race when request to set system modal received before
surface window is actually created. This CL keeps system model
property and apply it once window is created.

TEST=Manually, simulate situation for early system modal switch.
     Before fix it crashed with the same stack trace as crash
     reports. After this I observed warning message in log and
     system modal state is working as expected.
BUG=639511
Review-Url: https://codereview.chromium.org/2743053002
Cr-Commit-Position: refs/heads/master@{#456256}
Committed: https://chromium.googlesource.com/chromium/src/+/1111902f947ac92cb21d6ed443e2c607337308c1

[khmel, 2017-03-10 17:11:21]

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

[khmel, 2017-03-10 17:11:22]

Hi David,

PTAL

Thanks!

[reveman, 2017-03-10 21:38:22]

Can you add (or adjust existing) ShellSurface unit test for this?

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:500: system_modal_ = system_modal;
should we return early if it didn't change?

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:502: LOG(WARNING) << "Skip setting system modal
state to " << system_modal_
We shouldn't LOG things that can be triggered by a client as that allows a
malicious client to fill our log files. Maybe DLOG if important but in this case
it seems like it's just something we should handle silently.

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:510: }
Please add a private UpdateSystemModal() function that contains l.502-l.510 and
call that here instead.

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:777: if (system_modal_)
Please replace these two lines with a call to UpdateSystemModal().

[khmel, 2017-03-10 22:11:32]

Thank you for comments!

PTAL updated version

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:500: system_modal_ = system_modal;
On 2017/03/10 21:38:22, reveman wrote:
> should we return early if it didn't change?

Done.

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:502: LOG(WARNING) << "Skip setting system modal
state to " << system_modal_
On 2017/03/10 21:38:22, reveman wrote:
> We shouldn't LOG things that can be triggered by a client as that allows a
> malicious client to fill our log files. Maybe DLOG if important but in this
case
> it seems like it's just something we should handle silently.

Done.

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:510: }
On 2017/03/10 21:38:22, reveman wrote:
> Please add a private UpdateSystemModal() function that contains l.502-l.510
and
> call that here instead.

Done.

https://codereview.chromium.org/2743053002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:777: if (system_modal_)
On 2017/03/10 21:38:22, reveman wrote:
> Please replace these two lines with a call to UpdateSystemModal().

Done.

[khmel, 2017-03-10 22:11:53]

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

[commit-bot: I haz the power, 2017-03-10 22:12:10]

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, 2017-03-10 22:42:40]

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

[commit-bot: I haz the power, 2017-03-10 22:42:43]

Dry run: This issue passed the CQ dry run.

[reveman, 2017-03-11 00:45:09]

lgtm  % nits

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
components/exo/shell_surface.cc:505: if (!widget_) {
nit: I don't think we should have this warning. Seems like perfectly valid
behavior by a client to call this before the widget exists now. Please change it
to "if (widget_) UpdateSystemModal();".

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
components/exo/shell_surface.cc:517: container_ ==
ash::kShellWindowId_SystemModalContainer);
nit: just DCHECK_EQ(container_, ash::kShellWindowId_SystemModalContainer) here
and add a if "container_ == ash::kShellWindowId_SystemModalContainer" check to
OnSurfaceCommit

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
File components/exo/shell_surface_unittest.cc (right):

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
components/exo/shell_surface_unittest.cc:473: TEST_F(ShellSurfaceTest,
ModalWindowInitializedBeforeCommit) {
nit: s/Initialized/SetSystemModal/

[khmel, 2017-03-11 01:33:32]

Thank you for prompt review!

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
components/exo/shell_surface.cc:505: if (!widget_) {
On 2017/03/11 00:45:08, reveman wrote:
> nit: I don't think we should have this warning. Seems like perfectly valid
> behavior by a client to call this before the widget exists now. Please change
it
> to "if (widget_) UpdateSystemModal();".

Nice, done

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
components/exo/shell_surface.cc:517: container_ ==
ash::kShellWindowId_SystemModalContainer);
On 2017/03/11 00:45:09, reveman wrote:
> nit: just DCHECK_EQ(container_, ash::kShellWindowId_SystemModalContainer) here
> and add a if "container_ == ash::kShellWindowId_SystemModalContainer" check to
> OnSurfaceCommit

Done.

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
File components/exo/shell_surface_unittest.cc (right):

https://codereview.chromium.org/2743053002/diff/20001/components/exo/shell_su...
components/exo/shell_surface_unittest.cc:473: TEST_F(ShellSurfaceTest,
ModalWindowInitializedBeforeCommit) {
On 2017/03/11 00:45:09, reveman wrote:
> nit: s/Initialized/SetSystemModal/

Done.

[khmel, 2017-03-11 01:33:36]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-03-11 01:33:36]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2743053002/#ps60001
(title: "forgotten nit")

[commit-bot: I haz the power, 2017-03-11 01:33:57]

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-03-11 02:05:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489196016018910,
"parent_rev": "31e7b9090b8a1301e5c51a47f5bbd5bbe37e8914", "commit_rev":
"1111902f947ac92cb21d6ed443e2c607337308c1"}

[commit-bot: I haz the power, 2017-03-11 02:05:56]

Description was changed from

==========
arc: Fix race condition in applying modal state.

There is race when request to set system modal received before
surface window is actually created. This CL keeps system model
property and apply it once window is created.

TEST=Manually, simulate situation for early system modal switch.
     Before fix it crashed with the same stack trace as crash
     reports. After this I observed warning message in log and
     system modal state is working as expected.
BUG=639511
==========

to

==========
arc: Fix race condition in applying modal state.

There is race when request to set system modal received before
surface window is actually created. This CL keeps system model
property and apply it once window is created.

TEST=Manually, simulate situation for early system modal switch.
     Before fix it crashed with the same stack trace as crash
     reports. After this I observed warning message in log and
     system modal state is working as expected.
BUG=639511

Review-Url: https://codereview.chromium.org/2743053002
Cr-Commit-Position: refs/heads/master@{#456256}
Committed:
https://chromium.googlesource.com/chromium/src/+/1111902f947ac92cb21d6ed443e2...
==========

[commit-bot: I haz the power, 2017-03-11 02:05:57]

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