Fixed CHECKs for use of canceled ResourceContexts in ResourceDispatcherHostImpl.

Fixed CHECKs for use of canceled ResourceContexts in ResourceDispatcherHostImpl.

Checks weren't taking into account the possibility of a later allocation re-using a
deleted pointer.

TBR=willchan@chromium.org
BUG=90971
BUG=100566
TEST="browser_tests --gtest_filter=BrowserCloseTest.DISABLED_DownloadsCloseCheck_1 --gtest_repeat=10 --gtest_also_run_disabled_tests" with no crash.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137163

[Randy Smith (Not in Mondays), 2012-05-08 22:42:26]

Will: WDYT?

[Randy Smith (Not in Mondays), 2012-05-11 15:56:28]

Switching to TBR, as Will's LGTM'd over email and I want to use the commit
queue.

[commit-bot: I haz the power, 2012-05-11 15:57:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10389030/1

[willchan no longer on Chromium, 2012-05-11 16:15:08]

lgtm

Sorry, I *thought* I lgtm'd this earlier.

On Tue, May 8, 2012 at 3:42 PM, <rdsmith@chromium.org> wrote:

> Reviewers: willchan,
>
> Message:
> Will: WDYT?
>
>
> Description:
> Fixed CHECKs for use of canceled ResourceContexts in
> ResourceDispatcherHostImpl.
>
> Checks weren't taking into account the possibility of a later allocation
> re-using a
> deleted pointer.
>
> R=willchan@chromium.org
> BUG=90971
> BUG=100566
> TEST="browser_tests
> --gtest_filter=**BrowserCloseTest.DISABLED_**DownloadsCloseCheck_1
> --gtest_repeat=10
> --gtest_also_run_disabled_**tests" with no crash.
>
>
> Please review this at
http://codereview.chromium.**org/10389030/<http://codereview.chromium.org/103...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M content/browser/renderer_host/**resource_dispatcher_host_impl.**h
>  M content/browser/renderer_host/**resource_dispatcher_host_impl.**cc
>  M content/browser/resource_**context_impl.cc
>  M content/public/browser/**resource_context.h
>
>
> Index: content/browser/renderer_host/**resource_dispatcher_host_impl.**cc
> diff --git
a/content/browser/renderer_**host/resource_dispatcher_host_**impl.cc
> b/content/browser/renderer_**host/resource_dispatcher_host_**impl.cc
> index b934e382186ee364c50913677b0a88**80c63dacec..**
> fc568bdbe1b22bce9e722085aa2224**e3b7dee5a5 100644
> --- a/content/browser/renderer_**host/resource_dispatcher_host_**impl.cc
> +++ b/content/browser/renderer_**host/resource_dispatcher_host_**impl.cc
> @@ -371,12 +371,17 @@ void ResourceDispatcherHostImpl::**
> SetAllowCrossOriginAuthPrompt(**bool value) {
>   allow_cross_origin_auth_**prompt_ = value;
>  }
>
> +void ResourceDispatcherHostImpl::**AddResourceContext(**ResourceContext*
> context) {
> +  active_resource_contexts_.**insert(context);
> +}
> +
>  void ResourceDispatcherHostImpl::**CancelRequestsForContext(
>     ResourceContext* context) {
>   DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::IO)**);
>   DCHECK(context);
>
> -  canceled_resource_contexts_.**insert(context);
> +  CHECK(ContainsKey(active_**resource_contexts_, context));
> +  active_resource_contexts_.**erase(context);
>
>   // Note that request cancellation has side effects. Therefore, we gather
> all
>   // the requests to cancel first, and then we start cancelling. We assert
> at
> @@ -481,7 +486,7 @@ net::Error ResourceDispatcherHostImpl::**
> BeginDownload(
>   char url_buf[128];
>   base::strlcpy(url_buf, url.spec().c_str(), arraysize(url_buf));
>   base::debug::Alias(url_buf);
> -  CHECK(!ContainsKey(canceled_**resource_contexts_, context));
> +  CHECK(ContainsKey(active_**resource_contexts_, context));
>
>   const net::URLRequestContext* request_context =
> context->GetRequestContext();
>   request->set_referrer(**MaybeStripReferrer(GURL(**
> request->referrer())).spec());
> @@ -754,7 +759,7 @@ void ResourceDispatcherHostImpl::**BeginRequest(
>
>   ResourceContext* resource_context = filter_->resource_context();
>   // http://crbug.com/90971
> -  CHECK(!ContainsKey(canceled_**resource_contexts_, resource_context));
> +  CHECK(ContainsKey(active_**resource_contexts_, resource_context));
>
>   // Might need to resolve the blob references in the upload data.
>   if (request_data.upload_data) {
> @@ -1162,7 +1167,7 @@ void ResourceDispatcherHostImpl::**BeginSaveFile(
>   char url_buf[128];
>   base::strlcpy(url_buf, url.spec().c_str(), arraysize(url_buf));
>   base::debug::Alias(url_buf);
> -  CHECK(!ContainsKey(canceled_**resource_contexts_, context));
> +  CHECK(ContainsKey(active_**resource_contexts_, context));
>
>   scoped_refptr<ResourceHandler> handler(
>       new SaveFileResourceHandler(child_**id,
> Index: content/browser/renderer_host/**resource_dispatcher_host_impl.**h
> diff --git
a/content/browser/renderer_**host/resource_dispatcher_host_**impl.h
> b/content/browser/renderer_**host/resource_dispatcher_host_**impl.h
> index 7807a447c5b1e834ef14d59066277a**ed86d1aa7f..**
> d99cedd3ce2c8457c8009d547b6aff**5dc9a9cd58 100644
> --- a/content/browser/renderer_**host/resource_dispatcher_host_**impl.h
> +++ b/content/browser/renderer_**host/resource_dispatcher_host_**impl.h
> @@ -88,6 +88,9 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
>   // new requests).  Cancels all pending requests.
>   void Shutdown();
>
> +  // Notify the ResourceDispatcherHostImpl of a new resource context.
> +  void AddResourceContext(**ResourceContext* context);
> +
>   // Force cancels any pending requests for the given |context|. This is
>   // necessary to ensure that before |context| goes away, all requests
>   // for it are dead.
> @@ -514,7 +517,7 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
>
>   // http://crbug.com/90971 - Assists in tracking down use-after-frees on
>   // shutdown.
> -  std::set<const ResourceContext*> canceled_resource_contexts_;
> +  std::set<const ResourceContext*> active_resource_contexts_;
>
>   DISALLOW_COPY_AND_ASSIGN(**ResourceDispatcherHostImpl);
>  };
> Index: content/browser/resource_**context_impl.cc
> diff --git a/content/browser/resource_**context_impl.cc
> b/content/browser/resource_**context_impl.cc
> index db1baeb342e99ca8688f2e10dbbeb9**3f5979de55..**
> 4620658b7542b4bf9f079c159fb0c0**76796ba7bc 100644
> --- a/content/browser/resource_**context_impl.cc
> +++ b/content/browser/resource_**context_impl.cc
> @@ -180,6 +180,11 @@ AppCacheService* ResourceContext::**
> GetAppCacheService(**ResourceContext* context) {
>       context, kAppCacheServicKeyName);
>  }
>
> +ResourceContext::**ResourceContext() {
> +  if (ResourceDispatcherHostImpl::**Get())
> +    ResourceDispatcherHostImpl::**Get()->AddResourceContext(**this);
> +}
> +
>  ResourceContext::~**ResourceContext() {
>   if (ResourceDispatcherHostImpl::**Get())
>     ResourceDispatcherHostImpl::**Get()->**CancelRequestsForContext(this)*
> *;
> Index: content/public/browser/**resource_context.h
> diff --git a/content/public/browser/**resource_context.h
> b/content/public/browser/**resource_context.h
> index 5b4bf5be5a8684797597a675a22385**4da3770ffd..**
> 0bdcff8a9811677747b05c4cfbd54f**626b24217f 100644
> --- a/content/public/browser/**resource_context.h
> +++ b/content/public/browser/**resource_context.h
> @@ -27,6 +27,7 @@ class CONTENT_EXPORT ResourceContext : public
> base::SupportsUserData {
>   static appcache::AppCacheService* GetAppCacheService(
>       ResourceContext* resource_context);
>
> +  ResourceContext();
>   virtual ~ResourceContext();
>   virtual net::HostResolver* GetHostResolver() = 0;
>   virtual net::URLRequestContext* GetRequestContext() = 0;
>
>
>

[commit-bot: I haz the power, 2012-05-11 16:52:07]

Try job failure for 10389030-1 (retry) (retry) on mac_rel for step
"content_unittests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Randy Smith (Not in Mondays), 2012-05-14 17:03:58]

Will: Take another quick look?  Summary: Some tests were calling the cancel
function before destruction of the ResourceContext, so I pulled out
adding/removing from the active list from cancel.  This makes Cancel idempotent,
which I think is the right interface anyway.

[willchan no longer on Chromium, 2012-05-14 17:11:42]

lgtm

[commit-bot: I haz the power, 2012-05-15 16:04:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10389030/12005

[commit-bot: I haz the power, 2012-05-15 17:40:54]

Change committed as 137163