Delete delegates in WindowState when window is being destroyed

Pinned app can request restore during destruction, (which itself
shouldn't happen), but the windowstate should also have a safe guard
by deleting delegate when being deleted.

BUG=811398
TEST=covered by unit tests

Change-Id: I4de6608e97dcf9e48531d31b07a530c10ef68978
Reviewed-on: https://chromium-review.googlesource.com/929315
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#538262}(cherry picked from commit 64bf482feb86ae02503c588cad1cb56e46c31b1d)
Reviewed-on: https://chromium-review.googlesource.com/939921
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#609}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
diff --git a/ash/wm/client_controlled_state.cc b/ash/wm/client_controlled_state.cc
index 664d290..439bdb6 100644
--- a/ash/wm/client_controlled_state.cc
+++ b/ash/wm/client_controlled_state.cc
@@ -35,6 +35,8 @@
 
 void ClientControlledState::HandleTransitionEvents(WindowState* window_state,
                                                    const WMEvent* event) {
+  if (!delegate_)
+    return;
   bool pin_transition = window_state->IsTrustedPinned() ||
                         window_state->IsPinned() || event->IsPinEvent();
   // Pinned State transition is handled on server side.
@@ -98,6 +100,8 @@
 
 void ClientControlledState::HandleCompoundEvents(WindowState* window_state,
                                                  const WMEvent* event) {
+  if (!delegate_)
+    return;
   switch (event->type()) {
     case WM_EVENT_TOGGLE_MAXIMIZE_CAPTION:
       if (window_state->IsFullscreen()) {
@@ -141,6 +145,8 @@
 
 void ClientControlledState::HandleBoundsEvents(WindowState* window_state,
                                                const WMEvent* event) {
+  if (!delegate_)
+    return;
   switch (event->type()) {
     case WM_EVENT_SET_BOUNDS: {
       const gfx::Rect& bounds =
@@ -171,12 +177,17 @@
   }
 }
 
+void ClientControlledState::OnWindowDestroying(WindowState* window_state) {
+  delegate_.reset();
+}
+
 bool ClientControlledState::EnterNextState(
     WindowState* window_state,
     mojom::WindowStateType next_state_type,
     BoundsChangeAnimationType animation_type) {
-  // Do nothing if  we're already in the same state.
-  if (state_type_ == next_state_type)
+  // Do nothing if  we're already in the same state, or delegate has already
+  // been deleted.
+  if (state_type_ == next_state_type || !delegate_)
     return false;
   bounds_change_animation_type_ = animation_type;
   mojom::WindowStateType previous_state_type = state_type_;
diff --git a/ash/wm/client_controlled_state.h b/ash/wm/client_controlled_state.h
index 9f57b14d..9f04763 100644
--- a/ash/wm/client_controlled_state.h
+++ b/ash/wm/client_controlled_state.h
@@ -72,6 +72,7 @@
                           const WMEvent* event) override;
   void HandleTransitionEvents(WindowState* window_state,
                               const WMEvent* event) override;
+  void OnWindowDestroying(WindowState* window_state) override;
 
   // Enters next state. This is used when the state moves from one to another
   // within the same desktop mode. Returns true if the state has changed, or
diff --git a/ash/wm/client_controlled_state_unittest.cc b/ash/wm/client_controlled_state_unittest.cc
index c6c93bb..daac8f6 100644
--- a/ash/wm/client_controlled_state_unittest.cc
+++ b/ash/wm/client_controlled_state_unittest.cc
@@ -30,6 +30,7 @@
 
   void HandleWindowStateRequest(WindowState* window_state,
                                 mojom::WindowStateType next_state) override {
+    EXPECT_FALSE(deleted_);
     old_state_ = window_state->GetStateType();
     new_state_ = next_state;
   }
@@ -51,10 +52,13 @@
     requested_bounds_.SetRect(0, 0, 0, 0);
   }
 
+  void mark_as_deleted() { deleted_ = true; }
+
  private:
   mojom::WindowStateType old_state_ = mojom::WindowStateType::DEFAULT;
   mojom::WindowStateType new_state_ = mojom::WindowStateType::DEFAULT;
   gfx::Rect requested_bounds_;
+  bool deleted_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestClientControlledStateDelegate);
 };
@@ -390,6 +394,18 @@
   EXPECT_TRUE(window_state_2->IsTrustedPinned());
 }
 
+TEST_F(ClientControlledStateTest, ClosePinned) {
+  EXPECT_FALSE(window_state()->IsPinned());
+  EXPECT_FALSE(GetScreenPinningController()->IsPinned());
+
+  const WMEvent trusted_pin_event(WM_EVENT_TRUSTED_PIN);
+  window_state()->OnWMEvent(&trusted_pin_event);
+  EXPECT_TRUE(window_state()->IsPinned());
+  EXPECT_TRUE(GetScreenPinningController()->IsPinned());
+  delegate()->mark_as_deleted();
+  widget()->CloseNow();
+}
+
 TEST_F(ClientControlledStateTest, MoveWindowToDisplay) {
   UpdateDisplay("500x500, 500x500");
 
diff --git a/ash/wm/screen_pinning_controller.cc b/ash/wm/screen_pinning_controller.cc
index cb00882..65d6819 100644
--- a/ash/wm/screen_pinning_controller.cc
+++ b/ash/wm/screen_pinning_controller.cc
@@ -232,10 +232,8 @@
 void ScreenPinningController::OnWillRemoveWindowFromPinnedContainer(
     aura::Window* window) {
   window->RemoveObserver(pinned_container_child_window_observer_.get());
-  if (window == pinned_window_) {
+  if (window == pinned_window_)
     wm::GetWindowState(pinned_window_)->Restore();
-    return;
-  }
 }
 
 void ScreenPinningController::OnPinnedContainerWindowStackingChanged(
diff --git a/ash/wm/window_state.cc b/ash/wm/window_state.cc
index 7712c98..7806def 100644
--- a/ash/wm/window_state.cc
+++ b/ash/wm/window_state.cc
@@ -686,5 +686,11 @@
   MoveAllTransientChildrenToNewRoot(window);
 }
 
+void WindowState::OnWindowDestroying(aura::Window* window) {
+  DCHECK_EQ(window_, window);
+  current_state_->OnWindowDestroying(this);
+  delegate_.reset();
+}
+
 }  // namespace wm
 }  // namespace ash
diff --git a/ash/wm/window_state.h b/ash/wm/window_state.h
index b745ac3..1adcf703 100644
--- a/ash/wm/window_state.h
+++ b/ash/wm/window_state.h
@@ -85,6 +85,9 @@
     // Note: This only gets called when the state object gets changed.
     virtual void DetachState(WindowState* window_state) = 0;
 
+    // Called when the window is being destroyed.
+    virtual void OnWindowDestroying(WindowState* window_state) {}
+
    private:
     DISALLOW_COPY_AND_ASSIGN(State);
   };
@@ -400,6 +403,7 @@
                                const void* key,
                                intptr_t old) override;
   void OnWindowAddedToRootWindow(aura::Window* window) override;
+  void OnWindowDestroying(aura::Window* window) override;
 
   // The owner of this window settings.
   aura::Window* window_;