Fix a leak of the new location GURL by NotificationTask when doing a redirect...

Fix a leak of the new location GURL by NotificationTask when doing a redirect.

The problem is scoped_ptr<ResourceRequestDetails> is used to delete an instance of ResourceRedirectDetails, however the base class's destructor is non-virtual, so ResourceRedirectDetails extra field (GURL new_url_) does not get torn down properly.

Note that the blame callstacks in the bug report appear unrelated, because std::string is doing some fancy refcounting under the hood. So as GURLs get passed around, they take over the GURL::spec's memory that got allocated earlier by the network stack.

BUG=http://crbug.com/10873
TEST=The "RedirectTest.*" ui tests when run on linux valgrind bots should show no leaks


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

[eroman, 2009-07-08 06:23:20]

Fix a leak of the new location GURL by NotificationTask when doing a redirect.

The problem is scoped_ptr<ResourceRequestDetails> is used to delete an instance
of ResourceRedirectDetails, however the base class's destructor is non-virtual,
so ResourceRedirectDetails extra field (GURL new_url_) does not get torn down
properly.

Note that the blame callstacks in the bug report appear unrelated, because
std::string is doing some fancy refcounting under the hood. So as GURLs get
passed around, they take over the GURL::spec's memory that got allocated much
earlier, by the network stack.

BUG=http://crbug.com/10873
TEST=The "RedirectTest.*" ui tests when run on linux valgrind bots should show
no leaks.

[brettw, 2009-07-08 15:36:25]

LGTM. thanks!

On Tue, Jul 7, 2009 at 11:23 PM, <eroman@chromium.org> wrote:
> Reviewers: brettw,
>
> Message:
> Fix a leak of the new location GURL by NotificationTask when doing a
> redirect.
>
> The problem is scoped_ptr<ResourceRequestDetails> is used to delete an
> instance of ResourceRedirectDetails, however the base class's destructor
> is non-virtual, so ResourceRedirectDetails extra field (GURL new_url_)
> does not get torn down properly.
>
> Note that the blame callstacks in the bug report appear unrelated,
> because std::string is doing some fancy refcounting under the hood. So
> as GURLs get passed around, they take over the GURL::spec's memory that
> got allocated much earlier, by the network stack.
>
> BUG=3Dhttp://crbug.com/10873
> TEST=3DThe "RedirectTest.*" ui tests when run on linux valgrind bots
> should show no leaks.
>
>
> Description:
> Fix a leak of the new location GURL by NotificationTask when doing a
> redirect.
>
> The problem is scoped_ptr<ResourceRequestDetails> is used to delete an
> instance of ResourceRedirectDetails, however the base class's destructor
> is non-virtual, so ResourceRedirectDetails extra field (GURL new_url_)
> does not get torn down properly.
>
> Note that the blame callstacks in the bug report appear unrelated,
> because std::string is doing some fancy refcounting under the hood. So
> as GURLs get passed around, they take over the GURL::spec's memory
> allocated that got allocated much earlier during the network stack.
>
> BUG=3Dhttp://crbug.com/10873
> TEST=3DThe "RedirectTest.*" ui tests when run on linux valgrind bots
> should show no leaks/
>
>
> Please review this at http://codereview.chromium.org/155204
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 chrome/browser/renderer_host/resource_request_details.h
> =A0M =A0 =A0 tools/valgrind/suppressions.txt
>
>
> Index: tools/valgrind/suppressions.txt
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- tools/valgrind/suppressions.txt =A0 =A0 (revision 20081)
> +++ tools/valgrind/suppressions.txt =A0 =A0 (working copy)
> @@ -494,31 +494,6 @@
> =A0 =A0fun:_ZN11MessageLoop7RunTaskEP4Task
> =A0}
> =A0{
> - =A0 # See http://crbug.com/10871
> - =A0 bug_10871
> - =A0 Memcheck:Leak
> - =A0 fun:_Znwj
> - =A0 fun:_ZNSs4_Rep9_S_createEjjRKSaIcE
> - =A0 fun:_ZNSs4_Rep8_M_cloneERKSaIcEj
> - =A0 fun:_ZNSs7reserveEj
> -
> fun:_ZNK4GURL27ResolveWithCharsetConverterERKSsPN9url_canon16CharsetConve=
rterE
> - =A0 fun:_ZNK4GURL7ResolveERKSs
> - =A0 fun:_ZN17URLRequestHttpJob18IsRedirectResponseEP4GURLPi
> -}
> -{
> - =A0 # See http://crbug.com/10873
> - =A0 bug_10873
> - =A0 Memcheck:Leak
> - =A0 fun:_Znwj
> - =A0 fun:_ZNSs4_Rep9_S_createEjjRKSaIcE
> - =A0 fun:_ZNSs4_Rep8_M_cloneERKSaIcEj
> - =A0 fun:_ZNSs7reserveEj
> - =A0 fun:_ZNK4GURL17ReplaceComponentsERKN9url_canon12ReplacementsIcEE
> - =A0 fun:_ZN13URLRequestJob21NotifyHeadersCompleteEv
> - =A0 fun:_ZN17URLRequestHttpJob21NotifyHeadersCompleteEv
> - =A0 fun:_ZN17URLRequestHttpJob16OnStartCompletedEi
> -}
> -{
> =A0 =A0# See http://crbug.com/11116
> =A0 =A0bug_11116a
> =A0 =A0Memcheck:Leak
> Index: chrome/browser/renderer_host/resource_request_details.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- chrome/browser/renderer_host/resource_request_details.h =A0 =A0 (revi=
sion
> 20081)
> +++ chrome/browser/renderer_host/resource_request_details.h =A0 =A0 (work=
ing
> copy)
> @@ -41,7 +41,7 @@
> =A0 =A0 filter_policy_ =3D info->filter_policy;
> =A0 }
>
> - =A0~ResourceRequestDetails() { }
> + =A0virtual ~ResourceRequestDetails() { }
>
> =A0 const GURL& url() const { return url_; }
> =A0 const GURL& original_url() const { return original_url_; }
>
>
>