Post a task to the IO thread to add a ResourceContext in ResourceDispatcherHostImpl.

Post a task to the IO thread to add a ResourceContext in ResourceDispatcherHostImpl.

ResourceDispatcherHostImpl::AddResourceContext() is called on the UI thread,
but all other users of |active_resource_contexts_| access it on the IO thread.

Committed: https://crrev.com/3f29b4056e85ed6259b679e9858134422caaf72f
Cr-Commit-Position: refs/heads/master@{#371687}

[Anand Mistry (off Chromium), 2016-01-18 23:28:05]

amistry@chromium.org changed reviewers:
+ mmenke@chromium.org

[Anand Mistry (off Chromium), 2016-01-18 23:28:05]

I found this using tsan. It was happening when an incognito profile was being
created. I've lost the tsan output, but inspecting the profile code suggests
this can happen in Chrome when either:
1. An incognito window is created
2. A new profile is created

[mmenke, 2016-01-19 16:55:57]

On 2016/01/18 23:28:05, Anand Mistry wrote:
> I found this using tsan. It was happening when an incognito profile was being
> created. I've lost the tsan output, but inspecting the profile code suggests
> this can happen in Chrome when either:
> 1. An incognito window is created
> 2. A new profile is created

Thanks for catching this!

It looks like |active_resource_contexts_| was only added to investigate
https://crbug.com/90971, which has since been marked as fixed.  I think it makes
more sense to just remove this code, and the code that calls
ResourceDispatcherHostImpl::AddResourceContext.

[Anand Mistry (off Chromium), 2016-01-19 23:03:22]

On 2016/01/19 16:55:57, mmenke wrote:
> On 2016/01/18 23:28:05, Anand Mistry wrote:
> > I found this using tsan. It was happening when an incognito profile was
being
> > created. I've lost the tsan output, but inspecting the profile code suggests
> > this can happen in Chrome when either:
> > 1. An incognito window is created
> > 2. A new profile is created
> 
> Thanks for catching this!
> 
> It looks like |active_resource_contexts_| was only added to investigate
> https://crbug.com/90971, which has since been marked as fixed.  I think it
makes
> more sense to just remove this code, and the code that calls
> ResourceDispatcherHostImpl::AddResourceContext.

I took a look at crash reports. These CHECKs are still triggering on M49.
Presumably there's still value in keeping these around.

As for the bug, it looks like it's to add the CHECKs to catch a certain class of
bugs. Removing these would un-fix the bug, so to speak.

[mmenke, 2016-01-19 23:25:14]

On 2016/01/19 23:03:22, Anand Mistry wrote:
> On 2016/01/19 16:55:57, mmenke wrote:
> > On 2016/01/18 23:28:05, Anand Mistry wrote:
> > > I found this using tsan. It was happening when an incognito profile was
> being
> > > created. I've lost the tsan output, but inspecting the profile code
suggests
> > > this can happen in Chrome when either:
> > > 1. An incognito window is created
> > > 2. A new profile is created
> > 
> > Thanks for catching this!
> > 
> > It looks like |active_resource_contexts_| was only added to investigate
> > https://crbug.com/90971, which has since been marked as fixed.  I think it
> makes
> > more sense to just remove this code, and the code that calls
> > ResourceDispatcherHostImpl::AddResourceContext.
> 
> I took a look at crash reports. These CHECKs are still triggering on M49.
> Presumably there's still value in keeping these around.
> 
> As for the bug, it looks like it's to add the CHECKs to catch a certain class
of
> bugs. Removing these would un-fix the bug, so to speak.

Ah, right...These are mostly cases where we receive messages from process that
have had their RenderViewHosts destroyed, because the filter code is refcounted.
 Not an RDH bug, but real issues.

I think it makes more sense to have ResourceContext::ResourceContext() post task
over to the IO thread, rather than to use locks (The destructor must be called
on the IO thread, so no changes needed there)

[Anand Mistry (off Chromium), 2016-01-19 23:41:54]

On 2016/01/19 23:25:14, mmenke wrote:
> On 2016/01/19 23:03:22, Anand Mistry wrote:
> > On 2016/01/19 16:55:57, mmenke wrote:
> > > On 2016/01/18 23:28:05, Anand Mistry wrote:
> > > > I found this using tsan. It was happening when an incognito profile was
> > being
> > > > created. I've lost the tsan output, but inspecting the profile code
> suggests
> > > > this can happen in Chrome when either:
> > > > 1. An incognito window is created
> > > > 2. A new profile is created
> > > 
> > > Thanks for catching this!
> > > 
> > > It looks like |active_resource_contexts_| was only added to investigate
> > > https://crbug.com/90971, which has since been marked as fixed.  I think it
> > makes
> > > more sense to just remove this code, and the code that calls
> > > ResourceDispatcherHostImpl::AddResourceContext.
> > 
> > I took a look at crash reports. These CHECKs are still triggering on M49.
> > Presumably there's still value in keeping these around.
> > 
> > As for the bug, it looks like it's to add the CHECKs to catch a certain
class
> of
> > bugs. Removing these would un-fix the bug, so to speak.
> 
> Ah, right...These are mostly cases where we receive messages from process that
> have had their RenderViewHosts destroyed, because the filter code is
refcounted.
>  Not an RDH bug, but real issues.
> 
> I think it makes more sense to have ResourceContext::ResourceContext() post
task
> over to the IO thread, rather than to use locks (The destructor must be called
> on the IO thread, so no changes needed there)

According to the comment in ~ResourceContext, tests can destroy it on the UI
thread. Oh tests.

But as for the constructor, my concern is that there could be an ordering issue
if you post to the IO thread. Even though posted tasks are ordered, there might
be a case where a ResourceContext is destroyed before the added task gets a
chance to run. I can't think of any specific case, but that's my concern.

And if you're concerned about performance, this is far cheaper than posting a
task.

[mmenke, 2016-01-20 00:09:59]

On 2016/01/19 23:41:54, Anand Mistry wrote:
> On 2016/01/19 23:25:14, mmenke wrote:
> > On 2016/01/19 23:03:22, Anand Mistry wrote:
> > > On 2016/01/19 16:55:57, mmenke wrote:
> > > > On 2016/01/18 23:28:05, Anand Mistry wrote:
> > > > > I found this using tsan. It was happening when an incognito profile
was
> > > being
> > > > > created. I've lost the tsan output, but inspecting the profile code
> > suggests
> > > > > this can happen in Chrome when either:
> > > > > 1. An incognito window is created
> > > > > 2. A new profile is created
> > > > 
> > > > Thanks for catching this!
> > > > 
> > > > It looks like |active_resource_contexts_| was only added to investigate
> > > > https://crbug.com/90971, which has since been marked as fixed.  I think
it
> > > makes
> > > > more sense to just remove this code, and the code that calls
> > > > ResourceDispatcherHostImpl::AddResourceContext.
> > > 
> > > I took a look at crash reports. These CHECKs are still triggering on M49.
> > > Presumably there's still value in keeping these around.
> > > 
> > > As for the bug, it looks like it's to add the CHECKs to catch a certain
> class
> > of
> > > bugs. Removing these would un-fix the bug, so to speak.
> > 
> > Ah, right...These are mostly cases where we receive messages from process
that
> > have had their RenderViewHosts destroyed, because the filter code is
> refcounted.
> >  Not an RDH bug, but real issues.
> > 
> > I think it makes more sense to have ResourceContext::ResourceContext() post
> task
> > over to the IO thread, rather than to use locks (The destructor must be
called
> > on the IO thread, so no changes needed there)
> 
> According to the comment in ~ResourceContext, tests can destroy it on the UI
> thread. Oh tests.
> 
> But as for the constructor, my concern is that there could be an ordering
issue
> if you post to the IO thread. Even though posted tasks are ordered, there
might
> be a case where a ResourceContext is destroyed before the added task gets a
> chance to run. I can't think of any specific case, but that's my concern.
> 
> And if you're concerned about performance, this is far cheaper than posting a
> task.

There can't be an ordering issue.  You can issue requests for a RequestContext
until its been constructed, so it would be impossible for a message about what
to do with the RequestContext to make it to the IOThread before the posted task.

I'm not concerned about performance, message passing is just preferred in
chrome, due to deadlock concerns (Though if I were concerned about performance,
we're considering posting a single task on profile startup to grabbing and
releasing a lock for every single network request issued by chrome.  Even given
that obtaining a lock is fairly cheap, and there's never any contention for it,
the task seems much cheaper)

[Anand Mistry (off Chromium), 2016-01-20 02:29:23]

Description was changed from

==========
Add lock around |ResourceDispatcherHostImpl::active_resource_contexts_|, which
can be modified and used on different threads.
==========

to

==========
Post a task to the IO thread to add a ResourceContext in
ResourceDispatcherHostImpl.

ResourceDispatcherHostImpl::AddResourceContext() is called on the UI thread,
but all other users of |active_resource_contexts_| access it on the IO thread.
==========

[Anand Mistry (off Chromium), 2016-01-20 02:29:54]

Description was changed from

==========
Post a task to the IO thread to add a ResourceContext in
ResourceDispatcherHostImpl.

ResourceDispatcherHostImpl::AddResourceContext() is called on the UI thread,
but all other users of |active_resource_contexts_| access it on the IO thread.
==========

to

==========
Post a task to the IO thread to add a ResourceContext in
ResourceDispatcherHostImpl.

ResourceDispatcherHostImpl::AddResourceContext() is called on the UI thread,
but all other users of |active_resource_contexts_| access it on the IO thread.
==========

[Anand Mistry (off Chromium), 2016-01-20 02:31:29]

On 2016/01/20 00:09:59, mmenke wrote:
> On 2016/01/19 23:41:54, Anand Mistry wrote:
> > On 2016/01/19 23:25:14, mmenke wrote:
> > > On 2016/01/19 23:03:22, Anand Mistry wrote:
> > > > On 2016/01/19 16:55:57, mmenke wrote:
> > > > > On 2016/01/18 23:28:05, Anand Mistry wrote:
> > > > > > I found this using tsan. It was happening when an incognito profile
> was
> > > > being
> > > > > > created. I've lost the tsan output, but inspecting the profile code
> > > suggests
> > > > > > this can happen in Chrome when either:
> > > > > > 1. An incognito window is created
> > > > > > 2. A new profile is created
> > > > > 
> > > > > Thanks for catching this!
> > > > > 
> > > > > It looks like |active_resource_contexts_| was only added to
investigate
> > > > > https://crbug.com/90971, which has since been marked as fixed.  I
think
> it
> > > > makes
> > > > > more sense to just remove this code, and the code that calls
> > > > > ResourceDispatcherHostImpl::AddResourceContext.
> > > > 
> > > > I took a look at crash reports. These CHECKs are still triggering on
M49.
> > > > Presumably there's still value in keeping these around.
> > > > 
> > > > As for the bug, it looks like it's to add the CHECKs to catch a certain
> > class
> > > of
> > > > bugs. Removing these would un-fix the bug, so to speak.
> > > 
> > > Ah, right...These are mostly cases where we receive messages from process
> that
> > > have had their RenderViewHosts destroyed, because the filter code is
> > refcounted.
> > >  Not an RDH bug, but real issues.
> > > 
> > > I think it makes more sense to have ResourceContext::ResourceContext()
post
> > task
> > > over to the IO thread, rather than to use locks (The destructor must be
> called
> > > on the IO thread, so no changes needed there)
> > 
> > According to the comment in ~ResourceContext, tests can destroy it on the UI
> > thread. Oh tests.
> > 
> > But as for the constructor, my concern is that there could be an ordering
> issue
> > if you post to the IO thread. Even though posted tasks are ordered, there
> might
> > be a case where a ResourceContext is destroyed before the added task gets a
> > chance to run. I can't think of any specific case, but that's my concern.
> > 
> > And if you're concerned about performance, this is far cheaper than posting
a
> > task.
> 
> There can't be an ordering issue.  You can issue requests for a RequestContext
> until its been constructed, so it would be impossible for a message about what
> to do with the RequestContext to make it to the IOThread before the posted
task.

Thanks for the clarification.

> I'm not concerned about performance, message passing is just preferred in
> chrome, due to deadlock concerns (Though if I were concerned about
performance,
> we're considering posting a single task on profile startup to grabbing and
> releasing a lock for every single network request issued by chrome.  Even
given
> that obtaining a lock is fairly cheap, and there's never any contention for
it,
> the task seems much cheaper)

Ack. Switched to posting a task. PTAL.

[Anand Mistry (off Chromium), 2016-01-20 02:31:40]

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

[commit-bot: I haz the power, 2016-01-20 02:34:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1600263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1600263002/20001

[commit-bot: I haz the power, 2016-01-20 02:48:57]

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

[commit-bot: I haz the power, 2016-01-20 02:48:57]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2016-01-20 15:48:03]

Seems to me like the PostTask should really be in
ResourceContext::ResourceContext - every other method in RDH (Except the
constructor, and maybe the destructor) is IO-thread only.  Having one other
random method that must be called on the UI thread seems weird.

[Anand Mistry (off Chromium), 2016-01-21 00:00:44]

On 2016/01/20 15:48:03, mmenke wrote:
> Seems to me like the PostTask should really be in
> ResourceContext::ResourceContext - every other method in RDH (Except the
> constructor, and maybe the destructor) is IO-thread only.  Having one other
> random method that must be called on the UI thread seems weird.

Gotcha. PTAL.

[mmenke, 2016-01-21 16:33:34]

LGTM!

https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:546:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
May want to add a DCHECK to RemoveResourceContext as well, just for
documentation purposes (And out of paranoia)

[Anand Mistry (off Chromium), 2016-01-21 19:41:52]

https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:546:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2016/01/21 16:33:34, mmenke wrote:
> May want to add a DCHECK to RemoveResourceContext as well, just for
> documentation purposes (And out of paranoia)

The reason I didn't is because of the comment about tests destroying the
ResourceContext on the UI thread.

[mmenke, 2016-01-21 19:45:14]

On 2016/01/21 19:41:52, Anand Mistry wrote:
>
https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
> content/browser/loader/resource_dispatcher_host_impl.cc:546:
> DCHECK_CURRENTLY_ON(BrowserThread::IO);
> On 2016/01/21 16:33:34, mmenke wrote:
> > May want to add a DCHECK to RemoveResourceContext as well, just for
> > documentation purposes (And out of paranoia)
> 
> The reason I didn't is because of the comment about tests destroying the
> ResourceContext on the UI thread.

Ah, good point. My LGTM still stands without it.

[mmenke, 2016-01-21 19:46:40]

On 2016/01/21 19:45:14, mmenke wrote:
> On 2016/01/21 19:41:52, Anand Mistry wrote:
> >
>
https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
> > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
> > content/browser/loader/resource_dispatcher_host_impl.cc:546:
> > DCHECK_CURRENTLY_ON(BrowserThread::IO);
> > On 2016/01/21 16:33:34, mmenke wrote:
> > > May want to add a DCHECK to RemoveResourceContext as well, just for
> > > documentation purposes (And out of paranoia)
> > 
> > The reason I didn't is because of the comment about tests destroying the
> > ResourceContext on the UI thread.
> 
> Ah, good point. My LGTM still stands without it.

Hrm...  That does mean we can add contexts before removing them in tests, but I
don't think that actually breaks anything.

[Anand Mistry (off Chromium), 2016-01-21 20:03:21]

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

[commit-bot: I haz the power, 2016-01-21 20:04:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1600263002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1600263002/40001

[Anand Mistry (off Chromium), 2016-01-21 20:04:46]

On 2016/01/21 19:46:40, mmenke wrote:
> On 2016/01/21 19:45:14, mmenke wrote:
> > On 2016/01/21 19:41:52, Anand Mistry wrote:
> > >
> >
>
https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
> > > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1600263002/diff/40001/content/browser/loader/...
> > > content/browser/loader/resource_dispatcher_host_impl.cc:546:
> > > DCHECK_CURRENTLY_ON(BrowserThread::IO);
> > > On 2016/01/21 16:33:34, mmenke wrote:
> > > > May want to add a DCHECK to RemoveResourceContext as well, just for
> > > > documentation purposes (And out of paranoia)
> > > 
> > > The reason I didn't is because of the comment about tests destroying the
> > > ResourceContext on the UI thread.
> > 
> > Ah, good point. My LGTM still stands without it.
> 
> Hrm...  That does mean we can add contexts before removing them in tests, but
I
> don't think that actually breaks anything.

It's a possibility if nothing depends on the ResourceContext. Dry run started,
lets see what breaks.

[commit-bot: I haz the power, 2016-01-21 21:10:19]

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

[commit-bot: I haz the power, 2016-01-21 21:10:20]

Dry run: This issue passed the CQ dry run.

[Anand Mistry (off Chromium), 2016-01-21 22:28:15]

amistry@chromium.org changed reviewers:
+ kinuko@chromium.org

[Anand Mistry (off Chromium), 2016-01-21 22:28:15]

kinuko@chromium.org: For content OWNERS approval.

[kinuko, 2016-01-26 09:14:18]

lgtm

[Anand Mistry (off Chromium), 2016-01-27 00:40:42]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-01-27 00:40:42]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1600263002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-27 00:41:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1600263002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1600263002/60001

[commit-bot: I haz the power, 2016-01-27 01:56:31]

Description was changed from

==========
Post a task to the IO thread to add a ResourceContext in
ResourceDispatcherHostImpl.

ResourceDispatcherHostImpl::AddResourceContext() is called on the UI thread,
but all other users of |active_resource_contexts_| access it on the IO thread.
==========

to

==========
Post a task to the IO thread to add a ResourceContext in
ResourceDispatcherHostImpl.

ResourceDispatcherHostImpl::AddResourceContext() is called on the UI thread,
but all other users of |active_resource_contexts_| access it on the IO thread.
==========

[commit-bot: I haz the power, 2016-01-27 01:56:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-27 01:57:57]

Description was changed from

==========
Post a task to the IO thread to add a ResourceContext in
ResourceDispatcherHostImpl.

ResourceDispatcherHostImpl::AddResourceContext() is called on the UI thread,
but all other users of |active_resource_contexts_| access it on the IO thread.
==========

to

==========
Post a task to the IO thread to add a ResourceContext in
ResourceDispatcherHostImpl.

ResourceDispatcherHostImpl::AddResourceContext() is called on the UI thread,
but all other users of |active_resource_contexts_| access it on the IO thread.

Committed: https://crrev.com/3f29b4056e85ed6259b679e9858134422caaf72f
Cr-Commit-Position: refs/heads/master@{#371687}
==========

[commit-bot: I haz the power, 2016-01-27 01:57:59]

Patchset 4 (id:??) landed as
https://crrev.com/3f29b4056e85ed6259b679e9858134422caaf72f
Cr-Commit-Position: refs/heads/master@{#371687}