arc: Fix race condition in applying modal state.

components/exo/shell_surface.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/exo/shell_surface.h"
 
 #include <algorithm>
 
 #include "ash/common/frame/custom_frame_view_ash.h"
 #include "ash/common/shelf/wm_shelf.h"
@@ skipping 478 matching lines @@
   // managed system modal dialogs using a single ShellSurface instance.
   // Hit-test region will be non-empty when at least one dialog exists on
   // the client side. Here we detect the transition between no client side
   // dialog and at least one dialog so activatable state is properly
   // updated.
   if (container_ != ash::kShellWindowId_SystemModalContainer) {
     LOG(ERROR)
         << "Only a window in SystemModalContainer can change the modality";
     return;
   }
+
+  system_modal_ = system_modal;

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

should we return early if it didn't change?

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

Done.

+  if (!widget_) {
+    LOG(WARNING) << "Skip setting system modal state to " << system_modal_

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

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.

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

Done.

+                 << " because window is not ready yet.";
+    return;
+  }
+
   widget_->GetNativeWindow()->SetProperty(
       aura::client::kModalKey,
-      system_modal ? ui::MODAL_TYPE_SYSTEM : ui::MODAL_TYPE_NONE);
+      system_modal_ ? ui::MODAL_TYPE_SYSTEM : ui::MODAL_TYPE_NONE);
 }

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

Please add a private UpdateSystemModal() function that contains l.502-l.510 and
call that here instead.

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

Done.

 
 // static
 void ShellSurface::SetApplicationId(aura::Window* window,
                                     const std::string& id) {
   TRACE_EVENT1("exo", "ShellSurface::SetApplicationId", "application_id", id);
   window->SetProperty(aura::client::kAppIdKey, new std::string(id));
 }
 
 // static
 const std::string ShellSurface::GetApplicationId(aura::Window* window) {
@@ skipping 246 matching lines @@
       surface_->window()->SetTransform(transform);
       scale_ = pending_scale_;
     }
 
     // Show widget if needed.
     if (pending_show_widget_) {
       DCHECK(!widget_->IsClosed());
       DCHECK(!widget_->IsVisible());
       pending_show_widget_ = false;
       widget_->Show();
+      if (system_modal_)

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

Please replace these two lines with a call to UpdateSystemModal().

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

Done.

+        SetSystemModal(true);
     }
   }
 }
 
 bool ShellSurface::IsSurfaceSynchronized() const {
   // A shell surface is always desynchronized.
   return false;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ skipping 791 matching lines @@
     // small style shadow for them.
     if (!activatable_)
       shadow->SetElevation(wm::ShadowElevation::SMALL);
     // We don't have rounded corners unless frame is enabled.
     if (!frame_enabled_)
       shadow->SetRoundedCornerRadius(0);
   }
 }
 
 }  // namespace exo