net: Bind WeakPtr to HttpNetworkTransaction::io_callback_

net: Bind WeakPtr to HttpNetworkTransaction::io_callback_

To avoid use-after-free

BUG=244725
TEST=net_unittests

[hashimoto, 2013-09-17 04:30:00]

Please review.

[hashimoto, 2013-09-19 05:32:16]

Eric, could you take a look at this 8 line change?

[eroman, 2013-09-19 20:29:43]

Can you explain the situation trying to be fixed?

Using a weakptr here may be masking a deeper problem with not properly
cancelling work the transaction is destroyed.

[hashimoto, 2013-09-20 02:51:56]

On 2013/09/19 20:29:43, eroman wrote:
> Can you explain the situation trying to be fixed?
> 
> Using a weakptr here may be masking a deeper problem with not properly
> cancelling work the transaction is destroyed.

UploadDataStream does not cancel async file operations even when
HttpNetworkTransaction gets destroyed (http://crbug.com/244725) because HNT does
not own UDS, instead, HNT references UDS through a raw pointer of
HttpRequestInfo so destruction of HNT does not affect UDS.
The reason why UDS is not owned by HNT is to make it possible to reuse the same
UDS to send multiple requests (e.g. for redirects).
I think using WeakPtr is the right and the most straight-forward solution for
this problem.

FYI: HttpNetworkTransaction started using base::Unretained, instead of WeakPtr,
when it was performing file IO synchronously. (crrev.com/115220),
and all other users of UploadDataStream are using WeakPtr:
- HttpStreamParser
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...
- SpdyHttpStream
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_http...
- QuicHttpStream
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_http...

[hashimoto, 2013-09-24 00:34:08]

eroman@, PTAL?

[eroman, 2013-09-25 18:46:00]

LGTM to fix the crash.

(Although I still think we should be cancelling these operations).

+mmenke as FYI, in case you have other recommendations.

[hashimoto, 2013-09-26 05:28:44]

Adding mmenke@ as a reviewer just in case.

[mmenke, 2013-09-26 12:32:03]

I'm having trouble seeing how this could happen.

URLRequest is not being destroyed, since otherwise the upload stream would be
destroyed.  The URLRequestJob is only destroyed on URLRequest destruction (Which
isn't happening) and on Restart.  Restart only happens on cert/auth errors
(Which come from the job, after/while establishing a connection, which doesn't
happen at the same time as stream init) or on redirects (Which only happens
after we receive the headers, long after stream init).

Deleting the job deletes the HttpCache::Transaction.  HttpCacheTransaction only
deletes the HttpNetworkTransaction on destruction in DoSuccessfulSendRequest
(When we have response headers, so init is complete),
DoUpdateCachedResponseComplete (Which is only called after
DoSuccessfulSendRequest, when received the headers), SetupEntryForRead (Called
from DoSendRequestComplete when we have an error sending headers and retry a
request from cache and we can't have an error while initializing the upload
data, and in BeginCacheValidation which is called before making a network
request), and DoPartialNetworkReadCompleted, which is called after we've written
some data to the cache, received over the network.

I'd like a unit test at the URLRequest level that demonstrates just how this can
happen, both to understand how it's happening, and to protect against
regressions.

[hashimoto, 2013-09-26 14:54:49]

On 2013/09/26 12:32:03, mmenke wrote:
> I'm having trouble seeing how this could happen.
> 
> URLRequest is not being destroyed, since otherwise the upload stream would be
> destroyed.  The URLRequestJob is only destroyed on URLRequest destruction
(Which
> isn't happening) and on Restart.  Restart only happens on cert/auth errors
> (Which come from the job, after/while establishing a connection, which doesn't
> happen at the same time as stream init) or on redirects (Which only happens
> after we receive the headers, long after stream init).
URLRequest::DoCancel calls URLRequestJob::Kill and URLRequestHttpJob::Kill calls
DestroyTransaction.
I haven't investigated much on this, but can't canceled requests fall into this
kind of use-after-free situation?
> 
> Deleting the job deletes the HttpCache::Transaction.  HttpCacheTransaction
only
> deletes the HttpNetworkTransaction on destruction in DoSuccessfulSendRequest
> (When we have response headers, so init is complete),
> DoUpdateCachedResponseComplete (Which is only called after
> DoSuccessfulSendRequest, when received the headers), SetupEntryForRead (Called
> from DoSendRequestComplete when we have an error sending headers and retry a
> request from cache and we can't have an error while initializing the upload
> data, and in BeginCacheValidation which is called before making a network
> request), and DoPartialNetworkReadCompleted, which is called after we've
written
> some data to the cache, received over the network.
> 
> I'd like a unit test at the URLRequest level that demonstrates just how this
can
> happen, both to understand how it's happening, and to protect against
> regressions.

[mmenke, 2013-09-26 15:00:03]

On 2013/09/26 14:54:49, hashimoto wrote:
> On 2013/09/26 12:32:03, mmenke wrote:
> > I'm having trouble seeing how this could happen.
> > 
> > URLRequest is not being destroyed, since otherwise the upload stream would
be
> > destroyed.  The URLRequestJob is only destroyed on URLRequest destruction
> (Which
> > isn't happening) and on Restart.  Restart only happens on cert/auth errors
> > (Which come from the job, after/while establishing a connection, which
doesn't
> > happen at the same time as stream init) or on redirects (Which only happens
> > after we receive the headers, long after stream init).
> URLRequest::DoCancel calls URLRequestJob::Kill and URLRequestHttpJob::Kill
calls
> DestroyTransaction.
> I haven't investigated much on this, but can't canceled requests fall into
this
> kind of use-after-free situation?

Ahh...That makes sense.  Not sure who would cancel a request but not delete it,
but I agree that we don't want to crash in that case.  I don't think adding more
weak pointers is the solution, though.  Want to investigate more, but I'd prefer
to add a function to the stream that destroys outstanding weak pointers instead.

[hashimoto, 2013-10-02 03:46:08]

Closing this issue.
Will submit https://codereview.chromium.org/25217002/ instead.