Defend against RPHI::ForceReleaseWorkerRefCount called twice.

content/browser/renderer_host/render_process_host_impl.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Represents the browser side of the browser <--> renderer communication
 // channel. There will be one RenderProcessHost per renderer process.
 
 #include "content/browser/renderer_host/render_process_host_impl.h"
 
 #include <algorithm>
@@ skipping 1381 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!is_worker_ref_count_disabled_);
   DCHECK_GT(worker_ref_count(), 0U);
   --shared_worker_ref_count_;
   if (worker_ref_count() == 0)
     Cleanup();
 }
 
 void RenderProcessHostImpl::ForceReleaseWorkerRefCounts() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(!is_worker_ref_count_disabled_);
+  // ForceReleaseWorkerRefCounts can possibly be called twice if a new
+  // BrowserContext is created at the same address as an one that was just
+  // destroyed, and the new one is also soon destroyed.
+  // https://crbug.com/661843.

[horo, 2016/11/28 07:26:51]

I think we should introduce RenderProcessHostImpl::OnContextWillBeDestroyed()
which sets browser_context_ to null and calls ForceReleaseWorkerRefCounts().

[falken, 2016/11/28 07:34:21]

Hm, I considered this (see CL description) but Cleanup() uses browser_context_
and also sometimes doesn't to run synchronously ("delayed_cleanup_needed"). So
I'm not sure it's safe to clear browser_context_ even after calling Cleanup().

[horo, 2016/11/28 08:54:40]

After BrowserContext::NotifyWillBeDestroyed() is called, we should not refer the
browser_context.
So we should set browser_context_ to null in the end of
RenderProcessHostImpl::OnContextWillBeDestroyed().
 RenderProcessHostImpl::OnContextWillBeDestroyed() {
   ForceReleaseWorkerRefCounts();
   browser_context_ = nullptr;
 }

The browser_context may be alive even after
BrowserContext::NotifyWillBeDestroyed().
But this is a bug that MUST BE FIXED.
https://chromium.googlesource.com/chromium/src/+/b15e190/chrome/browser/profi...

[falken, 2016/11/29 04:19:36]

Hm yea I'm no longer sure of either solution. //content assumes that
BrowserContext outlives its RPHI. So any tricks to prevent a stale
browser_context_ pointer are just working around the fact that somehow this
expectation is violated, and a DCHECK like ProfileDestroyer above would be
better to make the violation explicitly known.

+  if (is_worker_ref_count_disabled_)
+    return;
   is_worker_ref_count_disabled_ = true;
   if (!worker_ref_count())
     return;
   service_worker_ref_count_ = 0;
   shared_worker_ref_count_ = 0;
   Cleanup();
 }
 
 bool RenderProcessHostImpl::IsWorkerRefCountDisabled() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ skipping 1586 matching lines @@
   LOG(ERROR) << "Terminating render process for bad Mojo message: " << error;
 
   // The ReceivedBadMessage call below will trigger a DumpWithoutCrashing. Alias
   // enough information here so that we can determine what the bad message was.
   base::debug::Alias(&error);
   bad_message::ReceivedBadMessage(render_process_id,
                                   bad_message::RPH_MOJO_PROCESS_ERROR);
 }
 
 }  // namespace content