Deregister window classes

Deregister window classes

BUG=

Committed: https://crrev.com/445ee792be6efa64d9da2c67b05a7ec0c01c65dd
Cr-Commit-Position: refs/heads/master@{#294617}

[DaveMoore, 2014-09-08 22:29:40]

davemoore@chromium.org changed reviewers:
+ bauerb@chromium.org, ben@chromium.org

[DaveMoore, 2014-09-08 22:29:41]

On mojo it's possible to unload this code. That leaves windows with references
to invalid memory if the atoms are not deregistered.

[Ben Goodger (Google), 2014-09-08 23:03:59]

lgtm, curious to hear from Bernhard why this was removed.

[Bernhard Bauer, 2014-09-09 08:04:06]

Okay, IIRC it was something like this: Back when the ClassRegistrar was
non-leaky, it would get destroyed in unit tests with the ShadowingAtExitManager.
Deregistering the classes would fail though (see http://crbug.com/113008), so
the next time we would try again to register the classes, we would trip a CHECK.
The net effect was that adding a ShadowingAtExitManager to a unit test would
cause a different unit test later on to fail. In http://crbug.com/113008, Peter
suggested not unregistering the window classes at all, so that's what I ended up
doing.

See also http://crrev.com/11417003, where more of the discussion around this can
be found (that last patch set on http://crrev.com/11364202 shouldn't actually be
there; I think I uploaded it to that CL by accident).

If you look at the trybot output
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg/...),
you can see that there are FATAL errors in window_impl.cc, but they're masked by
the automatic retrying (because the problem only manifests if multiple tests
that register window classes are run in the same process).

[DaveMoore, 2014-09-10 14:11:37]

Add switch to control window unregistering

[DaveMoore, 2014-09-11 00:30:03]

I made unregistering the window classes something that could be set via a static
method on WindowImpl. We'll set this in the mojo view manager tests.

[Bernhard Bauer, 2014-09-11 08:45:07]

https://codereview.chromium.org/557513002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_unittest.cc (right):

https://codereview.chromium.org/557513002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_unittest.cc:441:
gfx::WindowImpl::SetUnregisterClassesAtExit(true);
This relies somewhat on the fact that there is an AtExitManager, and that it
will be destroyed at the right time. It would be good if we could more directly
control when the classes are unregistered, e.g. with a scoped object. Or at
least, we could schedule this via AtExitManager::RegisterTask(), which will
check that there is an AtExitManager.

https://codereview.chromium.org/557513002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_unittest.cc:545: TEST_F(ViewManagerTest,
DISABLED_ValidId) {
What's this about?

https://codereview.chromium.org/557513002/diff/20001/ui/gfx/win/window_impl.h
File ui/gfx/win/window_impl.h (right):

https://codereview.chromium.org/557513002/diff/20001/ui/gfx/win/window_impl.h...
ui/gfx/win/window_impl.h:46: // of windows, but is necessary if the tests uses a
ShadowingAtExitManager.
This comment doesn't really make sense. Calling this is not necessary just
because the test uses a ShadowingAtExitManager, only if it actually unloads the
window procedure.

[DaveMoore, 2014-09-12 03:57:22]

Address review concerns

[DaveMoore, 2014-09-12 03:58:54]

Address review concerns

[DaveMoore, 2014-09-12 04:04:20]

Address review concerns

[DaveMoore, 2014-09-12 04:24:44]

Use RegisterTask() instead of RegisterCallback()

[DaveMoore, 2014-09-12 04:26:49]

Minor indent fix

[DaveMoore, 2014-09-12 04:27:29]

https://codereview.chromium.org/557513002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_unittest.cc (right):

https://codereview.chromium.org/557513002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_unittest.cc:441:
gfx::WindowImpl::SetUnregisterClassesAtExit(true);
Changed it to use RegisterTask() instead.

On 2014/09/11 08:45:06, Bernhard Bauer wrote:
> This relies somewhat on the fact that there is an AtExitManager, and that it
> will be destroyed at the right time. It would be good if we could more
directly
> control when the classes are unregistered, e.g. with a scoped object. Or at
> least, we could schedule this via AtExitManager::RegisterTask(), which will
> check that there is an AtExitManager.

https://codereview.chromium.org/557513002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_unittest.cc:545: TEST_F(ViewManagerTest,
DISABLED_ValidId) {
Accident. I'll submit the view_manager_unittests changes separately.
On 2014/09/11 08:45:06, Bernhard Bauer wrote:
> What's this about?

https://codereview.chromium.org/557513002/diff/20001/ui/gfx/win/window_impl.h
File ui/gfx/win/window_impl.h (right):

https://codereview.chromium.org/557513002/diff/20001/ui/gfx/win/window_impl.h...
ui/gfx/win/window_impl.h:46: // of windows, but is necessary if the tests uses a
ShadowingAtExitManager.
On 2014/09/11 08:45:06, Bernhard Bauer wrote:
> This comment doesn't really make sense. Calling this is not necessary just
> because the test uses a ShadowingAtExitManager, only if it actually unloads
the
> window procedure.

Done.

[Bernhard Bauer, 2014-09-12 08:22:33]

LGTM w/ nits (seems like you have compile errors though)

https://codereview.chromium.org/557513002/diff/120001/ui/gfx/win/window_impl.cc
File ui/gfx/win/window_impl.cc (right):

https://codereview.chromium.org/557513002/diff/120001/ui/gfx/win/window_impl....
ui/gfx/win/window_impl.cc:63: base::string16 name,
Pass by const reference?

https://codereview.chromium.org/557513002/diff/120001/ui/gfx/win/window_impl....
ui/gfx/win/window_impl.cc:181: void WindowImpl::SetUnregisterClassesAtExit(bool
unregister_classes_at_exit) {
This parameter isn't used (and not declared in the header...?)

https://codereview.chromium.org/557513002/diff/120001/ui/gfx/win/window_impl....
ui/gfx/win/window_impl.cc:310: void WindowImpl::UnregisterClasses(void* dummy) {
AtExitManager::RegisterCallback also takes a base::Closure, so you don't need
the dummy parameter.

[DaveMoore, 2014-09-12 16:32:35]

Make name arg a const ref

[DaveMoore, 2014-09-12 16:32:57]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-09-12 16:34:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/557513002/160001

[commit-bot: I haz the power, 2014-09-12 18:18:54]

Committed patchset #9 (id:160001) as 0b4918ed75e3b68408126082b26e76449958ad7d

[commit-bot: I haz the power, 2014-09-12 18:30:11]

Patchset 9 (id:??) landed as
https://crrev.com/445ee792be6efa64d9da2c67b05a7ec0c01c65dd
Cr-Commit-Position: refs/heads/master@{#294617}