extensions/EasyUnlockPrivateApi: Clean up BrowserContext-referencing dependencies in Shutdown.

Waiting to clean up these dependencies in the destructor means the
references to BrowserContext happen after BrowserContext has already
been torn down (leading to a crash).

Bug: 824989
Test: EasyUnlockPrivateApiTest.BrowserContextTearDown
Change-Id: I86ef6efb97594f0f066f2b1ecdebbe9d5aa81b83
Reviewed-on: https://chromium-review.googlesource.com/981774Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Tim Song <tengs@chromium.org>
Commit-Queue: James Hawkins <jhawkins@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546193}

chrome/browser/extensions/api/easy_unlock_private/OWNERS

+jhawkins@chromium.org
 tbarzic@chromium.org
 tengs@chromium.org
 

chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api.cc

@@ -127,6 +127,11 @@ EasyUnlockPrivateCryptoDelegate* EasyUnlockPrivateAPI::GetCryptoDelegate() {
   return crypto_delegate_.get();
 }
 
+void EasyUnlockPrivateAPI::Shutdown() {
+  // Any dependency which references BrowserContext must be cleaned up here.
+  connection_manager_.reset();
+}
+
 EasyUnlockPrivateGetStringsFunction::EasyUnlockPrivateGetStringsFunction() {
 }
 EasyUnlockPrivateGetStringsFunction::~EasyUnlockPrivateGetStringsFunction() {

chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api.h

@@ -64,6 +64,9 @@ class EasyUnlockPrivateAPI : public BrowserContextKeyedAPI {
   // BrowserContextKeyedAPI implementation.
   static const char* service_name() { return "EasyUnlockPrivate"; }
 
+  // KeyedService implementation.
+  void Shutdown() override;
+
   std::unique_ptr<EasyUnlockPrivateCryptoDelegate> crypto_delegate_;
 
   std::unique_ptr<EasyUnlockPrivateConnectionManager> connection_manager_;

chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc

@@ -10,12 +10,16 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/login/easy_unlock/easy_unlock_app_manager.h"
 #include "chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_factory.h"
 #include "chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_regular.h"
+#include "chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_connection_manager.h"
 #include "chrome/browser/extensions/extension_api_unittest.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
 #include "chrome/browser/extensions/extension_system_factory.h"
@@ -23,26 +27,45 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/api/easy_unlock_private.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "chrome/test/base/testing_profile.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_easy_unlock_client.h"
+#include "components/cryptauth/cryptauth_test_util.h"
+#include "components/cryptauth/fake_connection.h"
 #include "components/cryptauth/proto/cryptauth_api.pb.h"
 #include "components/proximity_auth/switches.h"
 #include "device/bluetooth/dbus/bluez_dbus_manager.h"
 #include "extensions/browser/api_test_utils.h"
+#include "extensions/browser/browser_context_keyed_api_factory.h"
 #include "extensions/browser/test_event_router.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/value_builder.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 #include "ui/aura/env.h"
 
+// TODO(jhawkins): Wrap in extensions namespace.
 namespace {
 
 namespace api = extensions::api::easy_unlock_private;
 
+using cryptauth::FakeConnection;
+using cryptauth::CreateLERemoteDeviceForTest;
+
+using extensions::BrowserContextKeyedAPIFactory;
+using extensions::DictionaryBuilder;
+using extensions::EasyUnlockPrivateAPI;
+using extensions::EasyUnlockPrivateCreateSecureMessageFunction;
+using extensions::EasyUnlockPrivateConnectionManager;
 using extensions::EasyUnlockPrivateGenerateEcP256KeyPairFunction;
 using extensions::EasyUnlockPrivatePerformECDHKeyAgreementFunction;
-using extensions::EasyUnlockPrivateCreateSecureMessageFunction;
-using extensions::EasyUnlockPrivateUnwrapSecureMessageFunction;
 using extensions::EasyUnlockPrivateSetAutoPairingResultFunction;
+using extensions::EasyUnlockPrivateUnwrapSecureMessageFunction;
+using extensions::Extension;
+using extensions::ExtensionBuilder;
+using extensions::ListBuilder;
 
 class TestableGetRemoteDevicesFunction
     : public extensions::EasyUnlockPrivateGetRemoteDevicesFunction {
@@ -119,6 +142,25 @@ void CopyData(std::string* data_target, const std::string& data_source) {
   *data_target = data_source;
 }
 
+EasyUnlockPrivateConnectionManager* GetConnectionManager(
+    content::BrowserContext* context) {
+  return BrowserContextKeyedAPIFactory<EasyUnlockPrivateAPI>::Get(context)
+      ->get_connection_manager();
+}
+
+scoped_refptr<const Extension> CreateTestExtension() {
+  return ExtensionBuilder()
+      .SetManifest(
+          DictionaryBuilder()
+              .Set("name", "Extension")
+              .Set("version", "1.0")
+              .Set("manifest_version", 2)
+              .Set("permissions", ListBuilder().Append("<all_urls>").Build())
+              .Build())
+      .SetID("test")
+      .Build();
+}
+
 class EasyUnlockPrivateApiTest : public extensions::ExtensionApiUnittest {
  public:
   EasyUnlockPrivateApiTest() {}
@@ -512,4 +554,23 @@ TEST_F(EasyUnlockPrivateApiTest, AutoPairing) {
   EXPECT_TRUE(result.error.empty());
 }
 
+// Tests that no BrowserContext dependencies of EasyUnlockPrivateApi (and its
+// dependencies) are referenced after the BrowserContext is torn down. The test
+// fails with a crash if such a condition exists.
+TEST_F(EasyUnlockPrivateApiTest, BrowserContextTearDown) {
+  EasyUnlockPrivateConnectionManager* manager = GetConnectionManager(profile());
+  ASSERT_TRUE(!!manager);
+
+  // Add a Connection. The shutdown path for EasyUnlockPrivateConnectionManager,
+  // a dependency of EasyUnlockPrivateApi, only references BrowserContext
+  // dependencies if it has a Connection to shutdown.
+  auto extension = CreateTestExtension();
+  auto connection =
+      std::make_unique<FakeConnection>(CreateLERemoteDeviceForTest());
+  manager->AddConnection(extension.get(), std::move(connection), true);
+
+  // The Profile is cleaned up at the end of this scope, and BrowserContext
+  // shutdown logic asserts no browser dependencies are referenced afterward.
+}
+
 }  // namespace