Make sure WindowObservers are removed from window before destruction

Make sure WindowObservers are removed from window before destruction

* Check if the window observer is empty upon destruction.
* remove the all observers in ~WindowObserver()
* Remove RemoveObserver that are no longer necessary
* Fix shell shutdown order so that observers are removed correctly before deleting all windows.

BUG=324018
R=skuhne@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239232

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239246

[sky, 2013-12-04 04:27:32]

https://codereview.chromium.org/101013002/diff/120001/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

https://codereview.chromium.org/101013002/diff/120001/ui/aura/window_observer...
ui/aura/window_observer.h:92: // The default implementation simply remove itself
from |window|'s observer
If we're going to do this, then we should follow the pattern of
WebContentsObserver and have Window invoke a private function that removes this
as an observer and then invokes OnWindowDestroyed.

[oshima, 2013-12-04 19:21:07]

Uploaded new patch. PTAL.

[oshima, 2013-12-04 19:21:07]

Uploaded new patch. PTAL.

[oshima, 2013-12-04 19:21:45]

On 2013/12/04 19:21:07, oshima wrote:
> Uploaded new patch. PTAL.

skuhne@ -> chrome/browser/ui/ash/multi_user/

[Mr4D (OOO till 08-26), 2013-12-04 19:45:46]

Only a few comments.

Thanks for this - I like simplification!

That said, coming to think about it, it is a bit strange that in a few cases you
should not remove the observer whereas in others you have to. Even more so since
we have so many different observers. Maybe we should have a more general
function in the observer pattern like "OnObservedObjectDestroyed" which behaves
for all observers the same?

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:47:
// Delete the window at the given index.
What about
"Removes the window at the given index, shifting all following indices up."

[Because you don't do this you would need to also handle the NULL case in other
testing framework functions]

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:48:
void delete_at(size_t index) {
Can you rename this to delete_window_at?

(this is a test and its not clear what gets deleted otherwise)

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:50:
window_[index] = NULL;
Looking into line 114 I would think that you should do an window_.erase(..)
here?

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:218:
SetUpForThisManyWindows(2);
Since you only care about the one window you can change this now to (1) and
remove the ' S[]' in the two EXPECT_EQ's below.

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:300:
SetUpForThisManyWindows(2);
You might also change this to a (1).

[oshima, 2013-12-04 20:19:09]

On 2013/12/04 19:45:46, Mr4D wrote:
> Only a few comments.
> 
> Thanks for this - I like simplification!
> 
> That said, coming to think about it, it is a bit strange that in a few cases
you
> should not remove the observer whereas in others you have to.

There should not be such case. I tried to remove all RemoveObserver in
OnWindowDestroyed,
but I might have missed somewhere. Can you tell me where?


> Even more so since
> we have so many different observers. Maybe we should have a more general
> function in the observer pattern like "OnObservedObjectDestroyed" which
behaves
> for all observers the same?
> 
>
https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
> File
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:47:
> // Delete the window at the given index.
> What about
> "Removes the window at the given index, shifting all following indices up."
> 
> [Because you don't do this you would need to also handle the NULL case in
other
> testing framework functions]
> 
>
https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:48:
> void delete_at(size_t index) {
> Can you rename this to delete_window_at?
> 
> (this is a test and its not clear what gets deleted otherwise)
> 
>
https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:50:
> window_[index] = NULL;
> Looking into line 114 I would think that you should do an window_.erase(..)
> here?
> 
>
https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:218:
> SetUpForThisManyWindows(2);
> Since you only care about the one window you can change this now to (1) and
> remove the ' S[]' in the two EXPECT_EQ's below.
> 
>
https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:300:
> SetUpForThisManyWindows(2);
> You might also change this to a (1).

[sky, 2013-12-04 22:52:58]

Also add a comment to WindowObserver that Window automatically removes itself as
an observer before invoking OnWindowDestroyed.

https://codereview.chromium.org/101013002/diff/280001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/101013002/diff/280001/ash/shell.cc#newcode634
ash/shell.cc:634: // before |display_controller_| being deleted.
being -> is

https://codereview.chromium.org/101013002/diff/280001/ash/shell.cc#newcode636
ash/shell.cc:636: // Destroy VideoActivityNotifier before destroying
VideoDetector.
This comment documents the code, and therefor isn't helpful. If you're going to
add a comment, document why the order is important here.

https://codereview.chromium.org/101013002/diff/280001/ash/shell.cc#newcode651
ash/shell.cc:651: // Launcher delete must be deleted before |focus_client_|.
Same thing here.

[oshima, 2013-12-05 01:26:44]

https://codereview.chromium.org/101013002/diff/280001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/101013002/diff/280001/ash/shell.cc#newcode634
ash/shell.cc:634: // before |display_controller_| being deleted.
On 2013/12/04 22:52:59, sky wrote:
> being -> is

Done.

https://codereview.chromium.org/101013002/diff/280001/ash/shell.cc#newcode636
ash/shell.cc:636: // Destroy VideoActivityNotifier before destroying
VideoDetector.
On 2013/12/04 22:52:59, sky wrote:
> This comment documents the code, and therefor isn't helpful. If you're going
to
> add a comment, document why the order is important here.

I need these comments not to break the order when I refactor (like this)
because such dependency isn't clear from the code here. I updated the
comment to be more clear.

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:47:
// Delete the window at the given index.
On 2013/12/04 19:45:46, Mr4D wrote:
> What about
> "Removes the window at the given index, shifting all following indices up."
> 
> [Because you don't do this you would need to also handle the NULL case in
other
> testing framework functions]

IMHO it's better this way than shifting index. At least you get segfault with
0x0 (meaning you're trying to access deleted window) while if you use wrong
index, you get segfault with random address. see my comment below for deletion.

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:48:
void delete_at(size_t index) {
On 2013/12/04 19:45:46, Mr4D wrote:
> Can you rename this to delete_window_at?
> 
> (this is a test and its not clear what gets deleted otherwise)

Done.

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:50:
window_[index] = NULL;
On 2013/12/04 19:45:46, Mr4D wrote:
> Looking into line 114 I would think that you should do an window_.erase(..)
> here?

delete NULL is valid (and no op) in C++.

https://codereview.chromium.org/101013002/diff/280001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:300:
SetUpForThisManyWindows(2);
On 2013/12/04 19:45:46, Mr4D wrote:
> You might also change this to a (1).

Done.

[sky, 2013-12-05 01:36:35]

LGTM

https://codereview.chromium.org/101013002/diff/280001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/101013002/diff/280001/ash/shell.cc#newcode636
ash/shell.cc:636: // Destroy VideoActivityNotifier before destroying
VideoDetector.
On 2013/12/05 01:26:44, oshima wrote:
> On 2013/12/04 22:52:59, sky wrote:
> > This comment documents the code, and therefor isn't helpful. If you're going
> to
> > add a comment, document why the order is important here.
> 
> I need these comments not to break the order when I refactor (like this)
> because such dependency isn't clear from the code here. I updated the
> comment to be more clear.

I'm fine with comments, but they should describe why the code is the way it is,
not what the code does. For example:
// Destroy VideoActivityNotifier before VideoDetector as VideoActivityNotifier
references VideoDetector.

https://codereview.chromium.org/101013002/diff/300001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/101013002/diff/300001/ash/shell.cc#newcode633
ash/shell.cc:633: // Controllers who has WindowObserver added must be deleted
has->have

https://codereview.chromium.org/101013002/diff/300001/ash/shell.cc#newcode636
ash/shell.cc:636: // VideoActivityNotifier must be deleted before
|video_detector_| is deleted.
Again, this doesn't doesn't document why. That's the important part of the
comment. Even better than a comment is a test and nuke the comment. Don't feel
you need to do that though.

https://codereview.chromium.org/101013002/diff/300001/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

https://codereview.chromium.org/101013002/diff/300001/ui/aura/window_observer...
ui/aura/window_observer.h:92: // its parent.  Window class automatically removes
its
remove 'class'

[Mr4D (OOO till 08-26), 2013-12-05 01:54:28]

See my comment. Thanks!

https://codereview.chromium.org/101013002/diff/300001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/101013002/diff/300001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:47:
// Delete the window at the given index, and set the referefence to NULL.
As stated before ("other testing framework functions"): Calling e.g. GetStatus
after this call will cause a crash.

So either fix that function - or add a comment here.

[oshima, 2013-12-05 18:04:01]

https://codereview.chromium.org/101013002/diff/300001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/101013002/diff/300001/ash/shell.cc#newcode633
ash/shell.cc:633: // Controllers who has WindowObserver added must be deleted
On 2013/12/05 01:36:35, sky wrote:
> has->have

Done.

https://codereview.chromium.org/101013002/diff/300001/ash/shell.cc#newcode636
ash/shell.cc:636: // VideoActivityNotifier must be deleted before
|video_detector_| is deleted.
On 2013/12/05 01:36:35, sky wrote:
> Again, this doesn't doesn't document why. That's the important part of the
> comment. Even better than a comment is a test and nuke the comment. Don't feel
> you need to do that though.

Updated the comment.

https://codereview.chromium.org/101013002/diff/300001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/101013002/diff/300001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:47:
// Delete the window at the given index, and set the referefence to NULL.
On 2013/12/05 01:54:29, Mr4D wrote:
> As stated before ("other testing framework functions"): Calling e.g. GetStatus
> after this call will cause a crash.
> 
> So either fix that function - or add a comment here.

Done. GetStatus now returns "D" for deleted window.

https://codereview.chromium.org/101013002/diff/300001/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

https://codereview.chromium.org/101013002/diff/300001/ui/aura/window_observer...
ui/aura/window_observer.h:92: // its parent.  Window class automatically removes
its
On 2013/12/05 01:36:35, sky wrote:
> remove 'class'

Done.

[Mr4D (OOO till 08-26), 2013-12-05 18:39:22]

lgtm

[commit-bot: I haz the power, 2013-12-05 18:52:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/101013002/320001

[commit-bot: I haz the power, 2013-12-05 18:53:02]

Failed to apply patch for ash/shell.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/shell.cc
  Hunk #1 FAILED at 630.
  1 out of 1 hunk FAILED -- saving rejects to file ash/shell.cc.rej

Patch:       ash/shell.cc
Index: ash/shell.cc
diff --git a/ash/shell.cc b/ash/shell.cc
index
6fb3df982a13fc09b5c9993ca90e1e4d711d87cc..31fc382a1dd19b47ef1634eef737411e2b6e0aef
100644
--- a/ash/shell.cc
+++ b/ash/shell.cc
@@ -630,42 +630,53 @@ Shell::~Shell() {
   // Drag-and-drop must be canceled prior to close all windows.
   drag_drop_controller_.reset();
 
+  // Controllers who have WindowObserver added must be deleted
+  // before |display_controller_| is deleted.
+
+#if defined(OS_CHROMEOS)
+  // VideoActivityNotifier must be deleted before |video_detector_| is
+  // deleted because it's observing video activity through
+  // VideoDetectorObserver interface.
+  video_activity_notifier_.reset();
+#endif  // defined(OS_CHROMEOS)
+  video_detector_.reset();
+
+  shadow_controller_.reset();
+  resize_shadow_controller_.reset();
+
+  window_cycle_controller_.reset();
+  mru_window_tracker_.reset();
+
   // Destroy all child windows including widgets.
   display_controller_->CloseChildWindows();
   display_controller_->CloseNonDesktopDisplay();
 
+  // Chrome implementation of launcher delegate depends on FocusClient,
+  // so must be deleted before |focus_client_|.
+  launcher_delegate_.reset();
+  focus_client_.reset();
+
   // Destroy SystemTrayNotifier after destroying SystemTray as TrayItems
   // needs to remove observers from it.
   system_tray_notifier_.reset();
 
-#if defined(OS_CHROMEOS)
-  // Destroy VideoActivityNotifier before destroying VideoDetector.
-  video_activity_notifier_.reset();
-#endif  // defined(OS_CHROMEOS)
-
   // These need a valid Shell instance to clean up properly, so explicitly
   // delete them before invalidating the instance.
   // Alphabetical. TODO(oshima): sort.
   magnification_controller_.reset();
   partial_magnification_controller_.reset();
-  resize_shadow_controller_.reset();
-  shadow_controller_.reset();
   tooltip_controller_.reset();
   event_client_.reset();
-  window_cycle_controller_.reset();
   nested_dispatcher_controller_.reset();
   user_action_client_.reset();
   visibility_controller_.reset();
-  launcher_delegate_.reset();
   // |launcher_item_delegate_manager_| observes |shelf_model_|. It must be
   // destroyed before |shelf_model_| is destroyed.
   launcher_item_delegate_manager_.reset();
   shelf_model_.reset();
-  video_detector_.reset();
 
   power_button_controller_.reset();
   lock_state_controller_.reset();
-  mru_window_tracker_.reset();
 
   resolution_notification_controller_.reset();
   desktop_background_controller_.reset();

[commit-bot: I haz the power, 2013-12-05 19:43:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/101013002/340001

[commit-bot: I haz the power, 2013-12-05 22:22:59]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-05 23:22:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/101013002/340001

[commit-bot: I haz the power, 2013-12-06 05:05:54]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-06 06:48:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/101013002/340001

[commit-bot: I haz the power, 2013-12-06 07:39:06]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-06 07:44:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/101013002/340001

[commit-bot: I haz the power, 2013-12-06 08:33:54]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[oshima, 2013-12-06 17:10:01]

Committed patchset #6 manually as r239232 (presubmit successful).

[oshima, 2013-12-06 18:39:08]

Committed patchset #7 manually as r239246 (presubmit successful).