Defend against RPHI::ForceReleaseWorkerRefCount called twice.

Defend against RPHI::ForceReleaseWorkerRefCount called twice.

If a BrowserContext is destroyed and a new one created with the same address,
RPHI::ForceReleaseWorkerRefCount can be called twice since pointer
comparison is used to determine which context owns a RPHI. See bug
for details.

This patch early returns if ForceReleaseWorkerRefCount was already called.
Another approach could be to clear browser_context_ in
ForceReleaseWorkerRefCount but that could be risky since Cleanup() may
dereference browser_context_.

BUG=661843

[falken, 2016-11-28 07:11:02]

falken@chromium.org changed reviewers:
+ horo@chromium.org

[falken, 2016-11-28 07:11:03]

horo@ can you review?

[falken, 2016-11-28 07:13:21]

The CQ bit was checked by falken@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 07:13:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2528233003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2528233003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1405: //
https://crbug.com/661843.
I think we should introduce RenderProcessHostImpl::OnContextWillBeDestroyed()
which sets browser_context_ to null and calls ForceReleaseWorkerRefCounts().

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

https://codereview.chromium.org/2528233003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2528233003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1405: //
https://crbug.com/661843.
On 2016/11/28 07:26:51, horo wrote:
> I think we should introduce RenderProcessHostImpl::OnContextWillBeDestroyed()
> which sets browser_context_ to null and calls ForceReleaseWorkerRefCounts().

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().

[commit-bot: I haz the power, 2016-11-28 07:57:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 07:57:30]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-28 08:54:41]

https://codereview.chromium.org/2528233003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2528233003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1405: //
https://crbug.com/661843.
On 2016/11/28 07:34:21, falken wrote:
> On 2016/11/28 07:26:51, horo wrote:
> > I think we should introduce
RenderProcessHostImpl::OnContextWillBeDestroyed()
> > which sets browser_context_ to null and calls ForceReleaseWorkerRefCounts().
> 
> 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().

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]

https://codereview.chromium.org/2528233003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2528233003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1405: //
https://crbug.com/661843.
On 2016/11/28 08:54:40, horo wrote:
> On 2016/11/28 07:34:21, falken wrote:
> > On 2016/11/28 07:26:51, horo wrote:
> > > I think we should introduce
> RenderProcessHostImpl::OnContextWillBeDestroyed()
> > > which sets browser_context_ to null and calls
ForceReleaseWorkerRefCounts().
> > 
> > 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().
> 
> 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...

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.