Fix lifetime issues for HeadlessBrowserContext observers.

Bug: 751180, 739458
Change-Id: Iceb120b6ddf94f9e032ad5e22d002a6ab44dbbd5
Reviewed-on: https://chromium-review.googlesource.com/598093
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491700}

headless/lib/browser/headless_browser_context_impl.cc

@@ -92,6 +92,13 @@ HeadlessBrowserContextImpl::HeadlessBrowserContextImpl(
 HeadlessBrowserContextImpl::~HeadlessBrowserContextImpl() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  // Inform observers that we're going away.
+  {
+    base::AutoLock lock(observers_lock_);
+    for (auto& observer : observers_)
+      observer.OnHeadlessBrowserContextDestruct();
+  }
+
   // Destroy all web contents before shutting down storage partitions.
   web_contents_map_.clear();
 

headless/lib/browser/headless_network_delegate.cc

@@ -21,9 +21,17 @@ const char kDevToolsEmulateNetworkConditionsClientId[] =
 
 HeadlessNetworkDelegate::HeadlessNetworkDelegate(
     HeadlessBrowserContextImpl* headless_browser_context)
-    : headless_browser_context_(headless_browser_context) {}
+    : headless_browser_context_(headless_browser_context) {
+  base::AutoLock lock(lock_);
+  if (headless_browser_context_)
+    headless_browser_context_->AddObserver(this);
+}
 
-HeadlessNetworkDelegate::~HeadlessNetworkDelegate() {}
+HeadlessNetworkDelegate::~HeadlessNetworkDelegate() {
+  base::AutoLock lock(lock_);
+  if (headless_browser_context_)
+    headless_browser_context_->RemoveObserver(this);
+}
 
 int HeadlessNetworkDelegate::OnBeforeURLRequest(
     net::URLRequest* request,
@@ -62,7 +70,8 @@ void HeadlessNetworkDelegate::OnResponseStarted(net::URLRequest* request,
 void HeadlessNetworkDelegate::OnCompleted(net::URLRequest* request,
                                           bool started,
                                           int net_error) {
-  if (net_error != net::OK)
+  base::AutoLock lock(lock_);
+  if (headless_browser_context_ && net_error != net::OK)
     headless_browser_context_->NotifyUrlRequestFailed(request, net_error);
 }
 
@@ -98,4 +107,9 @@ bool HeadlessNetworkDelegate::OnCanAccessFile(
   return true;
 }
 
+void HeadlessNetworkDelegate::OnHeadlessBrowserContextDestruct() {
+  base::AutoLock lock(lock_);
+  headless_browser_context_ = nullptr;
+}
+
 }  // namespace headless

headless/lib/browser/headless_network_delegate.h

@@ -7,6 +7,8 @@
 
 #include "base/macros.h"
 
+#include "base/synchronization/lock.h"
+#include "headless/public/headless_browser_context.h"
 #include "net/base/network_delegate_impl.h"
 
 namespace headless {
@@ -14,13 +16,15 @@ class HeadlessBrowserContextImpl;
 
 // We use the HeadlessNetworkDelegate to remove DevTools request headers before
 // requests are actually fetched and for reporting failed network requests.
-class HeadlessNetworkDelegate : public net::NetworkDelegateImpl {
+class HeadlessNetworkDelegate : public net::NetworkDelegateImpl,
+                                public HeadlessBrowserContext::Observer {
  public:
   explicit HeadlessNetworkDelegate(
       HeadlessBrowserContextImpl* headless_browser_context);
   ~HeadlessNetworkDelegate() override;
 
  private:
+  // net::NetworkDelegateImpl implementation:
   int OnBeforeURLRequest(net::URLRequest* request,
                          const net::CompletionCallback& callback,
                          GURL* new_url) override;
@@ -69,6 +73,10 @@ class HeadlessNetworkDelegate : public net::NetworkDelegateImpl {
                        const base::FilePath& original_path,
                        const base::FilePath& absolute_path) const override;
 
+  // HeadlessBrowserContext::Observer implementation:
+  void OnHeadlessBrowserContextDestruct() override;
+
+  base::Lock lock_;  // Protects |headless_browser_context_|.
   HeadlessBrowserContextImpl* headless_browser_context_;  // Not owned.
 
   DISALLOW_COPY_AND_ASSIGN(HeadlessNetworkDelegate);

headless/lib/browser/headless_url_request_context_getter.cc

@@ -11,6 +11,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/task_scheduler/post_task.h"
 #include "content/public/browser/browser_thread.h"
+#include "headless/lib/browser/headless_browser_context_impl.h"
 #include "headless/lib/browser/headless_browser_context_options.h"
 #include "headless/lib/browser/headless_network_delegate.h"
 #include "net/dns/mapped_host_resolver.h"
@@ -56,9 +57,15 @@ HeadlessURLRequestContextGetter::HeadlessURLRequestContextGetter(
     proxy_config_service_ =
         net::ProxyService::CreateSystemProxyConfigService(io_task_runner_);
   }
+  base::AutoLock lock(lock_);
+  headless_browser_context_->AddObserver(this);
 }
 
-HeadlessURLRequestContextGetter::~HeadlessURLRequestContextGetter() {}
+HeadlessURLRequestContextGetter::~HeadlessURLRequestContextGetter() {
+  base::AutoLock lock(lock_);
+  if (headless_browser_context_)
+    headless_browser_context_->RemoveObserver(this);
+}
 
 net::URLRequestContext*
 HeadlessURLRequestContextGetter::GetURLRequestContext() {
@@ -76,8 +83,12 @@ HeadlessURLRequestContextGetter::GetURLRequestContext() {
     } else {
       builder.set_proxy_config_service(std::move(proxy_config_service_));
     }
+
+    {
+      base::AutoLock lock(lock_);
       builder.set_network_delegate(
           base::MakeUnique<HeadlessNetworkDelegate>(headless_browser_context_));
+    }
 
     if (!host_resolver_rules_.empty()) {
       std::unique_ptr<net::HostResolver> host_resolver(
@@ -111,4 +122,9 @@ net::HostResolver* HeadlessURLRequestContextGetter::host_resolver() const {
   return url_request_context_->host_resolver();
 }
 
+void HeadlessURLRequestContextGetter::OnHeadlessBrowserContextDestruct() {
+  base::AutoLock lock(lock_);
+  headless_browser_context_ = nullptr;
+}
+
 }  // namespace headless

headless/lib/browser/headless_url_request_context_getter.h

@@ -30,7 +30,9 @@ namespace headless {
 class HeadlessBrowserContextOptions;
 class HeadlessBrowserContextImpl;
 
-class HeadlessURLRequestContextGetter : public net::URLRequestContextGetter {
+class HeadlessURLRequestContextGetter
+    : public net::URLRequestContextGetter,
+      public HeadlessBrowserContext::Observer {
  public:
   HeadlessURLRequestContextGetter(
       scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
@@ -48,6 +50,9 @@ class HeadlessURLRequestContextGetter : public net::URLRequestContextGetter {
 
   net::HostResolver* host_resolver() const;
 
+  // HeadlessBrowserContext::Observer implementation:
+  void OnHeadlessBrowserContextDestruct() override;
+
  protected:
   ~HeadlessURLRequestContextGetter() override;
 
@@ -66,7 +71,9 @@ class HeadlessURLRequestContextGetter : public net::URLRequestContextGetter {
   std::unique_ptr<net::URLRequestContext> url_request_context_;
   content::ProtocolHandlerMap protocol_handlers_;
   content::URLRequestInterceptorScopedVector request_interceptors_;
-  net::NetLog* net_log_;                                  // Not owned.
+  net::NetLog* net_log_;  // Not owned
+
+  base::Lock lock_;  // Protects |headless_browser_context_|.
   HeadlessBrowserContextImpl* headless_browser_context_;  // Not owned.
 
   DISALLOW_COPY_AND_ASSIGN(HeadlessURLRequestContextGetter);

headless/lib/headless_devtools_client_browsertest.cc

@@ -459,11 +459,7 @@ class TargetDomainCreateAndDeletePageTest
   }
 };
 
-#if defined(OS_WIN)
-DISABLED_HEADLESS_ASYNC_DEVTOOLED_TEST_F(TargetDomainCreateAndDeletePageTest);
-#else
 HEADLESS_ASYNC_DEVTOOLED_TEST_F(TargetDomainCreateAndDeletePageTest);
-#endif
 
 class TargetDomainCreateAndDeleteBrowserContextTest
     : public HeadlessAsyncDevTooledBrowserTest {
@@ -531,12 +527,7 @@ class TargetDomainCreateAndDeleteBrowserContextTest
   std::string browser_context_id_;
 };
 
-#if defined(OS_WIN)
-DISABLED_HEADLESS_ASYNC_DEVTOOLED_TEST_F(
-    TargetDomainCreateAndDeleteBrowserContextTest);
-#else
 HEADLESS_ASYNC_DEVTOOLED_TEST_F(TargetDomainCreateAndDeleteBrowserContextTest);
-#endif
 
 class TargetDomainDisposeContextFailsIfInUse
     : public HeadlessAsyncDevTooledBrowserTest {
@@ -612,13 +603,7 @@ class TargetDomainDisposeContextFailsIfInUse
   std::string page_id_;
 };
 
-// Test is flaky on Linux debug (https://crbug.com/751180)
-#if defined(OS_WIN) || defined(OS_LINUX)
-DISABLED_HEADLESS_ASYNC_DEVTOOLED_TEST_F(
-    TargetDomainDisposeContextFailsIfInUse);
-#else
 HEADLESS_ASYNC_DEVTOOLED_TEST_F(TargetDomainDisposeContextFailsIfInUse);
-#endif
 
 class TargetDomainCreateTwoContexts : public HeadlessAsyncDevTooledBrowserTest,
                                       public target::ExperimentalObserver,
@@ -890,11 +875,7 @@ class TargetDomainCreateTwoContexts : public HeadlessAsyncDevTooledBrowserTest,
   int context_closed_count_ = 0;
 };
 
-#if defined(OS_WIN)
-DISABLED_HEADLESS_ASYNC_DEVTOOLED_TEST_F(TargetDomainCreateTwoContexts);
-#else
 HEADLESS_ASYNC_DEVTOOLED_TEST_F(TargetDomainCreateTwoContexts);
-#endif
 
 class HeadlessDevToolsNavigationControlTest
     : public HeadlessAsyncDevTooledBrowserTest,
@@ -1448,6 +1429,8 @@ class FailedUrlRequestTest : public HeadlessAsyncDevTooledBrowserTest,
   }
 
   void OnLoadEventFired(const page::LoadEventFiredParams&) override {
+    browser_context_->RemoveObserver(this);
+
     base::AutoLock lock(lock_);
     EXPECT_EQ("iframe.html", url_that_failed_to_load_);
     FinishAsynchronousTest();

headless/lib/headless_web_contents_browsertest.cc

@@ -117,6 +117,8 @@ IN_PROC_BROWSER_TEST_F(HeadlessWebContentsTest, WindowOpen) {
   EXPECT_EQ(expected_bounds.size(),
             child->web_contents()->GetContainerBounds().size());
 #endif  // !defined(OS_MACOSX)
+
+  browser_context->RemoveObserver(&observer);
 }
 
 class HeadlessWindowOpenTabSocketTest : public HeadlessBrowserTest,
@@ -239,6 +241,8 @@ IN_PROC_BROWSER_TEST_F(HeadlessWindowOpenTabSocketTest,
 
   RunAsynchronousTest();
   EXPECT_EQ("Embedder sent us: One", message_);
+
+  browser_context->RemoveObserver(this);
 }
 
 class HeadlessNoDevToolsTabSocketTest : public HeadlessBrowserTest,

headless/public/headless_browser_context.h

@@ -88,6 +88,9 @@ class HEADLESS_EXPORT HeadlessBrowserContext::Observer {
   // thread.
   virtual void UrlRequestFailed(net::URLRequest* request, int net_error) {}
 
+  // Indicates the HeadlessBrowserContext is about to be deleted.
+  virtual void OnHeadlessBrowserContextDestruct() {}
+
  protected:
   virtual ~Observer() {}
 };