Split a portion of BufferedResourceLoader into a separate class ActiveLoader.

Split a portion of BufferedResourceLoader into a separate class ActiveLoader.

ActiveLoader encapsulates an active WebURLLoader and takes care of maintaining deferred status, references to parent object, and automatic cancelation during teardown.

As a result of fixing the imbalanced reference counts to BufferedResourceLoader there were a few use-after-free bugs due to doing work after executing callbacks. The ordering has been updated to ensure that no more work is done after executing callbacks.

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

[scherkus (not reviewing), 2011-11-23 00:56:31]

This was a smallish enough chunk I could split off from my forest of git trees.

Basic change is the following:
  1) Encapsulate associate state together in ActiveLoader
  2) ActiveLoader also manages the AddRef/Release()

Doing (2) fixes the memory leak, but as noted in CL description causes a bunch
of very fun use-after-free errors to pop up hence the additional changes :\

Patch is based on http://codereview.chromium.org/8649002/

Please take a careful look at locations where we execute a callback inside of
BRL.

[scherkus (not reviewing), 2011-11-23 01:01:33]

ALSO: I'm not married to the name or the approach that much -- I started off
with a simple struct of fields but then didn't like how the code in BRL had to
remember to set struct_.deferred_ and struct_.loader_->setDefersLoading() so I
wrapped it up in a method

[Ami GONE FROM CHROMIUM, 2011-11-29 18:55:49]

http://codereview.chromium.org/8667002/diff/1/tools/valgrind/memcheck/suppres...
File tools/valgrind/memcheck/suppressions.txt (left):

http://codereview.chromium.org/8667002/diff/1/tools/valgrind/memcheck/suppres...
tools/valgrind/memcheck/suppressions.txt:4697: bug_100914a
Rebase?   
(but also, Yay!)

http://codereview.chromium.org/8667002/diff/1/webkit/media/active_loader.h
File webkit/media/active_loader.h (right):

http://codereview.chromium.org/8667002/diff/1/webkit/media/active_loader.h#ne...
webkit/media/active_loader.h:29: WebKit::WebURLLoader* loader);
Doco taking ownership.

http://codereview.chromium.org/8667002/diff/1/webkit/media/buffered_resource_...
File webkit/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/8667002/diff/1/webkit/media/buffered_resource_...
webkit/media/buffered_resource_loader.h:87: // Stops everything associated with
this loader, including active loaders
"loaders", plural?  What?

http://codereview.chromium.org/8667002/diff/1/webkit/media/buffered_resource_...
webkit/media/buffered_resource_loader.h:129: // Used to inject a mock used for
unittests.
IWBN to doco ownership transfer.  Except AFAICT ownership transfer depends on
how far the loader gets before it's destroyed (since ActiveLoader takes
ownership, but only kicks in during Start).  Rationalize?

[Ami GONE FROM CHROMIUM, 2011-11-29 18:56:21]

On 2011/11/23 01:01:33, scherkus wrote:
> ALSO: I'm not married to the name or the approach that much -- I started off
> with a simple struct of fields but then didn't like how the code in BRL had to
> remember to set struct_.deferred_ and struct_.loader_->setDefersLoading() so I
> wrapped it up in a method

Maybe move AL to a subclass of BRL, fwd-declared in brl.h, decl/defn in brl.cc?

[scherkus (not reviewing), 2011-11-29 21:43:50]

I don't think moving AL into BRL will help much. In fact I have bigger plans for
AL that warrant keeping it as a separate class, but I need to vet them first,
but for now having it as a separate file doesn't hurt.

For example we could use AL to further separate BRL from WebURLLoader and let it
take care of things like generating proper headers, validating responses,
checking for same origin stuff, handling redirects, etc...  My hope is to one
day have BRL just worry about the buffering/sliding window aspects and nothing
else.

This is a bit off topic, but for the sake of illustration it should be possible
to rewrite SimpleDataSource to use an ActiveLoader for loading and the buffering
component of BRL for storing the data in memory.

http://codereview.chromium.org/8667002/diff/1/tools/valgrind/memcheck/suppres...
File tools/valgrind/memcheck/suppressions.txt (left):

http://codereview.chromium.org/8667002/diff/1/tools/valgrind/memcheck/suppres...
tools/valgrind/memcheck/suppressions.txt:4697: bug_100914a
On 2011/11/29 18:55:49, Ami Fischman wrote:
> Rebase?   
> (but also, Yay!)

Done.

http://codereview.chromium.org/8667002/diff/1/webkit/media/active_loader.h
File webkit/media/active_loader.h (right):

http://codereview.chromium.org/8667002/diff/1/webkit/media/active_loader.h#ne...
webkit/media/active_loader.h:29: WebKit::WebURLLoader* loader);
On 2011/11/29 18:55:49, Ami Fischman wrote:
> Doco taking ownership.

Done.

http://codereview.chromium.org/8667002/diff/1/webkit/media/buffered_resource_...
File webkit/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/8667002/diff/1/webkit/media/buffered_resource_...
webkit/media/buffered_resource_loader.h:87: // Stops everything associated with
this loader, including active loaders
On 2011/11/29 18:55:49, Ami Fischman wrote:
> "loaders", plural?  What?

Done.

http://codereview.chromium.org/8667002/diff/1/webkit/media/buffered_resource_...
webkit/media/buffered_resource_loader.h:129: // Used to inject a mock used for
unittests.
On 2011/11/29 18:55:49, Ami Fischman wrote:
> IWBN to doco ownership transfer.  Except AFAICT ownership transfer depends on
> how far the loader gets before it's destroyed (since ActiveLoader takes
> ownership, but only kicks in during Start).  Rationalize?

Going to make test_loader_ a scoped_ptr<> that gets .released() + will update
docs

[Ami GONE FROM CHROMIUM, 2011-11-29 21:47:12]

lgtm

[vrk (LEFT CHROMIUM), 2011-12-01 00:22:27]

LGTM

http://codereview.chromium.org/8667002/diff/7001/webkit/media/buffered_resour...
File webkit/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/8667002/diff/7001/webkit/media/buffered_resour...
webkit/media/buffered_resource_loader.cc:479: DoneStart(net::OK);
When would there be a start_callback_ at this point? Can didFinishLoading get
called before didReceiveResponse?

Also was wondering if this is the same scenario as below, where you can have a
start callback or a read callback but never both.

http://codereview.chromium.org/8667002/diff/7001/webkit/media/buffered_resour...
webkit/media/buffered_resource_loader.cc:514: return;
As mentioned offline: DCHECK that you don't have both a start callback and a
read callback.

[scherkus (not reviewing), 2011-12-02 18:24:24]

http://codereview.chromium.org/8667002/diff/7001/webkit/media/buffered_resour...
File webkit/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/8667002/diff/7001/webkit/media/buffered_resour...
webkit/media/buffered_resource_loader.cc:479: DoneStart(net::OK);
On 2011/12/01 00:22:28, Victoria Kirst wrote:
> When would there be a start_callback_ at this point? Can didFinishLoading get
> called before didReceiveResponse?
> 
> Also was wondering if this is the same scenario as below, where you can have a
> start callback or a read callback but never both.

I *think* so -- I'll add in a DCHECK for  now

http://codereview.chromium.org/8667002/diff/7001/webkit/media/buffered_resour...
webkit/media/buffered_resource_loader.cc:514: return;
On 2011/12/01 00:22:28, Victoria Kirst wrote:
> As mentioned offline: DCHECK that you don't have both a start callback and a
> read callback.

Done.