Allow changing AppWindowClient for tests

AppWindowClient::Set sets the global AppWindowClient. Unit tests
sometimes call it more than once, e.g. when multiple BrowserProcesses
are created. Currently, each additional call silently returns instead of
changing g_client. Even Set(nullptr) does nothing.

Normally, this is fine: only tests should call it more than once, and
usually it's called with the same value anyway (because BrowserProcess
and TestingBrowserProcess both use the global ChromeAppWindowClient
singleton).

Some tests do create and destroy unique AppWindowClients, like ones in
app_shell_unittest. This CL allows g_client to be changed to nullptr and
then to a new value. Otherwise, since these tests run sequentially in the
same process, g_client may point to freed memory. This will become an
issue as we add more tests here.

Bug: 751242
Change-Id: I1bfa8f12f1fbbd5e086ae3c033e038b93c5a18ce
Reviewed-on: https://chromium-review.googlesource.com/593052
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491645}

chrome/browser/browser_process_impl.cc

@@ -257,7 +257,6 @@ BrowserProcessImpl::BrowserProcessImpl(
   device_client_.reset(new ChromeDeviceClient);
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  // Athena sets its own instance during Athena's init process.
   extensions::AppWindowClient::Set(ChromeAppWindowClient::GetInstance());
 
   extension_event_router_forwarder_ = new extensions::EventRouterForwarder;
@@ -284,6 +283,7 @@ BrowserProcessImpl::~BrowserProcessImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   extensions::ExtensionsBrowserClient::Set(nullptr);
+  extensions::AppWindowClient::Set(nullptr);
 #endif
 
 #if !defined(OS_ANDROID)

chrome/test/base/testing_browser_process.cc

@@ -92,6 +92,7 @@ TestingBrowserProcess::~TestingBrowserProcess() {
   ShutdownBrowserPolicyConnector();
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   extensions::ExtensionsBrowserClient::Set(nullptr);
+  extensions::AppWindowClient::Set(nullptr);
 #endif
 
   // Destructors for some objects owned by TestingBrowserProcess will use

extensions/browser/app_window/app_window_client.cc

@@ -4,12 +4,13 @@
 
 #include "extensions/browser/app_window/app_window_client.h"
 
+#include "base/logging.h"
 
 namespace extensions {
 
 namespace {
 
-AppWindowClient* g_client = NULL;
+AppWindowClient* g_client = nullptr;
 
 }  // namespace
 
@@ -18,9 +19,16 @@ AppWindowClient* AppWindowClient::Get() {
 }
 
 void AppWindowClient::Set(AppWindowClient* client) {
-  // This can happen in unit tests, where the utility thread runs in-process.
-  if (g_client)
+  // Unit tests that set the AppWindowClient should clear it afterward.
+  if (g_client && client) {
+    // Rarely, a test may run multiple BrowserProcesses in a single process:
+    // crbug.com/751242. This will lead to redundant calls, but the pointers
+    // should at least be the same.
+    DCHECK_EQ(g_client, client)
+        << "AppWindowClient::Set called with different non-null pointers twice "
+        << "in a row. A previous test may have set this without clearing it.";
     return;
+  }
 
   g_client = client;
 }

extensions/shell/browser/shell_native_app_window_aura_unittest.cc

@@ -29,7 +29,7 @@ class ShellNativeAppWindowAuraTest : public ExtensionsTest {
     AppWindowClient::Set(&app_window_client_);
   }
 
-  ~ShellNativeAppWindowAuraTest() override {}
+  ~ShellNativeAppWindowAuraTest() override { AppWindowClient::Set(nullptr); }
 
  protected:
   ShellAppWindowClient app_window_client_;