[android_webview] Fix use after free in intercepted requests.

[android_webview] Fix use after free in intercepted requests.

This fixes the use after free problem that can occur if the Seek
or Read request on the worker thread runs after the job has been
deleted.

TEST=AndroidWebViewTests,android_webview_unittests
BUG=None
R=mnaganov@chromium.org


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170505

[mnaganov (inactive), 2012-11-29 10:46:55]

I assume it's OK for the InputStream implementation to have an instance deleted
on a thread different from the thread on which it was created?

https://codereview.chromium.org/11428052/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/11428052/diff/1/android_webview/browser/net/a...
android_webview/browser/net/android_stream_reader_url_request_job.h:90: class
InputStreamReaderWrapper :
If this class is only used by AndroidStreamReaderURLRequestJob, why not to
define it inside the .cc file?

[mkosiba (inactive), 2012-11-29 15:39:22]

The InputStream should be safe to delete on a different thread (it's using a
GlobalJavaRef underneath). I added some comments on the class as well.

I also added a DCHECK to check that should prevent anyone from accidentally
introducing concurrent operations on the stream.

https://codereview.chromium.org/11428052/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/11428052/diff/1/android_webview/browser/net/a...
android_webview/browser/net/android_stream_reader_url_request_job.h:90: class
InputStreamReaderWrapper :
On 2012/11/29 10:46:55, Mikhail Naganov (Chromium) wrote:
> If this class is only used by AndroidStreamReaderURLRequestJob, why not to
> define it inside the .cc file?

I assumed scoped_refptr would complain that the type isn't defined, but I can
give it a shot.

[mnaganov (inactive), 2012-11-29 17:05:22]

LGTM with a comment

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:64:
scoped_ptr<android_webview::InputStreamReader> input_stream_reader_;
nit: please add DISALLOW_COPY_AND_ASSIGN (sorry for not pointing this out at the
first review)

[joth, 2012-11-29 17:44:09]

lgtm with couple questions & nits/

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:34: //
Thread-safe ref counting is used to ensure that the data is still there
nit: by 'the data' you specifically mean the InputStream object?

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:37: public
base::RefCountedThreadSafe<InputStreamReaderWrapper> {
URLRequestJob is already refcounted thread safe. Did you consider just adding
the methods below as private methods in AndroidStreamReaderURLRequestJob?

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:43:
input_stream_reader_(input_stream_reader.Pass()) {
nit: indent initializers again

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:59:
private:
nit: \n before

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:74:
DCHECK(delegate_.get());
nit: .get() is spurious

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:180:
base::Unretained(dest),
if the job may be killed before the task runs, how do we know that |dest| is
going to still be alive then too?

https://codereview.chromium.org/11428052/diff/5002/android_webview/native/and...
File android_webview/native/android_protocol_handler.cc (right):

https://codereview.chromium.org/11428052/diff/5002/android_webview/native/and...
android_webview/native/android_protocol_handler.cc:57: InputStream& stream,
use pointer: non-const references not allowed.

[mkosiba (inactive), 2012-11-29 18:54:11]

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:34: //
Thread-safe ref counting is used to ensure that the data is still there
On 2012/11/29 17:44:09, joth wrote:
> nit: by 'the data' you specifically mean the InputStream object?

umm.. I mean the InputStream and InputStreamReader instances, will update
comment.

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:37: public
base::RefCountedThreadSafe<InputStreamReaderWrapper> {
On 2012/11/29 17:44:09, joth wrote:
> URLRequestJob is already refcounted thread safe. Did you consider just adding
> the methods below as private methods in AndroidStreamReaderURLRequestJob?

unfortunately URLRequestJob is only RefCounted (not threadsafe).

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:43:
input_stream_reader_(input_stream_reader.Pass()) {
On 2012/11/29 17:44:09, joth wrote:
> nit: indent initializers again

Done.

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:59:
private:
On 2012/11/29 17:44:09, joth wrote:
> nit: \n before

Done.

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:64:
scoped_ptr<android_webview::InputStreamReader> input_stream_reader_;
On 2012/11/29 17:05:22, Mikhail Naganov (Chromium) wrote:
> nit: please add DISALLOW_COPY_AND_ASSIGN (sorry for not pointing this out at
the
> first review)

Done.

https://codereview.chromium.org/11428052/diff/5002/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:180:
base::Unretained(dest),
On 2012/11/29 17:44:09, joth wrote:
> if the job may be killed before the task runs, how do we know that |dest| is
> going to still be alive then too?

I was mislead by the docs on net::URLRequest which sort of imply that dest will
be valid until the job calls NotifyReadComplete. The docs on net::IOBuffer say
it's refcountedthreadsafe exactly for this reason so dropping the Unretained.

https://codereview.chromium.org/11428052/diff/5002/android_webview/native/and...
File android_webview/native/android_protocol_handler.cc (right):

https://codereview.chromium.org/11428052/diff/5002/android_webview/native/and...
android_webview/native/android_protocol_handler.cc:57: InputStream& stream,
On 2012/11/29 17:44:09, joth wrote:
> use pointer: non-const references not allowed. 

ah, well, back to ye olde pointer then

[commit-bot: I haz the power, 2012-11-30 17:14:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/11428052/9001

[commit-bot: I haz the power, 2012-11-30 19:20:36]

Change committed as 170505