base::Bind: Remove OldCompletionCallback.

base::Bind: Remove OldCompletionCallback.

BUG=none
TEST=none
R=groby,awong,csilv

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

[groby-ooo-7-16, 2012-01-03 18:32:25]

LGTM w/ nits

http://codereview.chromium.org/9064007/diff/1/net/base/completion_callback.h
File net/base/completion_callback.h (right):

http://codereview.chromium.org/9064007/diff/1/net/base/completion_callback.h#...
net/base/completion_callback.h:11: #include "net/base/net_export.h"
Don't need that one any more.

http://codereview.chromium.org/9064007/diff/1/webkit/appcache/appcache_disk_c...
File webkit/appcache/appcache_disk_cache.h (right):

http://codereview.chromium.org/9064007/diff/1/webkit/appcache/appcache_disk_c...
webkit/appcache/appcache_disk_cache.h:95: // The |unused| parameter is a small
hack so that we can have the
Which one is |unused|. I assume you mean |backend|?

[James Hawkins, 2012-01-03 18:39:20]

http://codereview.chromium.org/9064007/diff/1/net/base/completion_callback.h
File net/base/completion_callback.h (right):

http://codereview.chromium.org/9064007/diff/1/net/base/completion_callback.h#...
net/base/completion_callback.h:11: #include "net/base/net_export.h"
On 2012/01/03 18:32:25, groby wrote:
> Don't need that one any more.

Done.

http://codereview.chromium.org/9064007/diff/1/webkit/appcache/appcache_disk_c...
File webkit/appcache/appcache_disk_cache.h (right):

http://codereview.chromium.org/9064007/diff/1/webkit/appcache/appcache_disk_c...
webkit/appcache/appcache_disk_cache.h:95: // The |unused| parameter is a small
hack so that we can have the
On 2012/01/03 18:32:25, groby wrote:
> Which one is |unused|. I assume you mean |backend|? 

Done.

[James Hawkins, 2012-01-03 18:39:36]

Going off PS1 tryjobs since the PS2 changes are trivial.

[groby-ooo-7-16, 2012-01-03 18:40:25]

lgtm

[csilv, 2012-01-03 18:47:29]

lgtm

http://codereview.chromium.org/9064007/diff/9001/net/base/completion_callback.h
File net/base/completion_callback.h (right):

http://codereview.chromium.org/9064007/diff/9001/net/base/completion_callback...
net/base/completion_callback.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
bump all copyright dates to 2012?

http://codereview.chromium.org/9064007/diff/9001/webkit/appcache/appcache_res...
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/9064007/diff/9001/webkit/appcache/appcache_res...
webkit/appcache/appcache_response.cc:8: #include "base/bind_helpers.h"
#include "base/compiler_specific.h" (for ALLOW_THIS_IN_INITIALIZER_LIST)

[James Hawkins, 2012-01-03 18:49:34]

http://codereview.chromium.org/9064007/diff/9001/net/base/completion_callback.h
File net/base/completion_callback.h (right):

http://codereview.chromium.org/9064007/diff/9001/net/base/completion_callback...
net/base/completion_callback.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2012/01/03 18:47:29, csilv wrote:
> bump all copyright dates to 2012?

Done.

http://codereview.chromium.org/9064007/diff/9001/webkit/appcache/appcache_res...
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/9064007/diff/9001/webkit/appcache/appcache_res...
webkit/appcache/appcache_response.cc:8: #include "base/bind_helpers.h"
On 2012/01/03 18:47:29, csilv wrote:
> #include "base/compiler_specific.h" (for ALLOW_THIS_IN_INITIALIZER_LIST)

Done.

[michaeln, 2012-01-04 01:26:07]

Looks like this change may be crashing some layout tests.
http://code.google.com/p/chromium/issues/detail?id=109077
http://code.google.com/p/chromium/issues/detail?id=109068

[awong, 2012-01-04 01:50:28]

Got some style nits.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:362: AppCacheDiskCacheInterface::Entry**
entry_ptr = NULL;
Move into else cause.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:378: OnCreateEntryComplete(entry_ptr, rv);
Call with NULL explicitly.

Same with the pattern above.

Doing this makes it so there's less code tracing when tyring to understand what
the value in OnCreateEntryComplete() is.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:385: AppCacheDiskCacheInterface::Entry**
entry_ptr = NULL;
Move this into the "else if" clause?

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:398: entry_ptr =
new(AppCacheDiskCacheInterface::Entry*);
nit: personally I prefer 
  new AppCacheDiskCacheInterface::Entry*
or
  new AppCacheDiskCacheInterface::Entry*()

cause this looks like a function invocation to me.  But your choice.

[awong, 2012-01-04 01:54:23]

@michaeln: Could you clarify which stack trace are you looking at which
makes you think this change is breaking things?  I'm looking through


http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%2032/buil...

and the only trace that looks related to app cache is the one with
appcache::AppCacheResponseWriter, but on closer inspection it dies in a
section of code that doesn't rely on state we've touched....

-Albert


On Tue, Jan 3, 2012 at 5:50 PM, <ajwong@chromium.org> wrote:

> Got some style nits.
>
>
> http://codereview.chromium.**org/9064007/diff/11002/webkit/**
>
appcache/appcache_response.cc<http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_response.cc>
> File webkit/appcache/appcache_**response.cc (right):
>
> http://codereview.chromium.**org/9064007/diff/11002/webkit/**
>
appcache/appcache_response.cc#**newcode362<http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_response.cc#newcode362>
> webkit/appcache/appcache_**response.cc:362:
> AppCacheDiskCacheInterface::**Entry** entry_ptr = NULL;
> Move into else cause.
>
> http://codereview.chromium.**org/9064007/diff/11002/webkit/**
>
appcache/appcache_response.cc#**newcode378<http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_response.cc#newcode378>
> webkit/appcache/appcache_**response.cc:378:
> OnCreateEntryComplete(entry_**ptr, rv);
> Call with NULL explicitly.
>
> Same with the pattern above.
>
> Doing this makes it so there's less code tracing when tyring to
> understand what the value in OnCreateEntryComplete() is.
>
> http://codereview.chromium.**org/9064007/diff/11002/webkit/**
>
appcache/appcache_response.cc#**newcode385<http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_response.cc#newcode385>
> webkit/appcache/appcache_**response.cc:385:
> AppCacheDiskCacheInterface::**Entry** entry_ptr = NULL;
> Move this into the "else if" clause?
>
> http://codereview.chromium.**org/9064007/diff/11002/webkit/**
>
appcache/appcache_response.cc#**newcode398<http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_response.cc#newcode398>
> webkit/appcache/appcache_**response.cc:398: entry_ptr =
> new(**AppCacheDiskCacheInterface::**Entry*);
> nit: personally I prefer
>  new AppCacheDiskCacheInterface::**Entry*
> or
>  new AppCacheDiskCacheInterface::**Entry*()
>
> cause this looks like a function invocation to me.  But your choice.
>
>
http://codereview.chromium.**org/9064007/<http://codereview.chromium.org/9064...
>

[michaeln, 2012-01-04 02:07:17]

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_di...
File webkit/appcache/appcache_disk_cache.cc (right):

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_di...
webkit/appcache/appcache_disk_cache.cc:133: create_backend_callback_.Cancel();
will this call to Cancel() delete storage for the backend ptr? But
disk_cache::CreateCacheBackend() may still access it after this call returns...
use-after-free error?

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_di...
webkit/appcache/appcache_disk_cache.cc:161: create_backend_callback_.Cancel();
ditto

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:148:
AppCacheResponseReader::~AppCacheResponseReader() {
Is disk_cache_->OpenEntry() 'copy' of the callback still valid  after this dtor
has returned?

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:374: base::Unretained(this),
base::Owned(entry_ptr));
use weak_factory_.GetWeakPtr() here?

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:378: OnCreateEntryComplete(entry_ptr, rv);
On 2012/01/04 01:50:28, awong wrote:
> Call with NULL explicitly.

It's not necessarily a NULL value at this point, rv can be OK.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:398: entry_ptr =
new(AppCacheDiskCacheInterface::Entry*);
On 2012/01/04 01:50:28, awong wrote:
> nit: personally I prefer 
>   new AppCacheDiskCacheInterface::Entry*
> or
>   new AppCacheDiskCacheInterface::Entry*()
> 
> cause this looks like a function invocation to me.  But your choice.

new(T) style looked odd to me too... prefer new T

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:401: base::Unretained(this),
base::Owned(entry_ptr));
use weak_factory_.GetWeakPtr() here?

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.h:188:
base::WeakPtrFactory<AppCacheResponseReader> weak_factory_;
Could the base classes WeakPtrFactory be utilized here?

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.h:253:
base::WeakPtrFactory<AppCacheResponseWriter> weak_factory_;
ditto

[michaeln, 2012-01-04 02:19:01]

I'm looking at the same stack, looks like the method could be running on a
deleted object.

    appcache::AppCacheResponseWriter::OnCreateEntryComplete() [0x958552f]

I'm looking at when the crash started on the flakiness dashboard...
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...
... none before r116158

[michaeln, 2012-01-04 02:28:40]

and here

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...

[michaeln, 2012-01-04 02:33:29]

those links don't work very well with the extra escaping... try these...

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...

[James Hawkins, 2012-01-05 22:32:51]

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_di...
File webkit/appcache/appcache_disk_cache.cc (right):

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_di...
webkit/appcache/appcache_disk_cache.cc:133: create_backend_callback_.Cancel();
On 2012/01/04 02:07:17, michaeln wrote:
> will this call to Cancel() delete storage for the backend ptr? But
> disk_cache::CreateCacheBackend() may still access it after this call
returns...
> use-after-free error?

I think you're right, but I also think this bug exists in the previous code.

CreateBackendCallback::backend_ptr is deleted on release().

Is that correct?

[James Hawkins, 2012-01-05 22:44:56]

These fixes done in http://codereview.chromium.org/9110017

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:148:
AppCacheResponseReader::~AppCacheResponseReader() {
On 2012/01/04 02:07:17, michaeln wrote:
> Is disk_cache_->OpenEntry() 'copy' of the callback still valid  after this
dtor
> has returned? 

No, because of the WeakPtr.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:362: AppCacheDiskCacheInterface::Entry**
entry_ptr = NULL;
On 2012/01/04 01:50:28, awong wrote:
> Move into else cause.

See michaeln's comment below as to why that's not possible.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:374: base::Unretained(this),
base::Owned(entry_ptr));
On 2012/01/04 02:07:17, michaeln wrote:
> use weak_factory_.GetWeakPtr() here?

Done.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:385: AppCacheDiskCacheInterface::Entry**
entry_ptr = NULL;
On 2012/01/04 01:50:28, awong wrote:
> Move this into the "else if" clause?

Done.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:398: entry_ptr =
new(AppCacheDiskCacheInterface::Entry*);
On 2012/01/04 01:50:28, awong wrote:
> nit: personally I prefer 
>   new AppCacheDiskCacheInterface::Entry*
> or
>   new AppCacheDiskCacheInterface::Entry*()
> 
> cause this looks like a function invocation to me.  But your choice.

Done.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:398: entry_ptr =
new(AppCacheDiskCacheInterface::Entry*);
On 2012/01/04 02:07:17, michaeln wrote:
> On 2012/01/04 01:50:28, awong wrote:
> > nit: personally I prefer 
> >   new AppCacheDiskCacheInterface::Entry*
> > or
> >   new AppCacheDiskCacheInterface::Entry*()
> > 
> > cause this looks like a function invocation to me.  But your choice.
> 
> new(T) style looked odd to me too... prefer new T

Done.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:401: base::Unretained(this),
base::Owned(entry_ptr));
On 2012/01/04 02:07:17, michaeln wrote:
> use weak_factory_.GetWeakPtr() here?

Done.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.h:188:
base::WeakPtrFactory<AppCacheResponseReader> weak_factory_;
On 2012/01/04 02:07:17, michaeln wrote:
> Could the base classes WeakPtrFactory be utilized here?

Not without typecasting at each GetWeakPtr(), which would be a loss for
readability.

[michaeln, 2012-01-05 23:34:48]

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_di...
File webkit/appcache/appcache_disk_cache.cc (right):

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_di...
webkit/appcache/appcache_disk_cache.cc:133: create_backend_callback_.Cancel();
> I think you're right, but I also think this bug exists in the previous code.
> 
> CreateBackendCallback::backend_ptr is deleted on release().
> 
> Is that correct?

Don't think so. Like we chatted about, the old CancelableOldCompletionCallback
infrastructure voodoo kept the CreateBackendCallback class alive till it was no
longer needed.

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_response.cc (right):

http://codereview.chromium.org/9064007/diff/11002/webkit/appcache/appcache_re...
webkit/appcache/appcache_response.cc:148:
AppCacheResponseReader::~AppCacheResponseReader() {
On 2012/01/05 22:44:56, James Hawkins wrote:
> On 2012/01/04 02:07:17, michaeln wrote:
> > Is disk_cache_->OpenEntry() 'copy' of the callback still valid  after this
> dtor
> > has returned? 
> 
> No, because of the WeakPtr.

Great, so AppCacheResponseReader::OnOpenEntryComplete won't get called.

More pointedly, is the heap allocated 'entry_ptr' param held via base::OwnPtr by
the disk_cache's copy/ref of the bind closure still valid after this dtor? I
expect it is, just checking?