Remove void** from disk_cache interface.

Remove void** from disk_cache interface.

Enumeration and iteration were passing around void**. With this CL, we
instead use an Iterator object.

R=clamy@chromium.org,jkarlin@chromium.org,jsbell@chromium.org
BUG=413644
Committed: https://crrev.com/732c8306d4864296511e7a3a252724b1bb34c342
Cr-Commit-Position: refs/heads/master@{#295659}

[gavinp, 2014-09-04 17:52:53]

clamy, PTAL.

jkarling, jsbell: FYI.

https://codereview.chromium.org/542733002/diff/120001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/542733002/diff/120001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:79: virtual ~Backend();
As long as we were adding disk_cache.cc (for the body of
EnumerationState::~EnumerationState()), it seemed lke a good idea to move this
there as well, in keeping with
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
.

It saves the linker a bit of churn, anyway.

https://codereview.chromium.org/542733002/diff/120001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:302: virtual ~Entry();
(see discussion at
https://codereview.chromium.org/542733002/diff/120001/net/disk_cache/disk_cac...
for an explanation of this)

[clamy, 2014-09-04 18:22:01]

Thanks! It looks _a lot_ better without the void**. The only thing I am a bit on
verge on is the declaration of the State classes in the function definitions. I
find that a bit hard to read. Is it used in the code base in other places?

https://chromiumcodereview.appspot.com/542733002/diff/120001/net/disk_cache/b...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://chromiumcodereview.appspot.com/542733002/diff/120001/net/disk_cache/b...
net/disk_cache/blockfile/backend_impl.cc:1206: class State : public
EnumerationState {
Maybe it's just me, but I find class definitions inside a function a bit hard to
read. Especially with a function argument called iter, and a class member iter_
- which are quite different, as far as I can tell.

[gavinp, 2014-09-04 18:31:46]

https://chromiumcodereview.appspot.com/542733002/diff/120001/net/disk_cache/b...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://chromiumcodereview.appspot.com/542733002/diff/120001/net/disk_cache/b...
net/disk_cache/blockfile/backend_impl.cc:1206: class State : public
EnumerationState {
On 2014/09/04 18:22:01, clamy wrote:
> Maybe it's just me, but I find class definitions inside a function a bit hard
to
> read. Especially with a function argument called iter, and a class member
iter_
> - which are quite different, as far as I can tell.

As it happens, I had to move this out of the function in BackendImpl, in order
to support building the dump_cache --upgrade feature, a windows only
functionality of dump_cache that depends on BackendImpl::OpenPrevEntry in the
blockfile backend.

So perhaps you'll like the latest upload; I renamed the confusing iter method
and member to the less confusing "data", which seems like it describes a void**
just fine.

I slightly prefer declaring these classes inside the functions; the reason is
just that we're doing some crazy stuff with static casts, and declaring it in
the function makes a strong statement about where you can expect that to occur,
and where the Backend::Iterator() will obey its normal interface. Do you have
the same concerns about the maybe more readable call sites in SimpleBackendImpl
or MemoryBackendImpl?

[clamy, 2014-09-04 19:01:37]

Thanks. The new version lgtm.

https://chromiumcodereview.appspot.com/542733002/diff/120001/net/disk_cache/b...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://chromiumcodereview.appspot.com/542733002/diff/120001/net/disk_cache/b...
net/disk_cache/blockfile/backend_impl.cc:1206: class State : public
EnumerationState {
On 2014/09/04 18:31:46, gavinp wrote:
> On 2014/09/04 18:22:01, clamy wrote:
> > Maybe it's just me, but I find class definitions inside a function a bit
hard
> to
> > read. Especially with a function argument called iter, and a class member
> iter_
> > - which are quite different, as far as I can tell.
> 
> As it happens, I had to move this out of the function in BackendImpl, in order
> to support building the dump_cache --upgrade feature, a windows only
> functionality of dump_cache that depends on BackendImpl::OpenPrevEntry in the
> blockfile backend.
> 
> So perhaps you'll like the latest upload; I renamed the confusing iter method
> and member to the less confusing "data", which seems like it describes a
void**
> just fine.
> 
> I slightly prefer declaring these classes inside the functions; the reason is
> just that we're doing some crazy stuff with static casts, and declaring it in
> the function makes a strong statement about where you can expect that to
occur,
> and where the Backend::Iterator() will obey its normal interface. Do you have
> the same concerns about the maybe more readable call sites in
SimpleBackendImpl
> or MemoryBackendImpl?

Yes the version with "data" is easier to follow. I think it should be fine for
the other call sites.

By the way, why is the upgrade feature using OpenPrevEntry instead of
OpenNextEntry? And is it really used?

[gavinp, 2014-09-04 19:21:34]

Thanks for the review clamy!

On 2014/09/04 19:01:37, clamy wrote:
>
> By the way, why is the upgrade feature using OpenPrevEntry instead of
> OpenNextEntry? And is it really used?
>

I started a thread on net-dev on this very topic, and the answer seems to be
"No."

https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/wlMR_zNJ85k

I'll land a quick upstream CL to remove this function, then land this CL rebased
on top of that.

[gavinp, 2014-09-05 01:41:32]

I've just rebased this CL on top of https://codereview.chromium.org/544923002/
and https://codereview.chromium.org/547513002/ , which remove the tracing
backend (which was only used in tests) and the windows only dump_cache upgrade
feature (which was not used).

The impact on this CL is that it's tighter and cleaner (particularly in
backend_impl.cc), and has fewer files. The upstream CLs are removing about 1700
lines of code, which seems useful!

[rvargas (doing something else), 2014-09-05 02:01:27]

rvargas@chromium.org changed reviewers:
+ rvargas@chromium.org

[rvargas (doing something else), 2014-09-05 02:01:29]

I'm only looking at the API at this point.

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.cc (right):

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.cc:9: Backend::EnumerationState::~EnumerationState()
{}
I think I'm missing something here. Why this file?

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:70: typedef scoped_ptr<EnumerationState> Iterator;
I don't think we need (or want) an extra typedef to hide a scoped_ptr

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:138: virtual int OpenNextEntry(Iterator* iter,
Entry** next_entry,
So far I'm unconvinced about the API change.

The current pattern is pretty standard: the cache returns an opaque handle to
the consumer and the consumer uses it for further enumerations until it doesn't
want to enumerate anymore. The consumer cannot do any operation directly with
that handle.

Sure, it is a C-style pattern, but it is extremely common and makes sure the
cache is in control of the iteration at all times.

What you propose opens up the caller to know, and do more with the iterator than
it should. It also poses restrictions on the implementation that I don't see
fully justified: deletion of the object is tied to the consumer[1] and cleanup
when the cache goes away may be more complicated (as now it has to make sure
that anything related to an iterator can be left behind because it gave up
ownership to the consumer, who really should not care about what is tied to the
iterator).

[1] Arguably, calling a function to say that the iterator is not needed is
basically the same as directly deleting the iterator, but there is a conceptual
difference between "I delete this because I own it, and I know what to do with
it" and "I don't know what this is and I don't need it anymore so I'll tell the
owner about it". It limits what the cache can do with it, or may force an extra
layer of isolation just for the sake of it. I still think that the second
message is the one that the API should deliver.

Another way to put it is that so far I much rather let the cache cleanup all
enumerations at destruction (and leave the caller with handles that are invalid,
but there's no cache to call), than leave a bunch of enumerators lying around
after the cache is gone.

[pasko, 2014-09-05 09:53:49]

pasko@chromium.org changed reviewers:
+ pasko@chromium.org

[pasko, 2014-09-05 09:53:51]

https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:137: // iteration, then the result of this function
is thereafter undefined.
This explicit statement about undefined behavior basically means we cannot use
enumeration for anything except enumerating cache on the start, the way the
shader cache is using it. If shader cache is the only usecase for enumeration,
we may only have one memory leak per backend, right?

The original implementation by clamy@ made a snapshot of all entries in
StartEnumeration, and OpenNextEntry would iterate only through those in the
snapshot except the ones that were removed in the meantime, modulo hash
collisions. I am more comfortable with this definition being put as the API
spec, though I am not sure it is what the blockfile backend does. Simplecache
definition of iteration allows arbitrary number of concurrent iterations and
cache modifications. And I would argue that Camille's implementation for that is
pretty simple.

Big question: Do we plan to add more cache enumeration clients?

[pasko, 2014-09-05 10:32:42]

On 2014/09/05 09:53:51, pasko wrote:
>
https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
> File net/disk_cache/disk_cache.h (right):
> 
>
https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
> net/disk_cache/disk_cache.h:137: // iteration, then the result of this
function
> is thereafter undefined.
> This explicit statement about undefined behavior basically means we cannot use
> enumeration for anything except enumerating cache on the start, the way the
> shader cache is using it. If shader cache is the only usecase for enumeration,
> we may only have one memory leak per backend, right?
> 
> The original implementation by clamy@ made a snapshot of all entries in
> StartEnumeration, and OpenNextEntry would iterate only through those in the
> snapshot except the ones that were removed in the meantime, modulo hash
> collisions. I am more comfortable with this definition being put as the API
> spec, though I am not sure it is what the blockfile backend does. Simplecache
> definition of iteration allows arbitrary number of concurrent iterations and
> cache modifications. And I would argue that Camille's implementation for that
is
> pretty simple.
> 
> Big question: Do we plan to add more cache enumeration clients?

According to previous version of the comment, the behavior of the blockfile
backend is not defined in this case. This probably means we don't plan to add
more enumeration clients to the blockfile backend.

Thanks. I am not against the change :)

[cbentzel, 2014-09-05 13:40:58]

On 2014/09/05 10:32:42, pasko wrote:
> On 2014/09/05 09:53:51, pasko wrote:
> >
>
https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
> > File net/disk_cache/disk_cache.h (right):
> > 
> >
>
https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
> > net/disk_cache/disk_cache.h:137: // iteration, then the result of this
> function
> > is thereafter undefined.
> > This explicit statement about undefined behavior basically means we cannot
use
> > enumeration for anything except enumerating cache on the start, the way the
> > shader cache is using it. If shader cache is the only usecase for
enumeration,
> > we may only have one memory leak per backend, right?
> > 
> > The original implementation by clamy@ made a snapshot of all entries in
> > StartEnumeration, and OpenNextEntry would iterate only through those in the
> > snapshot except the ones that were removed in the meantime, modulo hash
> > collisions. I am more comfortable with this definition being put as the API
> > spec, though I am not sure it is what the blockfile backend does.
Simplecache
> > definition of iteration allows arbitrary number of concurrent iterations and
> > cache modifications. And I would argue that Camille's implementation for
that
> is
> > pretty simple.
> > 
> > Big question: Do we plan to add more cache enumeration clients?
> 
> According to previous version of the comment, the behavior of the blockfile
> backend is not defined in this case. This probably means we don't plan to add
> more enumeration clients to the blockfile backend.
> 
> Thanks. I am not against the change :)

My rough take is that it's good to remove the void** idiom, but I'm not sure if
the API is proposed in this CL is the right replacement.

One suggestion:

class CacheEntryIterator {
public:
  virtual int OpenNextEntry(Entry** entry, const CompletionCallback& callback) =
0;
  virtual ~CacheEntryIterator() = 0;
};

class NET_EXPORT Backend {
public:
  scoped_ptr<CacheEntryIterator> CreateIterator() = 0;
};

Callers would work directly with the iterators rather than going through the
backend.

This does a couple of nice things:
  - Removes the need for the downcast of the iterator that is currently
happening in the OpenNextEntry calls on the Backend.
  - Has the potential to lead to doing iteration that starts in the middle of
the cache rather than always at the start.
  - API more consistent with other uses of iterators (STL and Chrome-specific
ones). Not sure if we'd want to have an "end" iterator and a comparison
operation for iterators to make even closer to STL. Perhaps overkill if all
iterations go through every entry though.
  - OpenNextEntry only focuses on advancing, rather than initialization and
advancing.

We'd have to work out what happens when the Iterator object outlasts that of the
Backend - either make it illegal to use after Backend lifetime in the API, or
have the iterators maintain weak pointers to Backend (or keep track of lifetime)
and fail if they are accessed after the backend is removed.

Also need to deal with how to handle mutations to Backends while iterators
exist, but that already needs to be dealt with for the existing void** API, as
well as the proposed API as it exists in this CL.

[rvargas (doing something else), 2014-09-06 00:48:28]

On 2014/09/05 13:40:58, cbentzel wrote:
> On 2014/09/05 10:32:42, pasko wrote:
> > On 2014/09/05 09:53:51, pasko wrote:
> > >
> >
>
https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
> > > File net/disk_cache/disk_cache.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
> > > net/disk_cache/disk_cache.h:137: // iteration, then the result of this
> > function
> > > is thereafter undefined.
> > > This explicit statement about undefined behavior basically means we cannot
> use
> > > enumeration for anything except enumerating cache on the start, the way
the
> > > shader cache is using it. If shader cache is the only usecase for
> enumeration,
> > > we may only have one memory leak per backend, right?
> > > 
> > > The original implementation by clamy@ made a snapshot of all entries in
> > > StartEnumeration, and OpenNextEntry would iterate only through those in
the
> > > snapshot except the ones that were removed in the meantime, modulo hash
> > > collisions. I am more comfortable with this definition being put as the
API
> > > spec, though I am not sure it is what the blockfile backend does.
> Simplecache
> > > definition of iteration allows arbitrary number of concurrent iterations
and
> > > cache modifications. And I would argue that Camille's implementation for
> that
> > is
> > > pretty simple.
> > > 
> > > Big question: Do we plan to add more cache enumeration clients?
> > 
> > According to previous version of the comment, the behavior of the blockfile
> > backend is not defined in this case. This probably means we don't plan to
add
> > more enumeration clients to the blockfile backend.
> > 
> > Thanks. I am not against the change :)
> 
> My rough take is that it's good to remove the void** idiom, but I'm not sure
if
> the API is proposed in this CL is the right replacement.
> 
> One suggestion:
> 
> class CacheEntryIterator {
> public:
>   virtual int OpenNextEntry(Entry** entry, const CompletionCallback& callback)
=
> 0;
>   virtual ~CacheEntryIterator() = 0;
> };
> 
> class NET_EXPORT Backend {
> public:
>   scoped_ptr<CacheEntryIterator> CreateIterator() = 0;
> };
> 
> Callers would work directly with the iterators rather than going through the
> backend.
> 
> This does a couple of nice things:
>   - Removes the need for the downcast of the iterator that is currently
> happening in the OpenNextEntry calls on the Backend.
>   - Has the potential to lead to doing iteration that starts in the middle of
> the cache rather than always at the start.
>   - API more consistent with other uses of iterators (STL and Chrome-specific
> ones). Not sure if we'd want to have an "end" iterator and a comparison
> operation for iterators to make even closer to STL. Perhaps overkill if all
> iterations go through every entry though.
>   - OpenNextEntry only focuses on advancing, rather than initialization and
> advancing.
> 
> We'd have to work out what happens when the Iterator object outlasts that of
the
> Backend - either make it illegal to use after Backend lifetime in the API, or
> have the iterators maintain weak pointers to Backend (or keep track of
lifetime)
> and fail if they are accessed after the backend is removed.
> 
> Also need to deal with how to handle mutations to Backends while iterators
> exist, but that already needs to be dealt with for the existing void** API, as
> well as the proposed API as it exists in this CL.

What is exactly the problem with void* (aka, what are we trying to _solve_
here)?

I see you mention a cast, but if the cast (which generates no code) is wrong
then the user is passing back something other than what it previously
received... and that has exactly the same failure mode as the cast (as the
compiled code is effectively a cast of whatever the caller passes).

Enumerations from a random entry is something that doesn't necessarily makes
sense, as what that could mean is implementation-defined. Case in point: the
block file cache has multiple lists and that's handled with the current
iterator, so that grabbing the next entry correctly remembers what we were doing
an can go from one list to another. Your last point looks like this same thing
again.

So, for background:

I actually did not decided on a whim to use the current enumerator mechanism,
nor it was the first thing that came to my mind. The approach considered at that
time included:
- Something STL-ish
- Something inheritance-ish
- Something handle-ish

In fact, I actually consulted this point with someone who cares way more than me
about formal design patterns (and that's the reason I'm adding cpu@ here). The
other point to consider is that enumeration has never been a first class part of
the disk cache API... it has always been intended as a debug helper way to dump
the contents of the cache (basically about:cache). That is somewhat a reason to
avoid solutions that are too elaborate or worry too much about performance.

The last point that I want to make is that I see little reason to change things
that are not clearly broken; If I'm writing something for the first time the
trouble of picking and implementing any solution is close to be the same. After
code has been designed, implemented, reviewed, tested and deployed for a long
time, the bar to change it for the sake of it is higher.

Gavin's proposed change is basically what I considered as the second option
above. To be honest, it is also perfectly reasonable, and the disadvantage that
I mentioned before regarding the underlying message of the API is not a big
problem. But it is not a big improvement either (in fact I don't think it is an
improvement, but I can see people seeing it that way). It just doesn't cut the
bill to actually do it.

It is certainly unrelated to the bug in the description, right? Let's separate
fixing bugs from random "I wish this work this way instead"

[gavinp, 2014-09-09 18:43:53]

Patchset #15 (id:270001) has been deleted

[gavinp, 2014-09-09 18:44:00]

Patchset #15 (id:290001) has been deleted

[gavinp, 2014-09-09 18:44:51]

Patchset #15 (id:310001) has been deleted

[gavinp, 2014-09-11 23:44:22]

On 2014/09/05 13:40:58, cbentzel wrote:
> On 2014/09/05 10:32:42, pasko wrote:
> > On 2014/09/05 09:53:51, pasko wrote:
> > >
> >
>
https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
> > > File net/disk_cache/disk_cache.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/542733002/diff/250001/net/disk_cache/disk_cac...
> > > net/disk_cache/disk_cache.h:137: // iteration, then the result of this
> > function
> > > is thereafter undefined.
> > > This explicit statement about undefined behavior basically means we cannot
> use
> > > enumeration for anything except enumerating cache on the start, the way
the
> > > shader cache is using it. If shader cache is the only usecase for
> enumeration,
> > > we may only have one memory leak per backend, right?
> > > 
> > > The original implementation by clamy@ made a snapshot of all entries in
> > > StartEnumeration, and OpenNextEntry would iterate only through those in
the
> > > snapshot except the ones that were removed in the meantime, modulo hash
> > > collisions. I am more comfortable with this definition being put as the
API
> > > spec, though I am not sure it is what the blockfile backend does.
> Simplecache
> > > definition of iteration allows arbitrary number of concurrent iterations
and
> > > cache modifications. And I would argue that Camille's implementation for
> that
> > is
> > > pretty simple.
> > > 
> > > Big question: Do we plan to add more cache enumeration clients?
> > 
> > According to previous version of the comment, the behavior of the blockfile
> > backend is not defined in this case. This probably means we don't plan to
add
> > more enumeration clients to the blockfile backend.
> > 
> > Thanks. I am not against the change :)
> 
> My rough take is that it's good to remove the void** idiom, but I'm not sure
if
> the API is proposed in this CL is the right replacement.
> 
> One suggestion:
> 
> class CacheEntryIterator {
> public:
>   virtual int OpenNextEntry(Entry** entry, const CompletionCallback& callback)
=
> 0;
>   virtual ~CacheEntryIterator() = 0;
> };
> 
> class NET_EXPORT Backend {
> public:
>   scoped_ptr<CacheEntryIterator> CreateIterator() = 0;
> };

Thank you for this suggestion. I have updated the CL along these lines, please
take a look at the latest upload.

I think there's a lot of problems with the existing API; and we should keep them
in mind as we make coding changes in affected areas. In particular:
    1. The use of Entry** to return values is very dangerous in exactly the same
way as the void**. We should use a type that enforces ownership.
    2. Clients of iteration cannot distinguish completion of iteration from it
ending in error.

> 
> Callers would work directly with the iterators rather than going through the
> backend.
> 
> This does a couple of nice things:
>   - Removes the need for the downcast of the iterator that is currently
> happening in the OpenNextEntry calls on the Backend.
>   - Has the potential to lead to doing iteration that starts in the middle of
> the cache rather than always at the start.
>   - API more consistent with other uses of iterators (STL and Chrome-specific
> ones). Not sure if we'd want to have an "end" iterator and a comparison
> operation for iterators to make even closer to STL. Perhaps overkill if all
> iterations go through every entry though.

I think going STL style here would be overkill. The asynchronous style means
we'll never have any kind of increment operators, and so I'd argue that too much
STL resemblance would bring people to pain. The end() method in STL iterators is
for loops, but all of our loops are doomed to asynchronousity!

>   - OpenNextEntry only focuses on advancing, rather than initialization and
> advancing.
> 
> We'd have to work out what happens when the Iterator object outlasts that of
the
> Backend - either make it illegal to use after Backend lifetime in the API, or
> have the iterators maintain weak pointers to Backend (or keep track of
lifetime)
> and fail if they are accessed after the backend is removed.

I opted to fail to discuss this in the description of the API, and for each
implementation to maintain weak pointers. The current API describes many
undefined behaviours in words and depends on callers to get it right (see many
cases of existing tests which violate the documented rules, even!). By not
documenting a behaviour and using weak pointers to fail on subsequent
operations, we're in a reasonably well defined space without a need to overload
users with "words."

> 
> Also need to deal with how to handle mutations to Backends while iterators
> exist, but that already needs to be dealt with for the existing void** API, as
> well as the proposed API as it exists in this CL.

[gavinp, 2014-09-11 23:50:26]

On 2014/09/05 13:40:58, cbentzel wrote:
>
> Also need to deal with how to handle mutations to Backends while iterators
> exist, but that already needs to be dealt with for the existing void** API, as
> well as the proposed API as it exists in this CL.
>

Many of the backends have different behaviours and it's worth documenting that.
I added in the latest upload referencing the different behaviour in simple
cache.

The in memory backend is safe as well; just because it's totally synchronous. I
did not elect to document that here (although it is tested behaviour), but I
think perhaps that's worth adding in a follow up CL.

Earlier this year, I had a CL to parameterize the tests on disk cache. Among the
changes required was a "quirks" object describing how each backend differed from
the others. It was https://codereview.chromium.org/127083002/ , and I abandoned
it as impractical to land because of review problems. It added an object
describing the many differences our backends have in
https://codereview.chromium.org/127083002/diff/2180001/net/disk_cache/disk_ca...
. All those crazy distinctions arose from attempting to distinguish the tests
and parameterize them.

[gavinp, 2014-09-11 23:57:04]

Thank you everyone for your reviews.

This upload removes the downcasts per cbentzel's suggestions.

I am happy to be rid of the void** and void*. It happens that a close reading of
the C++98 specification reveals that the code used in the earlier referenced fix
was valid with defined behaviour. Also valid with defined behaviour is the
iteration code in the blockfile backend and the memory backend.

It turns out, that "defined" behaviour is mighty fragile; a cast to a void* is
defined as casting the most derived type of the referenced pointer in to the
void*; so in the blockfile backend implementation and the old memory backend
implementation, we are lucky there isn't multiple inheritance, since our lack of
dynamic_cast<> in chrome would mean we could not recover them in a well defined
way. Oh boy that's fragile!!!!

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.cc (right):

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.cc:9: Backend::EnumerationState::~EnumerationState()
{}
On 2014/09/05 02:01:29, rvargas wrote:
> I think I'm missing something here. Why this file?

See http://www.chromium.org/developers/coding-style#TOC-Inline-functions and
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
.

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:70: typedef scoped_ptr<EnumerationState> Iterator;
On 2014/09/05 02:01:29, rvargas wrote:
> I don't think we need (or want) an extra typedef to hide a scoped_ptr

The latest upload removes the typedef.

https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:138: virtual int OpenNextEntry(Iterator* iter,
Entry** next_entry,
The existing pattern of using a void** is neither standard or extremely common
in chrome APIs. This CL removes the last instance of us advertising such an API
in net/.

A caller cannot "do more with the iterator than it should."

You say you would "rather let the cache cleanup all enumerations at
destruction." Your suggestion contradicts the current documentation of the
EndEnumeration() function "... the enumeration must be ended ...".

Happily, this change reduces the need for documentation. The APIs use of
scoped_ptr<> defines ownership and forces callers to use it safely. As a result,
a lot of confusing comment text describing a complex ownership rule is removed.
Even better, many existing tests that violated those rules now only test
documented behaviour.

On 2014/09/05 02:01:29, rvargas wrote:
> So far I'm unconvinced about the API change.
> 
> The current pattern is pretty standard: the cache returns an opaque handle to
> the consumer and the consumer uses it for further enumerations until it
doesn't
> want to enumerate anymore. The consumer cannot do any operation directly with
> that handle.
> 
> Sure, it is a C-style pattern, but it is extremely common and makes sure the
> cache is in control of the iteration at all times.
> 
> What you propose opens up the caller to know, and do more with the iterator
than
> it should. It also poses restrictions on the implementation that I don't see
> fully justified: deletion of the object is tied to the consumer[1] and cleanup
> when the cache goes away may be more complicated (as now it has to make sure
> that anything related to an iterator can be left behind because it gave up
> ownership to the consumer, who really should not care about what is tied to
the
> iterator).
> 
> [1] Arguably, calling a function to say that the iterator is not needed is
> basically the same as directly deleting the iterator, but there is a
conceptual
> difference between "I delete this because I own it, and I know what to do with
> it" and "I don't know what this is and I don't need it anymore so I'll tell
the
> owner about it". It limits what the cache can do with it, or may force an
extra
> layer of isolation just for the sake of it. I still think that the second
> message is the one that the API should deliver.
> 
> Another way to put it is that so far I much rather let the cache cleanup all
> enumerations at destruction (and leave the caller with handles that are
invalid,
> but there's no cache to call), than leave a bunch of enumerators lying around
> after the cache is gone.

https://codereview.chromium.org/542733002/diff/450001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (left):

https://codereview.chromium.org/542733002/diff/450001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:1329: ASSERT_EQ(net::OK, OpenNextEntry(&iter,
&entry2));
From the original version of disk_cache.h: "Initialize |iter| to NULL before
calling this method the first time."

Whoops! This code is relying on undocumented behaviour that the blockfile
backend implementation of EndEnumeration() zeroes the iterator.

https://codereview.chromium.org/542733002/diff/450001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:2033: }
From the original version of disk_cache.h: "Whenever OpenNextEntry() returns
true(sic), ..., the enumeration must be ended by calling ... OpenNextEntry()"

(many other existing tests have this same pattern of relying on undefined
behaviour, I will not comment them individually)

https://codereview.chromium.org/542733002/diff/450001/net/tools/dump_cache/si...
File net/tools/dump_cache/simple_cache_dumper.cc (left):

https://codereview.chromium.org/542733002/diff/450001/net/tools/dump_cache/si...
net/tools/dump_cache/simple_cache_dumper.cc:159: cache_->EndEnumeration(&iter_);
From the original version of disk_cache.h: "Whenever OpenNextEntry() returns
true(sic), but ... must be ended by calling this method..."

This existing call has undefined effect.

[gavinp, 2014-09-12 00:10:54]

gavinp@chromium.org changed reviewers:
+ piman@chromium.org

[gavinp, 2014-09-12 00:11:38]

piman, jsbell: OWNERs reviews please of shader and service worker respectively?

[jsbell, 2014-09-12 00:15:59]

jsbell@chromium.org changed reviewers:
+ michaeln@chromium.org

[jsbell, 2014-09-12 00:16:00]

I'm not in service_worker/OWNERS on the chromium side, so roping michaeln@ in.

[rvargas (doing something else), 2014-09-12 00:24:04]

Now that you are asking for owners, this is still NOT LGTM until we at least
create a separate bug about the change of the API and not conflate it with
fixing a leak on the single cache.

[michaeln, 2014-09-12 00:34:04]

sw lgtm

[piman, 2014-09-12 02:33:01]

gpu LGTM.

[gavinp, 2014-09-12 03:34:21]

On 2014/09/12 00:24:04, rvargas wrote:
> Now that you are asking for owners, this is still NOT LGTM until we at least
> create a separate bug about the change of the API and not conflate it with
> fixing a leak on the single cache.

I'm sorry to hear about your procedural and other issues, Ricardo. I have set up
a meeting for us at the beginning of business hours on Friday in both of our
time zones, I look forward to speaking to you then.

[pasko, 2014-09-12 08:10:26]

LGTM with my proposed changes in comments, keeping a scoped_ptr to the Iterator
is less error prone and more cpp-ish.

Chiming in only to show my voice of support, I did not think it was useful
before. It is sad to see so much disagreement in this CL.

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (left):

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:136: // NOTE: This method does not modify the
last_used field of the entry, and
this comment clarifies some undesired behavior of the blockfile cache. It's not
doing so very clearly, but it's still better than nothing. Please move it to
net/disk_cache/blockfile/backend_impl.h

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:72: // OpenNextEntry returns |net::OK| and provides
|next_entry| if there is an
Please briefly mention net::ERR_IO_PENDING behavior.

Please also document the lifetime constraints of the Iterator. Is it  required
that the iterator is functional after the backend is destructed?

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:491: void OpenNextEntryImpl(Entry**
next_entry,
This has a lot in common with SimpleBackendImpl::GetNextEntryInIterator, but
it's hard to see what changed. In cases like this I would prefer reading it in
two changes: the first preserves the order of functions as it was, the second
(optional) brings the order in accordance with the order as it is in the header
by only moving the code without change.

FYI, not necessary now.

[jkarlin, 2014-09-12 11:46:03]

sw lgtm as well. Thanks for writing this.

[gavinp, 2014-09-12 14:15:32]

pasko, ty for your review. I've done the changes you requested. I also am sad at
the controversy that this relatively small change has generated.

rvargas, please see crbug.com/413644 "Remove void** from disk_cache API". I've
added a reference to that issue in this CL.

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (left):

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:136: // NOTE: This method does not modify the
last_used field of the entry, and
On 2014/09/12 08:10:26, pasko wrote:
> this comment clarifies some undesired behavior of the blockfile cache. It's
not
> doing so very clearly, but it's still better than nothing. Please move it to
> net/disk_cache/blockfile/backend_impl.h

Done. The revised comment in disk_cache.h summarizes this section as "If any
entry of the cache is modified during iteration, the result of this function is
thereafter undefined", which felt like about as much information as an API
consumer needs.

I've cut and pasted the longer original version in to disk_cache.h as you
suggested, editing only for the initial pronoun "This method" and reformatting
to 80 columns after that change.

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:72: // OpenNextEntry returns |net::OK| and provides
|next_entry| if there is an
On 2014/09/12 08:10:26, pasko wrote:
> Please briefly mention net::ERR_IO_PENDING behavior.
> 
> Please also document the lifetime constraints of the Iterator. Is it  required
> that the iterator is functional after the backend is destructed?

Done.

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/542733002/diff/510001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:491: void OpenNextEntryImpl(Entry**
next_entry,
On 2014/09/12 08:10:26, pasko wrote:
> This has a lot in common with SimpleBackendImpl::GetNextEntryInIterator, but
> it's hard to see what changed. In cases like this I would prefer reading it in
> two changes: the first preserves the order of functions as it was, the second
> (optional) brings the order in accordance with the order as it is in the
header
> by only moving the code without change.
> 
> FYI, not necessary now.

You're right, that would have been a good way to present the change. My
apologies for making your review harder than it should have been.

As it happens, the ordering situation is relatively free here; the function
could have been defined in the same location the old one was: There's no rule
about where class definitions should appear in .cc, nor a rule about where the
definitions of methods for a class defined in .cc file. This location was chosen
so that all the relevant functions would be defined right above
SimpleBackendImpl::CreateIterator().

Perhaps more controversially, I chose inline function bodies because I felt the
functions were short enough that it was an easier read this way. Because the
class is defined in a .cc file, there's no concerns about linker explosion or
object file explosion from generated code.

[gavinp, 2014-09-12 15:54:46]

On 2014/09/12 00:24:04, rvargas wrote:
> Now that you are asking for owners, this is still NOT LGTM until we at least
> create a separate bug about the change of the API and not conflate it with
> fixing a leak on the single cache.

Ricardo,

I've created the issue 413644 , and updated this CL. Unfortunately meeting today
didn't pan out, but I've set up a meeting at 9am PDT on Monday to continue the
conversation.

As of writing, jkarlin, jsbell, michaeln, and the whole serviceworker team are
sad that our Cache.Keys() implementation has a memory leak on Cache shutdown,
which this CL fixes. Given the wide support for this change from other engineers
who have worked in the cache, can you consider removing your blocker and we can
continue the conversation on Monday?

[clamy, 2014-09-12 17:41:05]

I conducted the following investigation to see if indeed, void** APIs were
common in our codebase. Grepping for "void**" in header files returns 148
headers in the chromium repository. 63 are in third_party (53 tcmalloc, 6
protobuf, 4 others). Then there are 29 platform specific headers (21 win
specific, 4 android specific, 3 mac specific, 1 linux specific). There are 35 
additional headers for components such as Mojo (8), NPAPI plugins (4), PPAPI
plugins (9), NACL sdk (5). Of the remaining 30, 9 are GPU related, leaving the
following 21 (7 of them are from disk cache):
base/debug/trace_event_memory.h
base/process/memory.h
chrome/renderer/printing/mock_printer.h
google_apis/gcm/base/socket_stream.h
media/audio/alsa/alsa_wrapper.h
media/audio/alsa/audio_manager_alsa.h
net/disk_cache/blockfile/backend_impl.h
net/disk_cache/blockfile/backend_impl_v3.h
net/disk_cache/blockfile/in_flight_backend_io.h
net/disk_cache/disk_cache.h
net/disk_cache/disk_cache_test_base.h
net/disk_cache/memory/mem_backend_impl.h
net/disk_cache/simple/simple_backend_impl.h
net/http/http_auth_handler_ntlm.h
net/http/http_response_headers.h
net/http/mock_http_cache.h
pdf/pdfium/pdfium_engine.h
remoting/base/compound_buffer.h
ui/ozone/platform/dri/dri_util.h
ui/ozone/platform/dri/dri_wrapper.h
ui/ozone/platform/dri/test/mock_dri_wrapper.h

Please keep in mind that this only represent headers that include the word
void**. Looking for the specific pattern of having a void** based iterator is
more complex. Greping for "void** it" only return one header out of the
disk_cache (net/http/http_response_headers.h). However, one cannot exclude that
this pattern may be present in other places, with a different name for the
iterator.

In any case it would seem that the current iterator pattern is anything but
standard when it comes to writing cross-platform chromium code that does not
directly interact with the OS. So apart from the drawbacks already mentioned, I
would like to argue that it is inconsistant with the rest of our codebase.

As for simply fixing the leak as opposed to changing the interface, Gavin wrote
a first patch that was doing just that. However, due to the use of void** in the
interface, the code ended up as C written in C++, which was downright obtuse
(and used a certain number of reinterpret_cast, which our coding style guide
specifically advise against). This is why, it seemed better to change the
interface, in order to have more readable an understandable code.

[jsbell, 2014-09-12 18:11:18]

On 2014/09/12 00:24:04, rvargas wrote:
> Now that you are asking for owners, this is still NOT LGTM until we at least
> create a separate bug about the change of the API and not conflate it with
> fixing a leak on the single cache.

+1 to separating out the fix from the API change.

gavinp: Can you land https://codereview.chromium.org/533293002 - with
reinterpret_cast<>s and all - so we can we can consider this API change on its
own merits?

As far as the API change goes: rvargas outlines the options and decisions in
https://chromiumcodereview.appspot.com/542733002/#msg13 very clearly. And as he
outlines (second-to-last paragraph) it's close to being a neutral change. But...
since we've recently been bitten by a misuse of the current API, and have a
prospective rework (i.e. this patch) in hand, taking advantage of language
features to reduce the risk of further bugs by imperfect engineers tips me over
to supporting the change. But this is all in the abstract since I haven't coded
against the API myself!

[cbentzel, 2014-09-12 19:10:53]

On 2014/09/12 18:11:18, jsbell wrote:
> On 2014/09/12 00:24:04, rvargas wrote:
> > Now that you are asking for owners, this is still NOT LGTM until we at least
> > create a separate bug about the change of the API and not conflate it with
> > fixing a leak on the single cache.
> 
> +1 to separating out the fix from the API change.
> 
> gavinp: Can you land https://codereview.chromium.org/533293002 - with
> reinterpret_cast<>s and all - so we can we can consider this API change on its
> own merits?
> 
> As far as the API change goes: rvargas outlines the options and decisions in
> https://chromiumcodereview.appspot.com/542733002/#msg13 very clearly. And as
he
> outlines (second-to-last paragraph) it's close to being a neutral change.
But...
> since we've recently been bitten by a misuse of the current API, and have a
> prospective rework (i.e. this patch) in hand, taking advantage of language
> features to reduce the risk of further bugs by imperfect engineers tips me
over
> to supporting the change. But this is all in the abstract since I haven't
coded
> against the API myself!

+1 to everything said here

[gavinp, 2014-09-15 17:50:25]

Patchset #28 (id:590001) has been deleted

[gavinp, 2014-09-15 18:58:33]

On 2014/09/12 19:10:53, cbentzel wrote:
> On 2014/09/12 18:11:18, jsbell wrote:
> > On 2014/09/12 00:24:04, rvargas wrote:
> > > Now that you are asking for owners, this is still NOT LGTM until we at
least
> > > create a separate bug about the change of the API and not conflate it with
> > > fixing a leak on the single cache.
> > 
> > +1 to separating out the fix from the API change.
> > 
> > gavinp: Can you land https://codereview.chromium.org/533293002 - with
> > reinterpret_cast<>s and all - so we can we can consider this API change on
its
> > own merits?
> > 
> > As far as the API change goes: rvargas outlines the options and decisions in
> > https://chromiumcodereview.appspot.com/542733002/#msg13 very clearly. And as
> he
> > outlines (second-to-last paragraph) it's close to being a neutral change.
> But...
> > since we've recently been bitten by a misuse of the current API, and have a
> > prospective rework (i.e. this patch) in hand, taking advantage of language
> > features to reduce the risk of further bugs by imperfect engineers tips me
> over
> > to supporting the change. But this is all in the abstract since I haven't
> coded
> > against the API myself!
> 
> +1 to everything said here

The upstream CL https://codereview.chromium.org/533293002 has now landed. Issue
410276 has now been removed from this CL. 

Given the impracticality of moving past a "naught ell gee tee em" in a
reasonable time frame, it's the best decision to not block progress for Service
Worker and jkarlin, who were being hurt by this problem.

It is a sad decision. Perhaps it's close to a neutral change, but it's an
internal API and we should be able to treat it as quite malleable. None of us
hesitate to upload bug fixes together with IPC changes to improve architecture
while we work, and I have never shied away from changing public Webkit
interfaces when that's the clean way to do some work. I have difficulty
crediting the arguments that void** sends a useful signal to users of this API,
given that actual users have cut themselves on this and asked for this change.

If stepping back and looking at it on its merits helps us, then great. I look
forward to that progress.

[cbentzel, 2014-09-18 00:20:53]

It seems like there's rough consensus that most folks (at least on this CL)
prefer the new API.

Unless there are technical arguments against it I recommend we proceed with it.

[cbentzel, 2014-09-18 00:27:56]

On 2014/09/18 00:20:53, cbentzel wrote:
> It seems like there's rough consensus that most folks (at least on this CL)
> prefer the new API.
> 
> Unless there are technical arguments against it I recommend we proceed with
it.

rvargas: The issues raised in your negative vote have been addressed (separated
out bug fix from API as well as entered a bug for this). Since those have been
resolved, I'd like to see this land barring technical objections such as whether
it introduces new bugs, etc.

[cbentzel, 2014-09-18 00:28:01]

On 2014/09/18 00:20:53, cbentzel wrote:
> It seems like there's rough consensus that most folks (at least on this CL)
> prefer the new API.
> 
> Unless there are technical arguments against it I recommend we proceed with
it.

rvargas: The issues raised in your negative vote have been addressed (separated
out bug fix from API as well as entered a bug for this). Since those have been
resolved, I'd like to see this land barring technical objections such as whether
it introduces new bugs, etc.

[cbentzel, 2014-09-18 00:51:48]

cbentzel@chromium.org changed reviewers:
+ cbentzel@chromium.org

[rvargas (doing something else), 2014-09-18 00:57:37]

On 2014/09/18 00:28:01, cbentzel wrote:
> On 2014/09/18 00:20:53, cbentzel wrote:
> > It seems like there's rough consensus that most folks (at least on this CL)
> > prefer the new API.
> > 
> > Unless there are technical arguments against it I recommend we proceed with
> it.
> 
> rvargas: The issues raised in your negative vote have been addressed
(separated
> out bug fix from API as well as entered a bug for this). Since those have been
> resolved, I'd like to see this land barring technical objections such as
whether
> it introduces new bugs, etc.

Hmm.. I was really hoping to have the discussion on the bug that was filed; that
seems like the right place to me[*], if we are deciding what to implement. I
actually wrote something there a few days ago.

I'll go over this code now.

[*] That was the whole point of filing a bug.

[cbentzel, 2014-09-18 01:19:28]

On 2014/09/18 00:57:37, rvargas wrote:
> On 2014/09/18 00:28:01, cbentzel wrote:
> > On 2014/09/18 00:20:53, cbentzel wrote:
> > > It seems like there's rough consensus that most folks (at least on this
CL)
> > > prefer the new API.
> > > 
> > > Unless there are technical arguments against it I recommend we proceed
with
> > it.
> > 
> > rvargas: The issues raised in your negative vote have been addressed
> (separated
> > out bug fix from API as well as entered a bug for this). Since those have
been
> > resolved, I'd like to see this land barring technical objections such as
> whether
> > it introduces new bugs, etc.
> 
> Hmm.. I was really hoping to have the discussion on the bug that was filed;
that
> seems like the right place to me[*], if we are deciding what to implement. I
> actually wrote something there a few days ago.
> 
> I'll go over this code now.
> 
> [*] That was the whole point of filing a bug.

Thanks for taking a look at the code.

[cbentzel, 2014-09-18 01:19:32]

On 2014/09/18 00:57:37, rvargas wrote:
> On 2014/09/18 00:28:01, cbentzel wrote:
> > On 2014/09/18 00:20:53, cbentzel wrote:
> > > It seems like there's rough consensus that most folks (at least on this
CL)
> > > prefer the new API.
> > > 
> > > Unless there are technical arguments against it I recommend we proceed
with
> > it.
> > 
> > rvargas: The issues raised in your negative vote have been addressed
> (separated
> > out bug fix from API as well as entered a bug for this). Since those have
been
> > resolved, I'd like to see this land barring technical objections such as
> whether
> > it introduces new bugs, etc.
> 
> Hmm.. I was really hoping to have the discussion on the bug that was filed;
that
> seems like the right place to me[*], if we are deciding what to implement. I
> actually wrote something there a few days ago.
> 
> I'll go over this code now.
> 
> [*] That was the whole point of filing a bug.

Thanks for taking a look at the code.

[rvargas (doing something else), 2014-09-18 02:32:12]

To clarify a little bit more my previous comment, I usually don't read other's
reviewers comments on CLs that I'm reviewing, unless they are very clearly
directed at me (and I happen to notice that mention). I know for a fact that I'm
not the only one to do so.

So, in a way, a code review is more of a n:1 venue rather than a n:n, and it
assumes that the decision to implement was already made and the only thing to go
over are the details of the particular implementation chosen.

As for patch set 28, I don't have any fundamental objection to it. It adds
another indirection (WeakPtr) that could have been avoided, but as I said from
the start this is not a path particularly sensitive to... basically anything.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:84: const scoped_ptr<TestIterator>& iter,
This should not be a const ref as conceptually this method modifies the
argument. (the argument being the iterator, not the scoped_pointer)

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl.cc:1255: class BackendIterator FINAL :
public Backend::Iterator {
declare this outside the method

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl.cc:1255: class BackendIterator FINAL :
public Backend::Iterator {
Is there any real value to annotate this class with FINAL?

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
File net/disk_cache/blockfile/backend_impl_v3.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl_v3.cc:1512: class NotImplementedIterator
FINAL : public Iterator {
ditto. (out of the method and no FINAL)

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:94: virtual ~Backend();
I still think we should not have a cc file for this.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache_test_base.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_test_base.cc:57: : iterator_(iterator.Pass()) {}
nit: not a single line anymore

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache_test_base.h (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_test_base.h:61: class TestIterator FINAL {
global nit: Using FINAL looks like extra noise on the places I've seen in this
review. There's nothing fundamental about preventing these classes to be
extended later.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_backend_impl.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:32: : max_size_(0), current_size_(0),
net_log_(net_log), weak_factory_(this) {}
not a single line anymore ({})

https://codereview.chromium.org/542733002/diff/610001/net/http/mock_http_cach...
File net/http/mock_http_cache.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/http/mock_http_cach...
net/http/mock_http_cache.cc:461: class NotImplementedIterator FINAL : public
Iterator {
declare outside the method

[pasko, 2014-09-18 07:44:29]

On 2014/09/18 02:32:12, rvargas wrote:
> To clarify a little bit more my previous comment, I usually don't read other's
> reviewers comments on CLs that I'm reviewing, unless they are very clearly
> directed at me (and I happen to notice that mention). I know for a fact that
I'm
> not the only one to do so.
> 
> So, in a way, a code review is more of a n:1 venue rather than a n:n, and it
> assumes that the decision to implement was already made and the only thing to
go
> over are the details of the particular implementation chosen.

I am surprised the explicit practice of reviewing and ignoring other reviewers'
comments exists. This is the first time I encountered it, and the behavior seems
counter-intuitive to me as the code author, and potentially negatively impacts
my ability to commit code. Can you please list other committers that you know
are explicitly ignoring other reviewers' comments so I can adjust to their
behavior?

[darin (slow to review), 2014-09-18 07:58:52]

On 2014/09/18 07:44:29, pasko wrote:
> On 2014/09/18 02:32:12, rvargas wrote:
> > To clarify a little bit more my previous comment, I usually don't read
other's
> > reviewers comments on CLs that I'm reviewing, unless they are very clearly
> > directed at me (and I happen to notice that mention). I know for a fact that
> I'm
> > not the only one to do so.
> > 
> > So, in a way, a code review is more of a n:1 venue rather than a n:n, and it
> > assumes that the decision to implement was already made and the only thing
to
> go
> > over are the details of the particular implementation chosen.
> 
> I am surprised the explicit practice of reviewing and ignoring other
reviewers'
> comments exists. This is the first time I encountered it, and the behavior
seems
> counter-intuitive to me as the code author, and potentially negatively impacts
> my ability to commit code. Can you please list other committers that you know
> are explicitly ignoring other reviewers' comments so I can adjust to their
> behavior?

I think I operate pretty similarly. When I get a code review, I start with
the code changes. If I have concerns, I indicate them, and then I submit
those concerns. If I'm content with a CL, then I will more likely look into
what others are saying too. This is all of course not absolute. Sometimes
I do the opposite :)

[gavinp, 2014-09-18 18:13:05]

TY for your review rvargas. I'll reply to comments on the code here in the CL,
and I've also separately written a reply on the issue at crbug.com/413644 .

Ricardo, you say that for patch set 28 you don't have "any fundamental objection
to it". That's good to hear! Earlier you wrote a "Naught Ell Ge Tee Em". In the
interests of being very conservative, I'm not going to understand your statement
about not having a fundamental objection as waiving your Naught El Gee Tee Em;
instead I'd like to ask you to tell us explicitly when you've waived it. Thank
you.

If we can advance this CL to the point that you can El Gee Tee Em it, even
better!

On 2014/09/18 02:32:12, rvargas wrote:
> To clarify a little bit more my previous comment, I usually don't read other's
> reviewers comments on CLs that I'm reviewing, unless they are very clearly
> directed at me (and I happen to notice that mention). I know for a fact that
I'm
> not the only one to do so.

In this particular case, that's a shame. I know I asked you to consider the
other reviews in https://codereview.chromium.org/542733002/#msg31 , but if that
escaped your attention, then it escaped your attention. We're all seeing the
same things now and that's what's important.

> 
> So, in a way, a code review is more of a n:1 venue rather than a n:n, and it
> assumes that the decision to implement was already made and the only thing to
go
> over are the details of the particular implementation chosen.

This was my decision; I apologise for making it wrong for you here. My opinion
was, and really still is that this CL and this feature change are small enough
that combining the justification with the implementation is more useful than
creating a procedural separation; in some sense an API change of this type is
self proving, the cleaner call sites and the good reviews by engineers using
this interface demonstrate its utility in a way that a discussion on an abstract
bug might not.

Now that you've asked me to, I'm trying to keep code discussion and relevant
replies here, but address the architectural issues over in the bug at
crbug.com/413644 . Of course my answers there do and will reference this code,
since it's a relevant exhibit to that discussion.

> 
> As for patch set 28, I don't have any fundamental objection to it. It adds
> another indirection (WeakPtr) that could have been avoided, but as I said from
> the start this is not a path particularly sensitive to... basically anything.
> 

I'd like to hear more about this. This CL both adds and removes indirection, and
most significantly it removes a lot of downcasts, or in the case of the simple
backend, it removes lots of scary reinterpret_cast instances. I think on balance
the benefit of removing the downcasts and reinterpret_casts outweighs the loss
of adding a WeakPtrFactory to one object, and creating a single WeakPtr (=
2*sizeof(void*) instance for each enumeration... What do you think about the
benefit of losing the downcasts?

Returning specifically to WeakPtr, there's a few places where we use them in
this CL, and I'm not sure which one you're speaking to. I'll discuss three of
them, and perhaps you'll have others you want to mention?

One place where a WeakPtr is being unambiguously introduced is MemBackendImpl,
which now has a weak_factory_ member. It's used in the iterator, see
https://codereview.chromium.org/542733002/diff/630001/net/disk_cache/memory/m...
.

In the blockfile backend, there isn't a new indirection per se, but we are going
through the existing WeakPtr interface to the InFlightBackendIO object, see
https://codereview.chromium.org/542733002/diff/630001/net/disk_cache/blockfil...
. This code was already indirect through an InFlightBackendIo*, but it was not
possible to be null before. That's new here, so we cons up a WeakPtr to cope
with it.

The situation in the simple cache is similar in that the type already supported
WeakPtr; except it's a SimpleBackendImpl*, which we were already going through
(from the Backend* vtable); it is new that we have the null check on it though.
https://codereview.chromium.org/542733002/diff/630001/net/disk_cache/simple/s...
. The type already had a WeakPtrFactory though.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:84: const scoped_ptr<TestIterator>& iter,
On 2014/09/18 02:32:11, rvargas wrote:
> This should not be a const ref as conceptually this method modifies the
> argument. (the argument being the iterator, not the scoped_pointer)

Hmm, but that's not what const means though, is it? You're thinking perhaps of a
scoped_ptr<const TestIterator>? The const in this case is promising that
EnumerateAndMatchKeys will not delete iter or otherwise mutate what iter points
to, not that it won't modify the pointed at value. It's akin to:

  TestIterator* const& ref_to_const_ptr_to_mutable_TestIterator;

and not as you might be suggesting above:

  const TestIterator*& ref_to_ptr_to_const_TestIterator;
  TestIterator const*& ditto;

The choices for arguments here were a few. Originally I wrote this like:

  bool EnumerateAndMatchKeys(int max_to_open, TestIterator* iter, ...);

But the frustration around that was that each call site had to call .get(),
making them a bit ugly. So by swallowing a bit of the confusing in this
signature, every call site was cleaner. My reading of
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...
suggests that since this argument is in fact const, we should prefer passing it
as a reference.

Do you think we should change the argument type? One possibility that might be
clearer is "scoped_ptr<TestIterator> const&" which means the same thing. Another
is "TestIterator*", and I'm open to other suggestions?

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl.cc:1255: class BackendIterator FINAL :
public Backend::Iterator {
On 2014/09/18 02:32:11, rvargas wrote:
> declare this outside the method

Done.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl.cc:1255: class BackendIterator FINAL :
public Backend::Iterator {
On 2014/09/18 02:32:12, rvargas wrote:
> Is there any real value to annotate this class with FINAL?

No. Removed.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
File net/disk_cache/blockfile/backend_impl_v3.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl_v3.cc:1512: class NotImplementedIterator
FINAL : public Iterator {
On 2014/09/18 02:32:12, rvargas wrote:
> ditto. (out of the method and no FINAL)

Done.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:94: virtual ~Backend();
On 2014/09/18 02:32:12, rvargas wrote:
> I still think we should not have a cc file for this.

You're right, I reviewed the rules in
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
, and this is a documented exception. Of course it's safe (though it generates
some linker churn), but previously I didn't know it was a documented exception.

When you first raised this, at 
https://codereview.chromium.org/542733002/diff/210001/net/disk_cache/disk_cac...
 I answered with reference to the rule I thought I was following; I was wrong,
but it would have been really helpful to point out what I was missing, rather
than reiterating your (correct) interpretation of the rules.

We got here anyway, so that's good in the end.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache_test_base.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_test_base.cc:57: : iterator_(iterator.Pass()) {}
On 2014/09/18 02:32:12, rvargas wrote:
> nit: not a single line anymore

Done.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache_test_base.h (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_test_base.h:61: class TestIterator FINAL {
On 2014/09/18 02:32:12, rvargas wrote:
> global nit: Using FINAL looks like extra noise on the places I've seen in this
> review. There's nothing fundamental about preventing these classes to be
> extended later.

Done.

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_backend_impl.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:32: : max_size_(0), current_size_(0),
net_log_(net_log), weak_factory_(this) {}
On 2014/09/18 02:32:12, rvargas wrote:
> not a single line anymore ({})

Done.

https://codereview.chromium.org/542733002/diff/610001/net/http/mock_http_cach...
File net/http/mock_http_cache.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/http/mock_http_cach...
net/http/mock_http_cache.cc:461: class NotImplementedIterator FINAL : public
Iterator {
On 2014/09/18 02:32:12, rvargas wrote:
> declare outside the method

Done.

[jkarlin, 2014-09-18 18:41:34]

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:84: const scoped_ptr<TestIterator>& iter,
On 2014/09/18 18:13:04, gavinp wrote:
> On 2014/09/18 02:32:11, rvargas wrote:
> > This should not be a const ref as conceptually this method modifies the
> > argument. (the argument being the iterator, not the scoped_pointer)
> 
> Hmm, but that's not what const means though, is it? You're thinking perhaps of
a
> scoped_ptr<const TestIterator>? The const in this case is promising that
> EnumerateAndMatchKeys will not delete iter or otherwise mutate what iter
points
> to, not that it won't modify the pointed at value. It's akin to:
> 
>   TestIterator* const& ref_to_const_ptr_to_mutable_TestIterator;
> 
> and not as you might be suggesting above:
> 
>   const TestIterator*& ref_to_ptr_to_const_TestIterator;
>   TestIterator const*& ditto;
> 
> The choices for arguments here were a few. Originally I wrote this like:
> 
>   bool EnumerateAndMatchKeys(int max_to_open, TestIterator* iter, ...);
> 
> But the frustration around that was that each call site had to call .get(),
> making them a bit ugly. So by swallowing a bit of the confusing in this
> signature, every call site was cleaner. My reading of
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...
> suggests that since this argument is in fact const, we should prefer passing
it
> as a reference.
> 
> Do you think we should change the argument type? One possibility that might be
> clearer is "scoped_ptr<TestIterator> const&" which means the same thing.
Another
> is "TestIterator*", and I'm open to other suggestions?

The chromium guide says not to pass scoped_ptr by const ref toward the bottom:
http://www.chromium.org/developers/smart-pointer-guidelines

[gavinp, 2014-09-18 19:00:40]

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/542733002/diff/610001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:84: const scoped_ptr<TestIterator>& iter,
On 2014/09/18 18:41:34, jkarlin wrote:
> On 2014/09/18 18:13:04, gavinp wrote:
> > On 2014/09/18 02:32:11, rvargas wrote:
> > > This should not be a const ref as conceptually this method modifies the
> > > argument. (the argument being the iterator, not the scoped_pointer)
> > 
> > Hmm, but that's not what const means though, is it? You're thinking perhaps
of
> a
> > scoped_ptr<const TestIterator>? The const in this case is promising that
> > EnumerateAndMatchKeys will not delete iter or otherwise mutate what iter
> points
> > to, not that it won't modify the pointed at value. It's akin to:
> > 
> >   TestIterator* const& ref_to_const_ptr_to_mutable_TestIterator;
> > 
> > and not as you might be suggesting above:
> > 
> >   const TestIterator*& ref_to_ptr_to_const_TestIterator;
> >   TestIterator const*& ditto;
> > 
> > The choices for arguments here were a few. Originally I wrote this like:
> > 
> >   bool EnumerateAndMatchKeys(int max_to_open, TestIterator* iter, ...);
> > 
> > But the frustration around that was that each call site had to call .get(),
> > making them a bit ugly. So by swallowing a bit of the confusing in this
> > signature, every call site was cleaner. My reading of
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...
> > suggests that since this argument is in fact const, we should prefer passing
> it
> > as a reference.
> > 
> > Do you think we should change the argument type? One possibility that might
be
> > clearer is "scoped_ptr<TestIterator> const&" which means the same thing.
> Another
> > is "TestIterator*", and I'm open to other suggestions?
> 
> The chromium guide says not to pass scoped_ptr by const ref toward the bottom:
> http://www.chromium.org/developers/smart-pointer-guidelines

Good catch. This forces it to TestIterator*.

I wonder why we don't programmatically enforce this (i.e. why is
scoped_ptr<>::get() a const method?), but that's a story for another day...

[rvargas (doing something else), 2014-09-19 01:26:52]

lgtm, thanks.

pasko@:
> I am surprised the explicit practice of reviewing and ignoring other
reviewers'
> comments exists.

It's not about ignoring other reviewers comments (I don't believe anybody
ignores a comment intentionally)... I think it's about expecting other reviewers
to read all comments that I made on a code review . I don't think that's a fair
expectation :(. In a way, code reviews are already a significant load just going
through the code.

I usually want to have a fresh mind about the code, so I prefer not looking at
other people's comments when I'm going over the code for the first time. If I
have a comment at some particular point, and there is another comment there (or
close), I usually expand that comment to see if it is the same/related to avoid
saying the same thing again. After that, I may or may not look at the other
comments (more often than not, I do not). But as Darin says, nothing is a hard
rule.

On 2014/09/18 18:13:05, gavinp wrote:
> > As for patch set 28, I don't have any fundamental objection to it. It adds
> > another indirection (WeakPtr) that could have been avoided, but as I said
from
> > the start this is not a path particularly sensitive to... basically
anything.
> > 
> 
> I'd like to hear more about this. This CL both adds and removes indirection,
and
> most significantly it removes a lot of downcasts, or in the case of the simple
> backend, it removes lots of scary reinterpret_cast instances. I think on
balance
> the benefit of removing the downcasts and reinterpret_casts outweighs the loss
> of adding a WeakPtrFactory to one object, and creating a single WeakPtr (=
> 2*sizeof(void*) instance for each enumeration... What do you think about the
> benefit of losing the downcasts?
> 
> Returning specifically to WeakPtr, there's a few places where we use them in
> this CL, and I'm not sure which one you're speaking to. I'll discuss three of
> them, and perhaps you'll have others you want to mention?

What I meant was that in order to implement the Iterator this way there has to
be a weak pointer to the backend on all implementations. That's exactly one of
the points that I mentioned on the bug when I said that I was playing with how
to add a scoped object and I was not so happy about it (because it can be
avoided).

But I agree that the cost of a weak pointer is low, and it is something that we
use a lot (with great success!), so at the end of the day it's not a very
important matter.

As for the casts, it is good to avoid them, but I'm definitely not religious
about it: I think that almost all language constructs have a place. If I give an
object to some user (or pointer to) expecting to receive exactly the same thing
back at some point, and that user returns something else, I fail to see how the
fact that I perform a cast makes things anything worse (or different). It
doesn't really matter what's the type of the object if the user can be confused
enough to return something else. Sure, if I have a lot of layers in my code and
I have to pass a bunch of things when it is easy to invert the order of
variables while I'm typing, having the compiler fail right away by using strong
types is better, but not all problems map into that scenario (and all unit tests
should fail right away), and frankly is not the type of bug that I worry the
most because it is very unlikely to go unnoticed.

And just so that it's completely clear, I do believe that the change as it is
now is a net positive.

[gavinp, 2014-09-19 03:54:37]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-09-19 03:54:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/542733002/650001

[commit-bot: I haz the power, 2014-09-19 04:20:41]

Committed patchset #30 (id:650001) as 9541f4472b2b567f211844cb6a6b48a0010601c2

[commit-bot: I haz the power, 2014-09-19 04:21:14]

Patchset 30 (id:??) landed as
https://crrev.com/732c8306d4864296511e7a3a252724b1bb34c342
Cr-Commit-Position: refs/heads/master@{#295659}

[vasilii, 2014-09-19 08:18:32]

A revert of this CL (patchset #30 id:650001) has been created in
https://codereview.chromium.org/585833002/ by vasilii@chromium.org.

The reason for reverting is: Introduced memory leaks on linux asan
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...:
Direct leak of 80 byte(s) in 2 object(s) allocated from:
#0 0x501ccb in operator new(unsigned long)
(/b/build/slave/Linux_ASan_LSan_Tests__1_/build/src/out/Release/net_unittests+0x501ccb)
#1 0x309f8d7 in disk_cache::BackendImpl::OpenNextEntryImpl(void**)
net/disk_cache/blockfile/backend_impl.cc:620:5
#2 0x30a02cd in disk_cache::BackendImpl::SyncOpenNextEntry(void**,
disk_cache::Entry**) net/disk_cache/blockfile/backend_impl.cc:436:17
#3 0x30d49dd in disk_cache::BackendIO::ExecuteBackendOperation()
net/disk_cache/blockfile/in_flight_backend_io.cc:248:17
#4 0x2e3b0bf in Run base/callback.h:401:12
#5 0x2e3b0bf in base::debug::TaskAnnotator::RunTask(char const*, char const*,
base::PendingTask const&) base/debug/task_annotator.cc:62
#6 0x2dc22fc in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:446:3
#7 0x2dc33cc in DeferOrRunPendingTask base/message_loop/message_loop.cc:456:5
#8 0x2dc33cc in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:565
#9 0x2e2361f in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_libevent.cc:232:21
#10 0x2ddd94b in base::RunLoop::Run() base/run_loop.cc:54:3
#11 0x2dc0bc4 in base::MessageLoop::Run()
base/message_loop/message_loop.cc:308:3
#12 0x2e0ad90 in base::Thread::ThreadMain() base/threading/thread.cc:228:5
#13 0x2dfeaf0 in base::(anonymous namespace)::ThreadFunc(void*)
base/threading/platform_thread_posix.cc:80:3
#14 0x7fa9f4e0ce99 in start_thread
/build/buildd/eglibc-2.15/nptl/pthread_create.c:308

Indirect leak of 64 byte(s) in 2 object(s) allocated from:
#0 0x501ccb in operator new(unsigned long)
(/b/build/slave/Linux_ASan_LSan_Tests__1_/build/src/out/Release/net_unittests+0x501ccb)
#1 0x30ebaaf in
disk_cache::Rankings::GetNext(disk_cache::StorageBlock<disk_cache::RankingsNode>*,
disk_cache::Rankings::List) net/disk_cache/blockfile/rankings.cc:435:5
#2 0x30a219d in
disk_cache::BackendImpl::OpenFollowingEntryFromList(disk_cache::Rankings::List,
disk_cache::StorageBlock<disk_cache::RankingsNode>**, disk_cache::EntryImpl**)
net/disk_cache/blockfile/backend_impl.cc:1694:36
#3 0x309fa3d in disk_cache::BackendImpl::OpenNextEntryImpl(void**)
net/disk_cache/blockfile/backend_impl.cc:638:11
#4 0x30a02cd in disk_cache::BackendImpl::SyncOpenNextEntry(void**,
disk_cache::Entry**) net/disk_cache/blockfile/backend_impl.cc:436:17
#5 0x30d49dd in disk_cache::BackendIO::ExecuteBackendOperation()
net/disk_cache/blockfile/in_flight_backend_io.cc:248:17
#6 0x2e3b0bf in Run base/callback.h:401:12
#7 0x2e3b0bf in base::debug::TaskAnnotator::RunTask(char const*, char const*,
base::PendingTask const&) base/debug/task_annotator.cc:62
#8 0x2dc22fc in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:446:3
#9 0x2dc33cc in DeferOrRunPendingTask base/message_loop/message_loop.cc:456:5
#10 0x2dc33cc in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:565
#11 0x2e2361f in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_libevent.cc:232:21
#12 0x2ddd94b in base::RunLoop::Run() base/run_loop.cc:54:3
#13 0x2dc0bc4 in base::MessageLoop::Run()
base/message_loop/message_loop.cc:308:3
#14 0x2e0ad90 in base::Thread::ThreadMain() base/threading/thread.cc:228:5
#15 0x2dfeaf0 in base::(anonymous namespace)::ThreadFunc(void*)
base/threading/platform_thread_posix.cc:80:3
#16 0x7fa9f4e0ce99 in start_thread
/build/buildd/eglibc-2.15/nptl/pthread_create.c:308

Indirect leak of 36 byte(s) in 1 object(s) allocated from:
#0 0x501ccb in operator new(unsigned long)
(/b/build/slave/Linux_ASan_LSan_Tests__1_/build/src/out/Release/net_unittests+0x501ccb)
#1 0x30c0696 in AllocateData net/disk_cache/blockfile/storage_block-inl.h:179:5
#2 0x30c0696 in disk_cache::StorageBlock<disk_cache::RankingsNode>::Load()
net/disk_cache/blockfile/storage_block-inl.h:121
#3 0x30e6fe9 in
disk_cache::Rankings::GetRanking(disk_cache::StorageBlock<disk_cache::RankingsNode>*)
net/disk_cache/blockfile/rankings.cc:586:8
#4 0x30ebd5a in
disk_cache::Rankings::GetNext(disk_cache::StorageBlock<disk_cache::RankingsNode>*,
disk_cache::Rankings::List) net/disk_cache/blockfile/rankings.cc:440:8
#5 0x30a219d in
disk_cache::BackendImpl::OpenFollowingEntryFromList(disk_cache::Rankings::List,
disk_cache::StorageBlock<disk_cache::RankingsNode>**, disk_cache::EntryImpl**)
net/disk_cache/blockfile/backend_impl.cc:1694:36
#6 0x309fa3d in disk_cache::BackendImpl::OpenNextEntryImpl(void**)
net/disk_cache/blockfile/backend_impl.cc:638:11
#7 0x30a02cd in disk_cache::BackendImpl::SyncOpenNextEntry(void**,
disk_cache::Entry**) net/disk_cache/blockfile/backend_impl.cc:436:17
#8 0x30d49dd in disk_cache::BackendIO::ExecuteBackendOperation()
net/disk_cache/blockfile/in_flight_backend_io.cc:248:17
#9 0x2e3b0bf in Run base/callback.h:401:12
#10 0x2e3b0bf in base::debug::TaskAnnotator::RunTask(char const*, char const*,
base::PendingTask const&) base/debug/task_annotator.cc:62
#11 0x2dc22fc in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:446:3
#12 0x2dc33cc in DeferOrRunPendingTask base/message_loop/message_loop.cc:456:5
#13 0x2dc33cc in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:565
#14 0x2e2361f in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_libevent.cc:232:21
#15 0x2ddd94b in base::RunLoop::Run() base/run_loop.cc:54:3
#16 0x2dc0bc4 in base::MessageLoop::Run()
base/message_loop/message_loop.cc:308:3
#17 0x2e0ad90 in base::Thread::ThreadMain() base/threading/thread.cc:228:5
#18 0x2dfeaf0 in base::(anonymous namespace)::ThreadFunc(void*)
base/threading/platform_thread_posix.cc:80:3
#19 0x7fa9f4e0ce99 in start_thread
/build/buildd/eglibc-2.15/nptl/pthread_create.c:308.

[gavinp, 2014-09-19 16:09:35]

See https://codereview.chromium.org/583283002/ for reland.

https://codereview.chromium.org/542733002/diff/650001/net/disk_cache/blockfil...
File net/disk_cache/blockfile/backend_impl.cc (left):

https://codereview.chromium.org/542733002/diff/650001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl.cc:620: iterator.reset(new
Rankings::Iterator(&rankings_));
Aha, the void** was really a scoped_ptr, very similar to the implementation in
simple cache that was leaking.

https://codereview.chromium.org/542733002/diff/650001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl.cc:1261:
background_queue_.EndEnumeration(*iter);
This turns out to be important.