Instantiate HttpRequestInfo before HttpNetworkTransaction in unit tests.

The HttpRequestInfo object must outlive the HttpNetworkTransaction object because HttpRequestInfo is passed to HttpNetworkTransaction::Start() and HttpNetworkTransaction access the request info object in many places.

With the current code UploadData gets deleted before HttpStreamParser and that causes
read-after-free cases as shown in the below bug.

BUG=70825
TEST=verify that the valgrind issue mentioned in the bug gets fixed

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

[wtc, 2011-01-26 18:35:17]

I added vandebo and willchan as backup reviewers because I may not be able
to review this CL today.  (A one-line change can be very subtle.)

We should understand why UploadData is deleted before UploadDataStream.
That seems wrong, and is new behavior introduced by your chunked upload CL.
Let's get to the bottom of the problem first.

[Nico, 2011-01-26 19:48:04]

Can you remove the suppression for this in
tools/valgrind/memcheck/suppressions.txt in this CL as well?

[Satish, 2011-01-27 13:55:17]

> We should understand why UploadData is deleted before UploadDataStream.
> That seems wrong, and is new behavior introduced by your chunked upload CL.

The chunked upload CL did not introduce any new behavior in this regard, it has
just brought out an existing issue. The problem is that HttpRequestInfo is the
class which holds a reference to UploadData and UploadDataStream just has a raw
pointer to this UploadData. In the normal control flow HttpRequestInfo gets
destroyed after HttpStreamParser so we can access UploadData from within
HttpStreamParser's dtor. But in certain unit tests we have locally declared
HttpRequestInfo objects which go out of scope before the http transaction gets
destroyed, so there it is unsafe to access UploadData from within
HttpStreamParser's dtor. One such test case is
HttpNetworkTransactionTest::Ignores100

I think the safest fix is for UploadDataStream to take a reference to UploadData
as done in this CL, so we don't add any rules on how long objects created before
calling HttpStreamParser have to live. This is in line with why we use refptrs
anyway.

[vandebo (ex-Chrome), 2011-01-27 18:36:45]

I drew out the ownership data, and Satish's explanation makes sense.  UploadData
is already a ref counted object (both HttpRequestInfo and URLRequest hold
references), so this looks like a reasonable change to me.

Please remove the valgrind suppressions in the same CL as the fix.

LGTM, but wait for Will to comment or defer comment, as he's done a lot of
cleanup of object ownership in net recently.

[willchan no longer on Chromium, 2011-01-27 18:54:45]

On Thu, Jan 27, 2011 at 5:55 AM, <satish@chromium.org> wrote:

> We should understand why UploadData is deleted before UploadDataStream.
>> That seems wrong, and is new behavior introduced by your chunked upload
>> CL.
>>
>
> The chunked upload CL did not introduce any new behavior in this regard, it
> has
> just brought out an existing issue. The problem is that HttpRequestInfo is
> the
> class which holds a reference to UploadData and UploadDataStream just has a
> raw
> pointer to this UploadData. In the normal control flow HttpRequestInfo gets
> destroyed after HttpStreamParser so we can access UploadData from within
> HttpStreamParser's dtor. But in certain unit tests we have locally declared
> HttpRequestInfo objects which go out of scope before the http transaction
> gets
> destroyed, so there it is unsafe to access UploadData from within
> HttpStreamParser's dtor. One such test case is
> HttpNetworkTransactionTest::Ignores100
>

Any place where the transaction outlives the HttpRequestInfo is a bug. If
you look at HttpNetworkTransaction, you will see that it accesses |request_|
in many different places. It is assuming that HttpRequestInfo is always
alive. We should probably fix all these instances.


> I think the safest fix is for UploadDataStream to take a reference to
> UploadData
> as done in this CL, so we don't add any rules on how long objects created
> before
> calling HttpStreamParser have to live. This is in line with why we use
> refptrs
> anyway.
>
>
> http://codereview.chromium.org/6327020/
>

[Satish, 2011-01-27 20:32:19]

> Any place where the transaction outlives the HttpRequestInfo is a bug. If
> you look at HttpNetworkTransaction, you will see that it accesses |request_|
> in many different places. It is assuming that HttpRequestInfo is always
> alive. We should probably fix all these instances.

Agreed. The only place so far I noticed this happen is in the unit tests
(http_network_transaction_unittest.cc) where scoped_ptr<HttpNetworkTransaction>
is created first, then a HttpRequestInfo is declared on stack. So they get
destroyed in the reverse order when the test ends, letting HttpRequestInfo to be
destroyed before HttpNetworkTransaction. Should I just fix these tests to create
request info first?

[willchan no longer on Chromium, 2011-01-27 20:34:07]

On Thu, Jan 27, 2011 at 12:32 PM, <satish@chromium.org> wrote:

> Any place where the transaction outlives the HttpRequestInfo is a bug. If
>> you look at HttpNetworkTransaction, you will see that it accesses
>> |request_|
>> in many different places. It is assuming that HttpRequestInfo is always
>> alive. We should probably fix all these instances.
>>
>
> Agreed. The only place so far I noticed this happen is in the unit tests
> (http_network_transaction_unittest.cc) where
> scoped_ptr<HttpNetworkTransaction>
> is created first, then a HttpRequestInfo is declared on stack. So they get
> destroyed in the reverse order when the test ends, letting HttpRequestInfo
> to be
> destroyed before HttpNetworkTransaction. Should I just fix these tests to
> create
> request info first?


That sounds right to me.


>
>
> http://codereview.chromium.org/6327020/
>

[Satish, 2011-01-27 20:46:11]

Thanks. I'll wait for wtc@ to confirm as well before uploading a new patch with
the valgrind suppression removal and the test fix.

[Satish, 2011-01-27 22:17:34]

New patch uploaded fixing all places in that unit test.

[willchan no longer on Chromium, 2011-01-27 22:53:14]

LGTM if valgrind looks good

[Satish, 2011-01-27 23:06:02]

I see valgrind fails on 2 other unrelated tests which are not part of this CL.
So I'll submit this first thing tomorrow morning if there are no objections.

[willchan no longer on Chromium, 2011-01-27 23:08:01]

Sounds good.

On 2011/01/27 23:06:02, Satish wrote:
> I see valgrind fails on 2 other unrelated tests which are not part of this CL.
> So I'll submit this first thing tomorrow morning if there are no objections.

[wtc, 2011-01-28 01:00:38]

LGTM.  Excellent work!

IMPORTANT: in the CL's description, please point out that
the HttpRequestInfo object must outlive the HttpNetworkTransaction
object because HttpRequestInfo is passed to
HttpNetworkTransaction::Start().

A related minor problem: the HttpStreamParser destructor
accesses UploadDataStream unnecessarily.  I will create a
CL to fix that.

http://codereview.chromium.org/6327020/diff/14001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6327020/diff/14001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8138:
scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
Nit: it would be nice to keep 'session_deps' and 'session' together.
You just need to move 'trans'.

This comment also applies to all the unit tests below that
you modified.

[wtc, 2011-01-28 01:06:15]

On 2011/01/27 18:54:45, willchan wrote:
>
> Any place where the transaction outlives the HttpRequestInfo is a bug. If
> you look at HttpNetworkTransaction, you will see that it accesses |request_|
> in many different places. It is assuming that HttpRequestInfo is always
> alive.

I'm glad that willchan also pointed this out.  This is why the
SVN commit message must note this important point.

[wtc, 2011-01-29 01:43:03]

On 2011/01/28 01:00:38, wtc wrote:
>
> A related minor problem: the HttpStreamParser destructor
> accesses UploadDataStream unnecessarily.  I will create a
> CL to fix that.

I was mistaken.  Please ignore this comment.

Unless we can guarantee UploadData::AppendChunk won't be
called after HttpStreamParser is destroyed, it is safer
to call request_body_->set_chunk_callback(NULL) in the
HttpStreamParser destructor.