AppCache: Ensure inflight ActiveCalls are not destroyed

AppCache: Ensure inflight ActiveCalls are not destroyed

Before this CL, AppCacheDiskCache could delete inflight ActiveCalls without
any abort operation when the backend is disabled, so callbacks bound with
ActiveCall objects are invoked in broken state.

After this CL, ActiveCall hides its constructor and manages its own lifetime
using refptr, so AppCacheDiskCache no longer has to take care of their
lifetime. In addition, AppCacheDiskCache passes its weakptr to ActiveCall,
which in turn checks it when an operation is completed. If the backend is
closed, ActiveCall closes the entry and returns net::ERR_ABORTED.

BUG=488757
TEST=n/a

Committed: https://crrev.com/25d1cd1fefacd75c8537ebdc4b2f4ef11a01bdba
Cr-Commit-Position: refs/heads/master@{#330250}

[nhiroki, 2015-05-13 10:52:45]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-05-13 10:54:51]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-05-13 11:26:31]

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

[nhiroki, 2015-05-13 11:32:33]

Hi michaeln@, can you review this?

I met an error caused by this while I was trying to migrate the SW's backend
from DiskCache to SimpleCache. Please see the link below for more details:
https://codereview.chromium.org/1140853002/

Thanks!

[michaeln, 2015-05-14 02:25:01]

https://codereview.chromium.org/1136373003/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_disk_cache.cc (right):

https://codereview.chromium.org/1136373003/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_disk_cache.cc:130:
base::Bind(&ActiveCall::OnAsyncCompletion, weak_factory_.GetWeakPtr()));
I remember there being a difference in behavior between the backends when it
came to shutdown, but don't remember exactly what that difference is. Given the
weakptr usage, looks like the difference is that pending calls continue and
their completion callbacks can get invoked sometime after the disk_cache_ has
been deleted? Is that right?

If that's the case, we may need to bind ownership of the ActiveCall into the
closure itself. The DiskCache.Open/CreateEntry methods could write their results
to &entry_ptr_, a use-after-free bug.

[nhiroki, 2015-05-14 04:27:06]

Thank you for reviewing. Comment reply only.

https://codereview.chromium.org/1136373003/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_disk_cache.cc (right):

https://codereview.chromium.org/1136373003/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_disk_cache.cc:130:
base::Bind(&ActiveCall::OnAsyncCompletion, weak_factory_.GetWeakPtr()));
On 2015/05/14 02:25:01, michaeln wrote:
> I remember there being a difference in behavior between the backends when it
> came to shutdown, but don't remember exactly what that difference is. Given
the
> weakptr usage, looks like the difference is that pending calls continue and
> their completion callbacks can get invoked sometime after the disk_cache_ has
> been deleted? Is that right?

I haven't had an absolute proof yet, but it seems most likely. I'm now
confirming that the situation actually happens:
https://codereview.chromium.org/1139573003/#ps20001

> If that's the case, we may need to bind ownership of the ActiveCall into the
> closure itself. The DiskCache.Open/CreateEntry methods could write their
results
> to &entry_ptr_, a use-after-free bug.

Good point. I'll remake this patch in that way.

[nhiroki, 2015-05-14 05:00:44]

Patchset #2 (id:60001) has been deleted

[nhiroki, 2015-05-14 05:17:39]

Patchset #2 (id:80001) has been deleted

[nhiroki, 2015-05-14 05:23:36]

On 2015/05/14 04:27:06, nhiroki wrote:
> Thank you for reviewing. Comment reply only.
> 
>
https://codereview.chromium.org/1136373003/diff/40001/content/browser/appcach...
> File content/browser/appcache/appcache_disk_cache.cc (right):
> 
>
https://codereview.chromium.org/1136373003/diff/40001/content/browser/appcach...
> content/browser/appcache/appcache_disk_cache.cc:130:
> base::Bind(&ActiveCall::OnAsyncCompletion, weak_factory_.GetWeakPtr()));
> On 2015/05/14 02:25:01, michaeln wrote:
> > I remember there being a difference in behavior between the backends when it
> > came to shutdown, but don't remember exactly what that difference is. Given
> the
> > weakptr usage, looks like the difference is that pending calls continue and
> > their completion callbacks can get invoked sometime after the disk_cache_
has
> > been deleted? Is that right?
> 
> I haven't had an absolute proof yet, but it seems most likely. I'm now
> confirming that the situation actually happens:
> https://codereview.chromium.org/1139573003/#ps20001

I confirmed that OnAsyncCompletion can be called after AppCacheDiskCache is
disabled (See error log[1] and its patch[2])

[1]
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_dbg...
(please search for
"ServiceWorkerBrowserTest.ResponseFromHTTPServiceWorkerIsNotMarkedAsSecure")
[2] https://codereview.chromium.org/1139573003/#ps20001

[nhiroki, 2015-05-14 05:26:25]

Updated. Can you take another look?

In the new patchset, ...

(1) AppCacheDiskCache passes its weakptr to ActiveCall, which in turn checks it
when an operation is completed. If the backend is closed, ActiveCall closes the
entry and returns net::ERR_ABORTED.

(2) ActiveCall hides its constructor and provides static functions instead, so
the caller (ie. AppCacheDiskCache) no longer has to take care of ActiveCall's
lifetime.

[nhiroki, 2015-05-14 06:23:14]

Patchset #3 (id:120001) has been deleted

[michaeln, 2015-05-14 23:24:00]

https://codereview.chromium.org/1136373003/diff/140001/content/browser/appcac...
File content/browser/appcache/appcache_disk_cache.cc (right):

https://codereview.chromium.org/1136373003/diff/140001/content/browser/appcac...
content/browser/appcache/appcache_disk_cache.cc:126:
base::Unretained(active_call)));
Hmmm... I think this will leak ActiveCall instances when used with the DEFAULT
backend?

The default backend (backend_impl.cc) drops pending io in its dtor without
invoking the completion callback.

The simple backend will get around to invoking completion callback sometime
after backend destruction.

Using base::Owned(active_call) might work provided the CompletionCallback was on
the stack to accomodate the immediate return case where the backend doesn't use
the callback to return the result.

[nhiroki, 2015-05-15 00:42:08]

https://codereview.chromium.org/1136373003/diff/140001/content/browser/appcac...
File content/browser/appcache/appcache_disk_cache.cc (right):

https://codereview.chromium.org/1136373003/diff/140001/content/browser/appcac...
content/browser/appcache/appcache_disk_cache.cc:126:
base::Unretained(active_call)));
On 2015/05/14 23:24:00, michaeln wrote:
> Hmmm... I think this will leak ActiveCall instances when used with the DEFAULT
> backend?
> 
> The default backend (backend_impl.cc) drops pending io in its dtor without
> invoking the completion callback.
> 
> The simple backend will get around to invoking completion callback sometime
> after backend destruction.

Oh...

> Using base::Owned(active_call) might work provided the CompletionCallback was
on
> the stack to accomodate the immediate return case where the backend doesn't
use
> the callback to return the result.
> 

In this way, how can we manage the second arg (ie. entry_ptr) of
Create/Open/DoomEntry call? If |entry_ptr| is owned by |active_call|, the
callback on the stack for the immediate return case cannot access the ptr
because |active_call| is destroyed immediately after
CreateEntry/OpenEntry/DoomEntry call.

(RefCounted<ActiveCall> could work?)

[nhiroki, 2015-05-15 01:02:26]

https://codereview.chromium.org/1136373003/diff/140001/content/browser/appcac...
File content/browser/appcache/appcache_disk_cache.cc (right):

https://codereview.chromium.org/1136373003/diff/140001/content/browser/appcac...
content/browser/appcache/appcache_disk_cache.cc:126:
base::Unretained(active_call)));
On 2015/05/15 00:42:08, nhiroki wrote:
> On 2015/05/14 23:24:00, michaeln wrote:
> > Hmmm... I think this will leak ActiveCall instances when used with the
DEFAULT
> > backend?
> > 
> > The default backend (backend_impl.cc) drops pending io in its dtor without
> > invoking the completion callback.
> > 
> > The simple backend will get around to invoking completion callback sometime
> > after backend destruction.
> 
> Oh...
> 
> > Using base::Owned(active_call) might work provided the CompletionCallback
was
> on
> > the stack to accomodate the immediate return case where the backend doesn't
> use
> > the callback to return the result.
> > 
> 
> In this way, how can we manage the second arg (ie. entry_ptr) of
> Create/Open/DoomEntry call? If |entry_ptr| is owned by |active_call|, the
> callback on the stack for the immediate return case cannot access the ptr
> because |active_call| is destroyed immediately after
> CreateEntry/OpenEntry/DoomEntry call.
> 
> (RefCounted<ActiveCall> could work?)

Uploaded a refcounted version to PS4

[michaeln, 2015-05-15 20:32:51]

lgtm!

[nhiroki, 2015-05-16 00:35:11]

Thank you!

[nhiroki, 2015-05-16 00:35:16]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-05-16 00:36:32]

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

[commit-bot: I haz the power, 2015-05-16 00:42:38]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2015-05-18 11:30:01]

Patchset 5 (id:??) landed as
https://crrev.com/25d1cd1fefacd75c8537ebdc4b2f4ef11a01bdba
Cr-Commit-Position: refs/heads/master@{#330250}