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

This reverts commit 7ec7b146.

Reason for revert: The crash has now been fixed, really, by r580102 and r580149.

Original change's description:
> 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/1154740
> Reviewed-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}

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

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

Bug: 866769, 854993
Change-Id: Ifacb8a30b851dc0d9808b26e01fecbd3dbbdb45d
Reviewed-on: https://chromium-review.googlesource.com/1180901Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584381}

content/browser/service_worker/service_worker_object_host.cc

@@ -18,8 +18,6 @@ namespace content {
 
 namespace {
 
-const int kMaxDebugLogSize = 256;
-
 using StatusCallback = base::OnceCallback<void(blink::ServiceWorkerStatusCode)>;
 using PrepareExtendableMessageEventCallback =
     base::OnceCallback<bool(mojom::ExtendableMessageEventPtr*)>;
@@ -192,39 +190,21 @@ 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) {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK_CURRENTLY_ON(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() {
-  // 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;
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   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);
@@ -334,34 +314,7 @@ 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,11 +7,9 @@
 
 #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"
@@ -48,8 +46,6 @@ class CONTENT_EXPORT ServiceWorkerObjectHost
                           scoped_refptr<ServiceWorkerVersion> version);
   ~ServiceWorkerObjectHost() override;
 
-  void CrashOnDoubleDelete();
-
   // ServiceWorkerVersion::Observer overrides.
   void OnVersionStateChanged(ServiceWorkerVersion* version) override;
 
@@ -86,12 +82,6 @@ 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;
 
@@ -118,7 +108,6 @@ 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
@@ -131,9 +120,6 @@ 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

@@ -312,18 +312,7 @@ ServiceWorkerProviderHost::ServiceWorkerProviderHost(
 }
 
 ServiceWorkerProviderHost::~ServiceWorkerProviderHost() {
-  // 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));
-  }
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (context_)
     context_->UnregisterProviderHostByClientID(client_uuid_);
@@ -594,8 +583,8 @@ void ServiceWorkerProviderHost::RemoveServiceWorkerRegistrationObjectHost(
 
 void ServiceWorkerProviderHost::RemoveServiceWorkerObjectHost(
     int64_t version_id) {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  CHECK(base::ContainsKey(service_worker_object_hosts_, version_id));
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(base::ContainsKey(service_worker_object_hosts_, version_id));
   service_worker_object_hosts_.erase(version_id);
 }
 

content/browser/service_worker/service_worker_provider_host.h

@@ -697,9 +697,6 @@ 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.