AU: Fix potential issues with premature destruction of HTTP fetchers.

AU: Fix potential issues with premature destruction of HTTP fetchers.

This patch adds a new TransferTerminated callback to the
HttpFetcher class. It fixes two potential memory corruption
issues with premature destruction of HttpFetcher instances:

1. When MultiHttpFetcher completes a range, it terminates
the current fetcher and starts the next one, if any. Change
so that the next fetcher is started when the
TransferTerminated callback is received from the current
fetcher. This prevents the multi fetcher from sending a
TransferComplete/TransferTerminated callbacks before the
underlying fetcher is cleaned up, which may lead to the
fetchers being destroyed prematurely.

2. If the download action fails due to a failed write,
terminate the transfer and then wait for the transfer
terminated callback before notifying the action processor
that the action is complete. Otherwise, the action may get
destroyed before the transfer is actually terminated
possibly leading to memory corruption, etc.

Hopefully these changes fix crosbug.com/8798.

BUG=8798
TEST=unit tests, tested on device with write errors

Change-Id: If416b95625ab31662f2e1308df6bdd1757a2ad78

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=9ce452b

[adlr, 2010-11-17 21:33:50]

a couple nits/questions

http://codereview.chromium.org/5009009/diff/5001/http_fetcher_unittest.cc
File http_fetcher_unittest.cc (right):

http://codereview.chromium.org/5009009/diff/5001/http_fetcher_unittest.cc#new...
http_fetcher_unittest.cc:323: ADD_FAILURE(); // We should never get here
put space back before //

http://codereview.chromium.org/5009009/diff/5001/http_fetcher_unittest.cc#new...
http_fetcher_unittest.cc:328: CHECK(!once_);
use EXPECT rather than CHECK? Unless that doesn't work here. EXPECT seems to
work more places than ASSERT

http://codereview.chromium.org/5009009/diff/5001/libcurl_http_fetcher.cc
File libcurl_http_fetcher.cc (right):

http://codereview.chromium.org/5009009/diff/5001/libcurl_http_fetcher.cc#newc...
libcurl_http_fetcher.cc:143: // Note that after the callback returns this object
may be destroyed.
this comment seems important, maybe it should also be in the .h file?

http://codereview.chromium.org/5009009/diff/5001/multi_http_fetcher.h
File multi_http_fetcher.h (right):

http://codereview.chromium.org/5009009/diff/5001/multi_http_fetcher.h#newcode93
multi_http_fetcher.h:93: TransferTerminated(this);
i'm confused as to what sequence of events would lead this line to get executed.
can you tell me?

thanks

[petkov, 2010-11-17 21:47:54]

PTAL

http://codereview.chromium.org/5009009/diff/5001/http_fetcher_unittest.cc
File http_fetcher_unittest.cc (right):

http://codereview.chromium.org/5009009/diff/5001/http_fetcher_unittest.cc#new...
http_fetcher_unittest.cc:323: ADD_FAILURE(); // We should never get here
On 2010/11/17 21:33:50, adlr wrote:
> put space back before //

Done.

http://codereview.chromium.org/5009009/diff/5001/http_fetcher_unittest.cc#new...
http_fetcher_unittest.cc:328: CHECK(!once_);
On 2010/11/17 21:33:50, adlr wrote:
> use EXPECT rather than CHECK? Unless that doesn't work here. EXPECT seems to
> work more places than ASSERT

Done.

http://codereview.chromium.org/5009009/diff/5001/libcurl_http_fetcher.cc
File libcurl_http_fetcher.cc (right):

http://codereview.chromium.org/5009009/diff/5001/libcurl_http_fetcher.cc#newc...
libcurl_http_fetcher.cc:143: // Note that after the callback returns this object
may be destroyed.
On 2010/11/17 21:33:50, adlr wrote:
> this comment seems important, maybe it should also be in the .h file?

You mean in the docstring for the ForceTransferTermination method? Done.

http://codereview.chromium.org/5009009/diff/5001/multi_http_fetcher.h
File multi_http_fetcher.h (right):

http://codereview.chromium.org/5009009/diff/5001/multi_http_fetcher.h#newcode93
multi_http_fetcher.h:93: TransferTerminated(this);
On 2010/11/17 21:33:50, adlr wrote:
> i'm confused as to what sequence of events would lead this line to get
executed.
> can you tell me?
> 
> thanks

I added a comment in the code. I guess the only case is when TerminateTransfer
is invoked before set_ranges.

[adlr, 2010-11-17 22:32:41]

LGTM. thanks!