Revert "service worker: Remove instrumentation for ServiceWorkerObjectHost crash."

This reverts commit bdff024c.

Reason for revert:
It turns out the crash was still happening up to 70.0.3501.2, before
this was committed. The crash went away since 70.0.3502.0, but it's
unclear whether that was because the instrumentation was removed or
the "quick fix" was removed at the same time (r577435).

I'm removing the instrumentation to see if the crashes will return.

Original change's description:
> service worker: Remove instrumentation for ServiceWorkerObjectHost crash.
> 
> Remove the instrumentation from issue 854993 (and duped issue 838410) as
> it is now fixed.
> 
> This is mostly a straight revert of the CLs, but it retains some
> checks as DCHECKs and other improvements like adding constness.
> 
> Bug: 866769, 854993
> Change-Id: Id617e06e85e2b947258ba5f80c1d7aa0396888af
> Reviewed-on: https://chromium-review.googlesource.com/1147888
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Commit-Queue: Matt Falkenhagen <falken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577430}

TBR=falken@chromium.org,kinuko@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 866769, 854993
Change-Id: I169f6c894d862008f4cfb0b778cd6f657b8351c0
Reviewed-on: https://chromium-review.googlesource.com/1154740Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579004}

content/browser/service_worker/service_worker_object_host.cc

@@ -18,6 +18,8 @@ namespace content {
 
 namespace {
 
+const int kMaxDebugLogSize = 256;
+
 using StatusCallback = base::OnceCallback<void(blink::ServiceWorkerStatusCode)>;
 using PrepareExtendableMessageEventCallback =
     base::OnceCallback<bool(mojom::ExtendableMessageEventPtr*)>;
@@ -190,21 +192,39 @@ ServiceWorkerObjectHost::ServiceWorkerObjectHost(
     : context_(context),
       provider_host_(provider_host),
       provider_origin_(url::Origin::Create(provider_host->document_url())),
+      provider_type_(provider_host->provider_type()),
       version_(std::move(version)),
       weak_ptr_factory_(this) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(context_ && provider_host_ && version_);
   DCHECK(context_->GetLiveRegistration(version_->registration_id()));
   version_->AddObserver(this);
+  AddToDebugLog(base::StringPrintf("ObjCtor:prov=%p,type=%d,this=%p",
+                                   provider_host_, provider_type_, this));
   bindings_.set_connection_error_handler(base::BindRepeating(
       &ServiceWorkerObjectHost::OnConnectionError, base::Unretained(this)));
 }
 
 ServiceWorkerObjectHost::~ServiceWorkerObjectHost() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  // TODO(crbug.com/838410): These CHECKs are temporary debugging for the linked
+  // bug.
+  AddToDebugLog(base::StringPrintf("ObjDtor:prov=%p,type=%d,this=%p",
+                                   provider_host_, provider_type_, this));
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (in_dtor_) {
+    CrashOnDoubleDelete();
+    return;
+  }
+  in_dtor_ = true;
   version_->RemoveObserver(this);
 }
 
+void ServiceWorkerObjectHost::CrashOnDoubleDelete() {
+  std::string log = ServiceWorkerObjectHost::GetDebugLogString();
+  DEBUG_ALIAS_FOR_CSTR(debug_log, log.c_str(), 2048);
+  CHECK(false);
+}
+
 void ServiceWorkerObjectHost::OnVersionStateChanged(
     ServiceWorkerVersion* version) {
   DCHECK(version);
@@ -314,7 +334,34 @@ void ServiceWorkerObjectHost::OnConnectionError() {
   if (!bindings_.empty())
     return;
   // Will destroy |this|.
+  AddToDebugLog(base::StringPrintf("HostErr:this=%p", this));
   provider_host_->RemoveServiceWorkerObjectHost(version_->version_id());
 }
 
+// static
+ServiceWorkerObjectHost::DebugLog*
+ServiceWorkerObjectHost::GetDebugLogInstance() {
+  static base::NoDestructor<DebugLog> g_log;
+  return g_log.get();
+}
+
+// static
+void ServiceWorkerObjectHost::AddToDebugLog(const std::string& event) {
+  DebugLog* log = GetDebugLogInstance();
+  log->push_back(event);
+  if (log->size() > kMaxDebugLogSize)
+    log->pop_front();
+}
+
+// static
+std::string ServiceWorkerObjectHost::GetDebugLogString() {
+  DebugLog* log = GetDebugLogInstance();
+  std::string result;
+  // Traverse from the end so if the string gets truncated we still
+  // have the most recent events.
+  for (auto iter = log->rbegin(); iter != log->rend(); ++iter)
+    result += *iter + "\n";
+  return result;
+}
+
 }  // namespace content

content/browser/service_worker/service_worker_object_host.h

@@ -7,9 +7,11 @@
 
 #include <memory>
 
+#include "base/containers/circular_deque.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/no_destructor.h"
 #include "content/browser/service_worker/service_worker_version.h"
 #include "content/common/content_export.h"
 #include "content/common/service_worker/service_worker_types.h"
@@ -46,6 +48,8 @@ class CONTENT_EXPORT ServiceWorkerObjectHost
                           scoped_refptr<ServiceWorkerVersion> version);
   ~ServiceWorkerObjectHost() override;
 
+  void CrashOnDoubleDelete();
+
   // ServiceWorkerVersion::Observer overrides.
   void OnVersionStateChanged(ServiceWorkerVersion* version) override;
 
@@ -82,6 +86,12 @@ class CONTENT_EXPORT ServiceWorkerObjectHost
 
   base::WeakPtr<ServiceWorkerObjectHost> AsWeakPtr();
 
+  // TODO(crbug.com/838410): Instrumentation for the linked bug.
+  using DebugLog = base::circular_deque<std::string>;
+  static DebugLog* GetDebugLogInstance();
+  static void AddToDebugLog(const std::string& event);
+  static std::string GetDebugLogString();
+
  private:
   friend class service_worker_object_host_unittest::ServiceWorkerObjectHostTest;
 
@@ -108,6 +118,7 @@ class CONTENT_EXPORT ServiceWorkerObjectHost
   // don't expect that context to change origins and still hold on to the
   // object.
   const url::Origin provider_origin_;
+  const blink::mojom::ServiceWorkerProviderType provider_type_;
   scoped_refptr<ServiceWorkerVersion> version_;
   // Typically both |bindings_| and |remote_objects_| contain only one Mojo
   // connection, corresponding to the content::WebServiceWorkerImpl in the
@@ -120,6 +131,9 @@ class CONTENT_EXPORT ServiceWorkerObjectHost
   mojo::AssociatedInterfacePtrSet<blink::mojom::ServiceWorkerObject>
       remote_objects_;
 
+  // TODO(crbug.com/838410): Temporary debugging for the linked bug.
+  bool in_dtor_ = false;
+
   base::WeakPtrFactory<ServiceWorkerObjectHost> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerObjectHost);

content/browser/service_worker/service_worker_provider_host.cc

@@ -296,7 +296,18 @@ ServiceWorkerProviderHost::ServiceWorkerProviderHost(
 }
 
 ServiceWorkerProviderHost::~ServiceWorkerProviderHost() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  // TODO(crbug.com/838410): The CHECKs are temporary debugging for the linked
+  // bug.
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  CHECK(!in_dtor_);
+  in_dtor_ = true;
+
+  if (!service_worker_object_hosts_.empty()) {
+    // Just log when hosts isn't empty. It'd be too noisy to record all provider
+    // ctor/dtor otherwise.
+    ServiceWorkerObjectHost::AddToDebugLog(
+        base::StringPrintf("ProvDtor:prov=%p,type=%d", this, info_->type));
+  }
 
   if (context_)
     context_->UnregisterProviderHostByClientID(client_uuid_);
@@ -557,8 +568,8 @@ void ServiceWorkerProviderHost::RemoveServiceWorkerRegistrationObjectHost(
 
 void ServiceWorkerProviderHost::RemoveServiceWorkerObjectHost(
     int64_t version_id) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(base::ContainsKey(service_worker_object_hosts_, version_id));
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  CHECK(base::ContainsKey(service_worker_object_hosts_, version_id));
   service_worker_object_hosts_.erase(version_id);
 }
 

content/browser/service_worker/service_worker_provider_host.h

@@ -692,6 +692,9 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   // redirects.
   bool is_execution_ready_ = false;
 
+  // TODO(crbug.com/838410): Temporary debugging for the linked bug.
+  bool in_dtor_ = false;
+
   // For service worker clients. The service workers in the chain of redirects
   // during the main resource request for this client. These workers should be
   // updated "soon". See AddServiceWorkerToUpdate() documentation.