Fix double-delete in content service worker code when using libstdc++

This is probably best illustrated by the following piece of stack trace
obtained from chromium 87.0.4276.2 built against libstdc++ instead of
libcxx:
  # base::debug::CollectStackTrace()
  # base::debug::StackTrace::StackTrace()
  # logging::LogMessage::~LogMessage()
  # logging::LogMessage::~LogMessage()
  # base::internal::LockImpl::Try()
  # base::SequenceCheckerImpl::CalledOnValidSequence()
  # base::internal::WeakReference::Flag::Invalidate()
  # base::internal::WeakReferenceOwner::~WeakReferenceOwner()
  # content::ServiceWorkerRegistrationObjectHost::~ServiceWorkerRegistrationObjectHost()
  # std::unique_ptr<>::~unique_ptr()
  # std::_Rb_tree<>::_M_drop_node()
  # std::_Rb_tree<>::_M_erase()
  # content::ServiceWorkerContainerHost::~ServiceWorkerContainerHost()
  # std::default_delete<>::operator()()
  # content::ServiceWorkerHost::~ServiceWorkerHost()
  # std::default_delete<>::operator()()
  # std::unique_ptr<>::~unique_ptr()
  # content::ServiceWorkerVersion::~ServiceWorkerVersion()
  # content::ServiceWorkerRegistration::~ServiceWorkerRegistration()
  # content::ServiceWorkerRegistrationObjectHost::~ServiceWorkerRegistrationObjectHost()

Its important to note here that all the calls to the
~ServiceWrokerRegistrationObjectHost happen with the same "this"
object, meaning this is a double-delete bug. It only happens with libstdc++
due to a difference in std::map::erase method implementation. The
difference is illustrated by the following example.

  #include <map>
  #include <memory>
  #include <stdio.h>

  struct A;
  std::map<int, std::unique_ptr<A>> g_map;
  struct A {
    ~A() { printf("Map size on A DTOR call: %zd\n", g_map.size()); }
  };
  int main(int argc, char* argv[]) {
    g_map[0] = std::make_unique<A>();
    g_map.erase(0);
  }

When compiled with clang trunk on goldbot.org with -stdlib=libc++ command
line argument the size of std::map is reported as 0. When using the same
compiler but with -stdlib=libstdc++ arg specified instead, the size is
reported as 1.

As far as I can tell the behavior here is not defined by any C++
standard version, its an implementation detail.

Bug: 957519, 1135070
Change-Id: Ic7d354c2129cdf790f4731a01f8c995021d23eff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2509793
Commit-Queue: Piotr Tworek <ptworek@vewd.com>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823974}

content/browser/service_worker/service_worker_container_host.cc

@@ -620,6 +620,30 @@ void ServiceWorkerContainerHost::RemoveServiceWorkerRegistrationObjectHost(
     int64_t registration_id) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
   DCHECK(base::Contains(registration_object_hosts_, registration_id));
+  // This is a workaround for a really unfavorable ownership structure of
+  // service worker content code. This boils down to the following ownership
+  // cycle:
+  // 1. This class owns ServiceWorkerRegistrationObjectHost via std::unique_ptr
+  //    in registration_object_hosts_.
+  // 2. The ServiceWorkerRegistrationObjectHost has a
+  //    scoped_refptr<ServiceWorkerRegistration> registration_ member.
+  // 3. The ServiceWorkerRegistration has multiple
+  //    scoped_refptr<ServiceWorkerVersion> members.
+  // 4. ServiceWorkerVersion has a std::unique_ptr<ServiceWorkerHost>
+  //    worker_host_ member.
+  // 5. ServiceWorkerHost in turn owns an instance of this class via
+  //    its worker_host_ member.
+  // What this all means is that erasing the registration_id here can actually
+  // lead to "this" ending up being destroyed after we exit from the erase
+  // call. This might not seem fatal, but is when using libstdc++. Apparently
+  // the C++ standard does not define when the destructor of the value from the
+  // map gets called. In libcxx its called after the key has been removed from
+  // the map, while in libstdc++ the destructor gets called first and then
+  // the key is removed before the erase call returns. This means that in
+  // case of libstdc++ the value we're removing from the map in the erase call
+  // can be deleted a second time when registration_object_hosts_ destructor
+  // gets called in ~ServiceWorkerContainerHost.
+  auto to_be_deleted = std::move(registration_object_hosts_[registration_id]);
   registration_object_hosts_.erase(registration_id);
 }
 

content/browser/service_worker/service_worker_object_host_unittest.cc

@@ -17,6 +17,7 @@
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_object_host.h"
 #include "content/browser/service_worker/service_worker_registration.h"
+#include "content/browser/service_worker/service_worker_registration_object_host.h"
 #include "content/browser/service_worker/service_worker_test_utils.h"
 #include "content/browser/service_worker/service_worker_version.h"
 #include "content/public/browser/render_frame_host.h"
@@ -211,6 +212,24 @@ class ServiceWorkerObjectHostTest : public testing::Test {
     object_host->OnConnectionError();
   }
 
+  void CallOnConnectionErrorForRegistrationObjectHost(
+      ServiceWorkerContainerHost* container_host,
+      int64_t version_id,
+      int64_t registration_id) {
+    ServiceWorkerObjectHost* object_host =
+        GetServiceWorkerObjectHost(container_host, version_id);
+    ServiceWorkerRegistrationObjectHost* registration_object_host =
+        container_host->registration_object_hosts_[registration_id].get();
+    EXPECT_FALSE(object_host->version_->HasOneRef());
+
+    object_host->receivers_.Clear();
+    object_host->OnConnectionError();
+
+    EXPECT_TRUE(registration_object_host->registration_->HasOneRef());
+    registration_object_host->receivers_.Clear();
+    registration_object_host->OnConnectionError();
+  }
+
   BrowserTaskEnvironment task_environment_;
   std::unique_ptr<EmbeddedWorkerTestHelper> helper_;
   scoped_refptr<ServiceWorkerRegistration> registration_;
@@ -445,5 +464,39 @@ TEST_F(ServiceWorkerObjectHostTest, OnConnectionError) {
   base::RunLoop().RunUntilIdle();
 }
 
+// This is a regression test for https://crbug.com/1135070
+TEST_F(ServiceWorkerObjectHostTest,
+       OnConnectionErrorForRegistrationObjectHost) {
+  const GURL scope("https://www.example.com/");
+  const GURL script_url("https://www.example.com/service_worker.js");
+  Initialize(std::make_unique<EmbeddedWorkerTestHelper>(base::FilePath()));
+  SetUpRegistration(scope, script_url);
+
+  // Make sure ServiceWorkerRegistration holds a reference to
+  // ServiceWorkerVersion.
+  registration_->SetActiveVersion(version_);
+
+  // Create the provider host.
+  ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk,
+            StartServiceWorker(version_.get()));
+
+  // Set up the case where ServiceWorkerObjectHost and
+  // ServiceWorkerRegistration owned by ServiceWorkerRegistrationObjectHost
+  // hold the last two references to ServiceWorkerVersion.
+  ServiceWorkerContainerHost* container_host =
+      version_->worker_host()->container_host();
+  auto registration_id = registration_->id();
+  auto version_id = version_->version_id();
+  version_ = nullptr;
+  registration_ = nullptr;
+
+  // Simulate the connection error that induces the container host destruction
+  // from ServiceWorkerContainerHost::RemoveServiceWorkerRegistrationObjectHost.
+  // This shouldn't crash.
+  CallOnConnectionErrorForRegistrationObjectHost(container_host, version_id,
+                                                 registration_id);
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace service_worker_object_host_unittest
 }  // namespace content

content/browser/service_worker/service_worker_registration_object_host.h

@@ -22,6 +22,10 @@ class ServiceWorkerContainerHost;
 class ServiceWorkerContextCore;
 class ServiceWorkerVersion;
 
+namespace service_worker_object_host_unittest {
+class ServiceWorkerObjectHostTest;
+}  // namespace service_worker_object_host_unittest
+
 // ServiceWorkerRegistrationObjectHost has a 1:1 correspondence to
 // blink::ServiceWorkerRegistration in the renderer process.
 // The host stays alive while the blink::ServiceWorkerRegistration is alive.
@@ -45,6 +49,7 @@ class CONTENT_EXPORT ServiceWorkerRegistrationObjectHost
 
  private:
   friend class ServiceWorkerRegistrationObjectHostTest;
+  friend class service_worker_object_host_unittest::ServiceWorkerObjectHostTest;
 
   using StatusCallback =
       base::OnceCallback<void(blink::ServiceWorkerStatusCode status)>;