Have WindowTreeClient detect error state earlier

Have WindowTreeClient detect error state earlier

Currently WindowTreeClient cleans up all windows during destruction. However
this masks incorrect usage of the client library. It also makes it difficult to
find errors, as crashes occur later on in shutdown.

This change updates the destructor to explicitly fail if there are windows.

TEST=manual, mash_browser_tests
BUG=678687
Review-Url: https://codereview.chromium.org/2696573003
Cr-Commit-Position: refs/heads/master@{#450968}
Committed: https://chromium.googlesource.com/chromium/src/+/dcfe7f207b2b188b50b3e25eef5765b662c36455

[jonross, 2017-02-13 21:25:40]

jonross@chromium.org changed reviewers:
+ sky@chromium.org

[jonross, 2017-02-13 21:25:40]

Hey sky@

As discussed, a more aggressive way to point our incorrect usage of
WindowTreeClient, by DCHECKing the windows at destruction time.

PTAL

[sky, 2017-02-13 23:16:15]

https://codereview.chromium.org/2696573003/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2696573003/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:187: DCHECK(windows_.empty());
I'm ok with making this a CHECK as I suspect this is going to come up, and it
would be good to know the reason vs mysterious errors down the line.

https://codereview.chromium.org/2696573003/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:189: for (WindowTreeClientObserver& observer :
observers_)
At this point we don't need be OnWillDestroyClient and OnDidDestroyClient. Maybe
remove the OnDidDestroyClient? It looks like the current user of it can
trivially be converted.

[jonross, 2017-02-14 20:35:21]

https://codereview.chromium.org/2696573003/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2696573003/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:187: DCHECK(windows_.empty());
On 2017/02/13 23:16:15, sky wrote:
> I'm ok with making this a CHECK as I suspect this is going to come up, and it
> would be good to know the reason vs mysterious errors down the line.

Done.

https://codereview.chromium.org/2696573003/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:189: for (WindowTreeClientObserver& observer :
observers_)
On 2017/02/13 23:16:15, sky wrote:
> At this point we don't need be OnWillDestroyClient and OnDidDestroyClient.
Maybe
> remove the OnDidDestroyClient? It looks like the current user of it can
> trivially be converted.

Done.

[sky, 2017-02-14 22:52:06]

LGTM

[jonross, 2017-02-15 19:15:41]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-02-15 19:16:28]

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-02-15 21:05:00]

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

[commit-bot: I haz the power, 2017-02-15 21:05:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jonross, 2017-02-15 21:49:06]

The new CHECK found an error with a test. It wasn't deleting its widgets before
shutting down mus.

Mind taking a look before I resubmit?

[sky, 2017-02-15 23:29:36]

LGTM

[jonross, 2017-02-16 02:10:55]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-02-16 02:12:18]

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-02-16 04:09:26]

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

[commit-bot: I haz the power, 2017-02-16 04:09:26]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jonross, 2017-02-16 14:23:12]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-02-16 14:23:38]

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-02-16 15:16:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487254992513060,
"parent_rev": "180587c15a36861cdaaceec045619026e55fe8c5", "commit_rev":
"dcfe7f207b2b188b50b3e25eef5765b662c36455"}

[commit-bot: I haz the power, 2017-02-16 15:16:50]

Description was changed from

==========
Have WindowTreeClient detect error state earlier

Currently WindowTreeClient cleans up all windows during destruction. However
this masks incorrect usage of the client library. It also makes it difficult to
find errors, as crashes occur later on in shutdown.

This change updates the destructor to explicitly fail if there are windows.

TEST=manual, mash_browser_tests
BUG=678687
==========

to

==========
Have WindowTreeClient detect error state earlier

Currently WindowTreeClient cleans up all windows during destruction. However
this masks incorrect usage of the client library. It also makes it difficult to
find errors, as crashes occur later on in shutdown.

This change updates the destructor to explicitly fail if there are windows.

TEST=manual, mash_browser_tests
BUG=678687

Review-Url: https://codereview.chromium.org/2696573003
Cr-Commit-Position: refs/heads/master@{#450968}
Committed:
https://chromium.googlesource.com/chromium/src/+/dcfe7f207b2b188b50b3e25eef57...
==========

[commit-bot: I haz the power, 2017-02-16 15:16:54]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/dcfe7f207b2b188b50b3e25eef57...