Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
743713002:
Cronet Fix Channel Write after Close when request is canceled after success. (Closed)
DescriptionCronet Fix Channel Write after Close when request is canceled after success.
Refactor URLRequestAdapter::Read to fix unbound buffer growth and data corruption in case of synchronous success.
BUG=433348
Committed: https://crrev.com/3e826cf4020a888da25f41d5fb574fa0100c5bae
Cr-Commit-Position: refs/heads/master@{#308283}
Patch Set 1 #Patch Set 2 : Sync #Patch Set 3 : Use IOBufferWithSize, update canceled_ on network thread. #Patch Set 4 : Self review. #
Total comments: 8
Patch Set 5 : Split out change to net::IOBufferWithSize. #
Total comments: 22
Patch Set 6 : Sync #Patch Set 7 : Address Matt's comments. #
Total comments: 2
Patch Set 8 : Sync #Patch Set 9 : Address Matt's comments. #
Total comments: 19
Patch Set 10 : Added UrlRequestMockDataJob and tests that use it. #
Total comments: 40
Patch Set 11 : Sync #Patch Set 12 : Address Matt's comments. #
Total comments: 35
Patch Set 13 : Sync and address Matt's comments. #
Total comments: 4
Patch Set 14 : Sync #Patch Set 15 : Adddress Matt's comments. #Patch Set 16 : Fix compilation error. #Patch Set 17 : Fix Windows linker error. #Messages
Total messages: 38 (2 generated)
mef@chromium.org changed reviewers: + clm@google.com, miloslav@google.com, mmenke@chromium.org, xunjieli@chromium.org
Please take a look. I've reworked a bit of URLRequestAdapter to fix channel write after close and unlimited growth of read_buffer_.
Nice! Looks much cleaner this way. https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java (right): https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:254: static class TestByteChannel extends ChunkedWritableByteChannel { Instances of this class should never be accessed from more than one thread, right? https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:248: if (!url_request_->status().is_success()) { Could url_request_->status().is_success() be false and bytes_read > 0 at the same time? I presume no, but if it could, then maybe we should still send the last bytes successfully read before failing the request.
https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java (right): https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:254: static class TestByteChannel extends ChunkedWritableByteChannel { On 2014/11/26 10:44:35, miloslav wrote: > Instances of this class should never be accessed from more than one thread, > right? Yes, in old API all operations with this channel are done on network thread. I had to inherit it from ChunkedWritableByteChannel instead of implementing WritableByteChannelInterface because ChromiumUrlRequest upcasts mSink to it in couple of places. I don't like that, but decided not to touch it as this API is on its way out. https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:248: if (!url_request_->status().is_success()) { On 2014/11/26 10:44:35, miloslav wrote: > Could url_request_->status().is_success() be false and bytes_read > 0 at the > same time? I presume no, but if it could, then maybe we should still send the > last bytes successfully read before failing the request. We use the same logic in other places and I would rather keep it consistent. Matt, WDYT? What are scenarios where some remaining bytes are valuable even though request has failed?
This really should be two CLs, one for each fixed issue. https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:314: if (url_request_ == nullptr) I'm assuming this compiles? Didn't realize scoped_ptrs could be compared to NULL.
Matt, I hear you about 2 separate CLs, but I'm not sure how to split. https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (left): https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:339: delegate_->OnBytesRead(this); I believe that calling OnBytesRead in combination with checking/setting |canceled_| from user thread was responsible for channel write after close, but I'm not sure that it is safe to factor out from the rest of read changes into separate CL. https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:314: if (url_request_ == nullptr) On 2014/11/26 14:07:43, mmenke wrote: > I'm assuming this compiles? Didn't realize scoped_ptrs could be compared to > NULL. Should I add .get() for clarity?
About splitting the CL: Per your description, you're doing two things here: Replacing GrowableIOBuffer with IOBufferWithSize, and fixing the write after close issue. These are two separate issues... Each should have on CL. Also wondering if we could manage to test the GrowableIOBuffer fix. https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:314: if (url_request_ == nullptr) On 2014/11/26 14:14:28, mef wrote: > On 2014/11/26 14:07:43, mmenke wrote: > > I'm assuming this compiles? Didn't realize scoped_ptrs could be compared to > > NULL. > > Should I add .get() for clarity? If it works, I'm assuming it's supposed to work, so I think it's fine as-is.
On 2014/11/26 14:34:14, mmenke wrote: > About splitting the CL: Per your description, you're doing two things here: > > Replacing GrowableIOBuffer with IOBufferWithSize, and fixing the write after > close issue. These are two separate issues... Each should have on CL. I suppose I don't have to replace GrowableIOBuffer with IOBufferWithSize in this CL, but I think that change to Read that removes the growth loop on synchronous success has to stay in this CL to ensure correctness. > Also wondering if we could manage to test the GrowableIOBuffer fix. I'm not sure how to test GrowableIOBuffer change, but if it is not growable, then it isn't going to grow, is it? > > https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... > File components/cronet/android/url_request_adapter.cc (right): > > https://codereview.chromium.org/743713002/diff/60001/components/cronet/androi... > components/cronet/android/url_request_adapter.cc:314: if (url_request_ == > nullptr) > On 2014/11/26 14:14:28, mef wrote: > > On 2014/11/26 14:07:43, mmenke wrote: > > > I'm assuming this compiles? Didn't realize scoped_ptrs could be compared to > > > NULL. > > > > Should I add .get() for clarity? > > If it works, I'm assuming it's supposed to work, so I think it's fine as-is.
On 2014/11/26 14:43:21, mef wrote: > On 2014/11/26 14:34:14, mmenke wrote: > > About splitting the CL: Per your description, you're doing two things here: > > > > Replacing GrowableIOBuffer with IOBufferWithSize, and fixing the write after > > close issue. These are two separate issues... Each should have on CL. > > I suppose I don't have to replace GrowableIOBuffer with IOBufferWithSize in this > CL, but I think that change to Read that removes the growth loop on synchronous > success has to stay in this CL to ensure correctness. > > > Also wondering if we could manage to test the GrowableIOBuffer fix. > > I'm not sure how to test GrowableIOBuffer change, but if it is not growable, > then it isn't going to grow, is it? Indeed, but we have no test that would have doubled the buffer size once. None of our tests even read from the socket twice. Not even clear if we exercise both the sync and async read cases in tests.
On 2014/11/26 14:53:25, mmenke wrote: > On 2014/11/26 14:43:21, mef wrote: > > On 2014/11/26 14:34:14, mmenke wrote: > > > About splitting the CL: Per your description, you're doing two things here: > > > > > > Replacing GrowableIOBuffer with IOBufferWithSize, and fixing the write after > > > close issue. These are two separate issues... Each should have on CL. > > > > I suppose I don't have to replace GrowableIOBuffer with IOBufferWithSize in > this > > CL, but I think that change to Read that removes the growth loop on > synchronous > > success has to stay in this CL to ensure correctness. > > > > > Also wondering if we could manage to test the GrowableIOBuffer fix. > > > > I'm not sure how to test GrowableIOBuffer change, but if it is not growable, > > then it isn't going to grow, is it? > > Indeed, but we have no test that would have doubled the buffer size once. None > of our tests even read from the socket twice. Not even clear if we exercise > both the sync and async read cases in tests. Sorry..that should be "read from the response body twice"
Matt, PTAL, I've split out change to net::IOBufferWithSize into separate CL.
https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java (right): https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:305: Executors.newCachedThreadPool().execute(cancelTask); Any reason not to just cancel on the current thread? https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java (right): https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java:74: public void blockForStarted() { nit: blockForStart is more consistent with blockForComplete. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:184: } nit: Shouldn't use braces on single line if's (Yea, know it was like that before) https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:229: void URLRequestAdapter::Read() { Why does all this stuff here need to be a part of this CL?
https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:260: Read(); Recommend against tail recursion here. Ideally should be optimized out, but that won't happen in debug builds, and it's unclear how deep the stack could end up, in practice, in the worst case. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:277: void URLRequestAdapter::OnBytesRead(int bytes_read) { Suggest just inlining this. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:283: bytes_read_ = 0; If we're going to refactor this logic here, should get rid of bytes_read_, and just pass the value directly to the delegate. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:310: void URLRequestAdapter::OnRequestCanceled() { Having OnCancelRequest and OnRequestCanceled seems like a bad idea. Can just inline this, anyways. Only called in one place, I believe, and just calls into another method, making it rather pointless. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:319: if (url_request_ == nullptr) This handles the cancel after complete case? If you just move the OnRequestCanceled() call into the NULL request check in OnCancelRequest, then can turn this into a DCHECK.
Thanks, PTAL. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java (right): https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:305: Executors.newCachedThreadPool().execute(cancelTask); On 2014/12/02 17:10:24, mmenke wrote: > Any reason not to just cancel on the current thread? I've adopted this test from testAppendChunkRaceWithCancel where we've agreed that cancelling from different thread better simulates the condition. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java (right): https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java:74: public void blockForStarted() { On 2014/12/02 17:10:24, mmenke wrote: > nit: blockForStart is more consistent with blockForComplete. Done. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:184: } On 2014/12/02 17:10:24, mmenke wrote: > nit: Shouldn't use braces on single line if's (Yea, know it was like that > before) Done. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:229: void URLRequestAdapter::Read() { On 2014/12/02 17:10:24, mmenke wrote: > Why does all this stuff here need to be a part of this CL? Discussed. :) https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:260: Read(); On 2014/12/02 18:15:12, mmenke wrote: > Recommend against tail recursion here. Ideally should be optimized out, but > that won't happen in debug builds, and it's unclear how deep the stack could end > up, in practice, in the worst case. Ack. I've tried to post the read to network thread, but that brings up "interesting" interactions with cancel posted from another thread. I'd leave it as is unless you feel strongly about it. Should I just post OnReadCompleted from Read instead? https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:277: void URLRequestAdapter::OnBytesRead(int bytes_read) { On 2014/12/02 18:15:12, mmenke wrote: > Suggest just inlining this. Done. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:283: bytes_read_ = 0; On 2014/12/02 18:15:12, mmenke wrote: > If we're going to refactor this logic here, should get rid of bytes_read_, and > just pass the value directly to the delegate. Done. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:310: void URLRequestAdapter::OnRequestCanceled() { On 2014/12/02 18:15:12, mmenke wrote: > Having OnCancelRequest and OnRequestCanceled seems like a bad idea. Can just > inline this, anyways. Only called in one place, I believe, and just calls into > another method, making it rather pointless. Done. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:319: if (url_request_ == nullptr) On 2014/12/02 18:15:12, mmenke wrote: > This handles the cancel after complete case? If you just move the > OnRequestCanceled() call into the NULL request check in OnCancelRequest, then > can turn this into a DCHECK. I think it also handles complete after cancel, doesn't it?
2 responses, one all-new bonus comment! https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:260: Read(); On 2014/12/02 19:41:39, mef wrote: > On 2014/12/02 18:15:12, mmenke wrote: > > Recommend against tail recursion here. Ideally should be optimized out, but > > that won't happen in debug builds, and it's unclear how deep the stack could > end > > up, in practice, in the worst case. > > Ack. I've tried to post the read to network thread, but that brings up > "interesting" interactions with cancel posted from another thread. I'd leave it > as is unless you feel strongly about it. > Should I just post OnReadCompleted from Read instead? I'd suggest splitting the logic into 3 methods: Read() { while (read completes synchronously) { if (HandleReadResult(read_result) < 0) // break / return on error. } } // Does what OnReadCompleted did. Returns error code on error, OK (Or bytes read - doesn't matter) on success. HandleReadResult(int bytes_read); OnReadCompleted(int bytes_read) { if (HandleReadResult(read_result) < 0) // early return on error Read(); } https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:319: if (url_request_ == nullptr) On 2014/12/02 19:41:39, mef wrote: > On 2014/12/02 18:15:12, mmenke wrote: > > This handles the cancel after complete case? If you just move the > > OnRequestCanceled() call into the NULL request check in OnCancelRequest, then > > can turn this into a DCHECK. > > I think it also handles complete after cancel, doesn't it? Don't think so. Cancel calls into OnRequestCompleted, which deletes the URLRequest. As it no longer exists after cancel, it can't call OnRequestCompleted. https://codereview.chromium.org/743713002/diff/120001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/120001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:172: void URLRequestAdapter::Cancel() { Problem: What if cancel is called on the network thread, during a read? We'll still feed data to the embedder until we get an async read. That's the problem with callbacks on the network thread. :( Should have a test for that, though it may be a bit of a pain to set up.
I'd appreciate a slightly more detailed CL description, eg what the problem is and how this CL solves the problem. I read through the code and the crbug, but it is still quite hard to understand the overall goal here.
On 2014/12/02 20:52:33, xunjieli wrote: > I'd appreciate a slightly more detailed CL description, eg what the problem is > and how this CL solves the problem. I read through the code and the crbug, but > it is still quite hard to understand the overall goal here. Should also include the sync read fix in the description.
On 2014/12/03 16:40:33, mmenke wrote: > On 2014/12/02 20:52:33, xunjieli wrote: > > I'd appreciate a slightly more detailed CL description, eg what the problem is > > and how this CL solves the problem. I read through the code and the crbug, but > > it is still quite hard to understand the overall goal here. > > Should also include the sync read fix in the description. Will do, although I'm not sure how to reproduce the sync read problem without enabling cache.
On 2014/12/03 16:43:11, mef wrote: > On 2014/12/03 16:40:33, mmenke wrote: > > On 2014/12/02 20:52:33, xunjieli wrote: > > > I'd appreciate a slightly more detailed CL description, eg what the problem > is > > > and how this CL solves the problem. I read through the code and the crbug, > but > > > it is still quite hard to understand the overall goal here. > > > > Should also include the sync read fix in the description. > > Will do, although I'm not sure how to reproduce the sync read problem without > enabling cache. Two options: 1) Add more mocks. URLRequestSimpleJob currently can return data synchronously, but there's a bug to change that, so may have to make your own job class type (Based on URLRequestSimpleJob, but simpler...) 2) Could add an option to the Java config to enable data URLs, and use a data URL over 32k. Not sure if we have any other uses for this capability. Does HttpURLConnection support data URLs? Looks like those use URLRequestSimpleJob, too, though, so they may switch to being async in the future. Could put it off until we add cache support, but would be nice to have tests sooner rather than later, since this is actually breaking things in production.
Matt, thanks! I've addressed your comments and will work on reproducing the issue. I think that the most easy / beneficial way is to add a setUseCaches method to HttpUrlRequest (default to false per existing behavior), so it could be controlled by embedder as needed. WDYT? https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:260: Read(); On 2014/12/02 20:33:47, mmenke wrote: > On 2014/12/02 19:41:39, mef wrote: > > On 2014/12/02 18:15:12, mmenke wrote: > > > Recommend against tail recursion here. Ideally should be optimized out, but > > > that won't happen in debug builds, and it's unclear how deep the stack could > > end > > > up, in practice, in the worst case. > > > > Ack. I've tried to post the read to network thread, but that brings up > > "interesting" interactions with cancel posted from another thread. I'd leave > it > > as is unless you feel strongly about it. > > Should I just post OnReadCompleted from Read instead? > > I'd suggest splitting the logic into 3 methods: > > Read() { > while (read completes synchronously) { > if (HandleReadResult(read_result) < 0) > // break / return on error. > } > } > > // Does what OnReadCompleted did. Returns error code on error, OK (Or bytes > read - doesn't matter) on success. > HandleReadResult(int bytes_read); > > OnReadCompleted(int bytes_read) { > if (HandleReadResult(read_result) < 0) > // early return on error > Read(); > } Done. https://codereview.chromium.org/743713002/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:319: if (url_request_ == nullptr) On 2014/12/02 20:33:47, mmenke wrote: > On 2014/12/02 19:41:39, mef wrote: > > On 2014/12/02 18:15:12, mmenke wrote: > > > This handles the cancel after complete case? If you just move the > > > OnRequestCanceled() call into the NULL request check in OnCancelRequest, > then > > > can turn this into a DCHECK. > > > > I think it also handles complete after cancel, doesn't it? > > Don't think so. Cancel calls into OnRequestCompleted, which deletes the > URLRequest. As it no longer exists after cancel, it can't call > OnRequestCompleted. Done.
On 2014/12/03 20:29:54, mef wrote: > Matt, > > thanks! I've addressed your comments and will work on reproducing the issue. > > I think that the most easy / beneficial way is to add a setUseCaches method to > HttpUrlRequest (default to false per existing behavior), so it could be > controlled by embedder as needed. WDYT? Think we should talk about APIs here - don't want to have 15 ways to enable the cache. enableCache. reallyEnableCache. enableCacheInWaysWeCouldOnlyDreamOfWhenWeAddedTheLastMethod....
+1 for enabling/disabling cache on per request basis. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java (right): https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:253: static class TestByteChannel extends ChunkedWritableByteChannel { Maybe a java doc here saying that TestByteChannel is used for making sure write is not called after the channel has been closed. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:280: // Make sure the activity was created as expected. nit: I think this is a stale comment. The null check is moved to launchCronetTestAppWithUrl. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:295: URL, HttpUrlRequest.REQUEST_PRIORITY_LOW, headers, I wonder if we should use a url with actual response body. I am not sure if http://127.0.0.1:8000 gives a response body. Preferably, we should choose url that gives a fairly large response body. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java (right): https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java:72: * Blocks until the request starts. nit: s/request/response to match onResponseStarted.
https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:25: static const size_t kBufferSizeIncrement = 32768; No longer incrementing. kReadBufferSize? https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:230: if (read_buffer_->RemainingCapacity() == 0) { Instead of this, can we just create a fixed buffer here, and replace with with "if (!read_buffer_) { // create buffer here. } https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:239: read_buffer_.get(), read_buffer_->RemainingCapacity(), &bytes_read); read_buffer_->RemainingCapacity -> kBufferSizeIncrement? https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:255: } Fix indent. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:259: read_buffer_->set_offset(0); I don't think this does anything? Once this is removed, we can stop using GrowableIOBuffer, too.
Thanks, PTAL. I've added the URLRequestMockDataJob and tests that use it to synchronously received the data and cancel request from channel write. https://codereview.chromium.org/743713002/diff/120001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/120001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:172: void URLRequestAdapter::Cancel() { On 2014/12/02 20:33:47, mmenke wrote: > Problem: What if cancel is called on the network thread, during a read? We'll > still feed data to the embedder until we get an async read. > > That's the problem with callbacks on the network thread. :( > > Should have a test for that, though it may be a bit of a pain to set up. Done. I don't think embedder can expect immediate cancelation as it is always posted to the network thread. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java (right): https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:253: static class TestByteChannel extends ChunkedWritableByteChannel { On 2014/12/04 15:21:56, xunjieli wrote: > Maybe a java doc here saying that TestByteChannel is used for making sure write > is not called after the channel has been closed. Done. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:280: // Make sure the activity was created as expected. On 2014/12/04 15:21:56, xunjieli wrote: > nit: I think this is a stale comment. The null check is moved to > launchCronetTestAppWithUrl. Done. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:280: // Make sure the activity was created as expected. On 2014/12/04 15:21:56, xunjieli wrote: > nit: I think this is a stale comment. The null check is moved to > launchCronetTestAppWithUrl. Done. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:295: URL, HttpUrlRequest.REQUEST_PRIORITY_LOW, headers, On 2014/12/04 15:21:56, xunjieli wrote: > I wonder if we should use a url with actual response body. I am not sure if > http://127.0.0.1:8000 gives a response body. Preferably, we should choose url > that gives a fairly large response body. http://127.0.0.1:8000 gives a response body, but per separate discussion I'll add another mock http job, which will return a mock response of arbitrary length. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java (right): https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java:72: * Blocks until the request starts. On 2014/12/04 15:21:56, xunjieli wrote: > nit: s/request/response to match onResponseStarted. Done. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:25: static const size_t kBufferSizeIncrement = 32768; On 2014/12/04 20:57:29, mmenke wrote: > No longer incrementing. kReadBufferSize? Done. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:230: if (read_buffer_->RemainingCapacity() == 0) { On 2014/12/04 20:57:29, mmenke wrote: > Instead of this, can we just create a fixed buffer here, and replace with with > "if (!read_buffer_) { > // create buffer here. > } Done. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:239: read_buffer_.get(), read_buffer_->RemainingCapacity(), &bytes_read); On 2014/12/04 20:57:29, mmenke wrote: > read_buffer_->RemainingCapacity -> kBufferSizeIncrement? Done. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:255: } On 2014/12/04 20:57:29, mmenke wrote: > Fix indent. Done. https://codereview.chromium.org/743713002/diff/160001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:259: read_buffer_->set_offset(0); On 2014/12/04 20:57:29, mmenke wrote: > I don't think this does anything? Once this is removed, we can stop using > GrowableIOBuffer, too. Done.
Haven't had time to do a full pass today https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.cc (right): https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. -(c), update year https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:7: #include <vector> Not used. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:10: #include "base/compiler_specific.h" Not used, I believe. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:11: #include "base/memory/ref_counted_memory.h" Not used. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:15: #include "net/base/net_errors.h" Not used https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:18: #include "net/http/http_util.h" Is http util used? https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:20: #include "net/url_request/url_request_status.h" Not used. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:29: std::string GetDataFromRequest(net::URLRequest* request) { const net::URLRequest&, here and below? https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:71: data_.reserve(data_repeat_count*data.length()); Space before/after "*". Actually, since this this is test-only code, suggest just getting rid of this line. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:73: data_ += data; Suggest using .append - think it's a little cleaner, and makes the fact the first has an underscore stand out more. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:87: *mime_type = mime_type_; Can we just use the default implementation of this method and the next? https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:99: int* bytes_read) { Fix indent https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:101: buf_size = static_cast<int>(std::min( This is a misnomer. Suggest just setting *bytes_read directly, or creating a new value, bytes_to_read. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:103: data_.length() - data_offset_ + 1)); Remove the "+1". If they're the same, you're at the end. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:129: filter->AddHostnameHandler("https", hostname, URLRequestMockDataJob::Factory); Should use AddHostnameInterceptor instead - URLRequest::ProtocolFactory is deprecated (or should be). https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:157: return GetMockUrl("https", hostname, data, repeat_count); repeat_count is an int, GetMockUrl is defined as taking an unsigned. Suggest ints in both places, since Java doesn't have unsigneds. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.h (right): https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. -(c), 2012->2014 https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:26: class NET_EXPORT URLRequestMockDataJob : public URLRequestJob { Maybe MockData -> MockSyncData (Here and elsewhere) https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:60: void StartAsync(); Suggest just making these private, for now. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:70: std::string charset_; We don't need these, as they're never initialized. Can just set things to "" or call string::clear() instead.
Thanks! https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.cc (right): https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/12/09 21:00:03, mmenke wrote: > -(c), update year Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:7: #include <vector> On 2014/12/09 21:00:03, mmenke wrote: > Not used. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:10: #include "base/compiler_specific.h" On 2014/12/09 21:00:04, mmenke wrote: > Not used, I believe. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:11: #include "base/memory/ref_counted_memory.h" On 2014/12/09 21:00:04, mmenke wrote: > Not used. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:15: #include "net/base/net_errors.h" On 2014/12/09 21:00:04, mmenke wrote: > Not used Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:18: #include "net/http/http_util.h" On 2014/12/09 21:00:03, mmenke wrote: > Is http util used? Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:20: #include "net/url_request/url_request_status.h" On 2014/12/09 21:00:04, mmenke wrote: > Not used. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:29: std::string GetDataFromRequest(net::URLRequest* request) { On 2014/12/09 21:00:03, mmenke wrote: > const net::URLRequest&, here and below? Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:71: data_.reserve(data_repeat_count*data.length()); On 2014/12/09 21:00:03, mmenke wrote: > Space before/after "*". > > Actually, since this this is test-only code, suggest just getting rid of this > line. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:73: data_ += data; On 2014/12/09 21:00:04, mmenke wrote: > Suggest using .append - think it's a little cleaner, and makes the fact the > first has an underscore stand out more. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:87: *mime_type = mime_type_; On 2014/12/09 21:00:03, mmenke wrote: > Can we just use the default implementation of this method and the next? Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:99: int* bytes_read) { On 2014/12/09 21:00:03, mmenke wrote: > Fix indent Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:101: buf_size = static_cast<int>(std::min( On 2014/12/09 21:00:04, mmenke wrote: > This is a misnomer. Suggest just setting *bytes_read directly, or creating a > new value, bytes_to_read. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:103: data_.length() - data_offset_ + 1)); On 2014/12/09 21:00:04, mmenke wrote: > Remove the "+1". If they're the same, you're at the end. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:129: filter->AddHostnameHandler("https", hostname, URLRequestMockDataJob::Factory); On 2014/12/09 21:00:03, mmenke wrote: > Should use AddHostnameInterceptor instead - URLRequest::ProtocolFactory is > deprecated (or should be). Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:157: return GetMockUrl("https", hostname, data, repeat_count); On 2014/12/09 21:00:04, mmenke wrote: > repeat_count is an int, GetMockUrl is defined as taking an unsigned. Suggest > ints in both places, since Java doesn't have unsigneds. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.h (right): https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/12/09 21:00:04, mmenke wrote: > -(c), 2012->2014 Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:26: class NET_EXPORT URLRequestMockDataJob : public URLRequestJob { On 2014/12/09 21:00:04, mmenke wrote: > Maybe MockData -> MockSyncData (Here and elsewhere) I thought that it may come in handy for async tests as well. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:60: void StartAsync(); On 2014/12/09 21:00:04, mmenke wrote: > Suggest just making these private, for now. Done. https://codereview.chromium.org/743713002/diff/180001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:70: std::string charset_; On 2014/12/09 21:00:04, mmenke wrote: > We don't need these, as they're never initialized. Can just set things to "" or > call string::clear() instead. Done.
Just minor suggestions. Last pass should be fast. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:148: assertTrue(isOpen()); Should we also be asserting mRequestToCancelOnWrite.cancel() hasn't been called yet? (i.e. this is the first write) https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:150: mRequestToCancelOnWrite.cancel(); Should we also have a test where we do this on the current thread's executor? https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:163: public boolean isOpen() { Do we need this, or mIsOpen? The super class already tracks closed/open state already. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:177: public void testWriteAfterCancel() throws Exception { Think this test name and the next are confusing: This is named after what shouldn't happen, the other is named after what the test does. Maybe "testNoWriteAfterCancelOnAnotherThread" and "testNoWriteAfterSyncCancel"? https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:203: listener.blockForComplete(); Assert that the byte channel is closed? Could do it on the next one, too. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:228: // Channel will cancel the request from the network thread during write. "during the first write"? https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java:34: private ConditionVariable mComplete = new ConditionVariable(); Make these two final? https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/test/mock_url_request_job_factory.cc (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/mock_url_request_job_factory.cc:22: net::URLRequestMockDataJob::AddUrlHandler(); nit: Per style guide, variables should be declared just above where they're used, so this should probably go below URLRequestMockHTTPJob...Or you could alphabetize them. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/mock_url_request_job_factory.cc:53: static_cast<int>(jdata_repeat_count))); Is this cast needed? If so, what does jint really map to? https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/test/src/org/chromium/cronet_test_apk/MockUrlRequestJobFactory.java (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/src/org/chromium/cronet_test_apk/MockUrlRequestJobFactory.java:118: public String getMockUrlForData(String data, int dataRepeatCount) { Helen did some refactoring here, you'll need to merge and update this code. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:181: return; Can we just DCHECK here? Looks like we make sure we call Cancel() only once in ChromiumUrlRequest. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:233: do { optional: Suggest a while loop instead of a do while loop. Do while loops aren't too common in chrome, and the double bytes_read initialization is a bit ugly, and means we don't save any code by the change. If you prefer to keep the do while, should at least get rid of the first "= 0". https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/url_request_adapter.h:142: bool HandleReadResult(net::URLRequest* request, int bytes_read); Document this method. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/url_request_adapter.h:142: bool HandleReadResult(net::URLRequest* request, int bytes_read); No need to pass in the request, since it's always just url_request_.get(). https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.cc (right): https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:36: int repeat_count = 1; No need to initialize this, since we just return 1 on failure, anyways. https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:37: if (!base::StringToInt(value, &repeat_count)) StringToInt and then returning unsigned is a bug. Suggest a DCHECK on repeat count being greater than 0, here and in MockUrl, and just using ints everywhere. https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.h (right): https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:49: ~URLRequestMockDataJob() override; This doesn't need to be protected, does it? Fine with it either as public or private.
Thanks! https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:148: assertTrue(isOpen()); On 2014/12/10 20:59:14, mmenke wrote: > Should we also be asserting mRequestToCancelOnWrite.cancel() hasn't been called > yet? (i.e. this is the first write) Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:150: mRequestToCancelOnWrite.cancel(); On 2014/12/10 20:59:14, mmenke wrote: > Should we also have a test where we do this on the current thread's executor? Hmm, could you elaborate on that? https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:163: public boolean isOpen() { On 2014/12/10 20:59:14, mmenke wrote: > Do we need this, or mIsOpen? The super class already tracks closed/open state > already. Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:177: public void testWriteAfterCancel() throws Exception { On 2014/12/10 20:59:14, mmenke wrote: > Think this test name and the next are confusing: This is named after what > shouldn't happen, the other is named after what the test does. > > Maybe "testNoWriteAfterCancelOnAnotherThread" and "testNoWriteAfterSyncCancel"? Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:203: listener.blockForComplete(); On 2014/12/10 20:59:14, mmenke wrote: > Assert that the byte channel is closed? Could do it on the next one, too. Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:228: // Channel will cancel the request from the network thread during write. On 2014/12/10 20:59:14, mmenke wrote: > "during the first write"? Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java:34: private ConditionVariable mComplete = new ConditionVariable(); On 2014/12/10 20:59:14, mmenke wrote: > Make these two final? Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/test/mock_url_request_job_factory.cc (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/mock_url_request_job_factory.cc:22: net::URLRequestMockDataJob::AddUrlHandler(); On 2014/12/10 20:59:14, mmenke wrote: > nit: Per style guide, variables should be declared just above where they're > used, so this should probably go below URLRequestMockHTTPJob...Or you could > alphabetize them. Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/mock_url_request_job_factory.cc:53: static_cast<int>(jdata_repeat_count))); On 2014/12/10 20:59:14, mmenke wrote: > Is this cast needed? If so, what does jint really map to? Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/test/src/org/chromium/cronet_test_apk/MockUrlRequestJobFactory.java (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/src/org/chromium/cronet_test_apk/MockUrlRequestJobFactory.java:118: public String getMockUrlForData(String data, int dataRepeatCount) { On 2014/12/10 20:59:14, mmenke wrote: > Helen did some refactoring here, you'll need to merge and update this code. Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:181: return; On 2014/12/10 20:59:14, mmenke wrote: > Can we just DCHECK here? Looks like we make sure we call Cancel() only once in > ChromiumUrlRequest. That check happens on Java thread, which is not the same as network thread. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:233: do { On 2014/12/10 20:59:14, mmenke wrote: > optional: Suggest a while loop instead of a do while loop. Do while loops > aren't too common in chrome, and the double bytes_read initialization is a bit > ugly, and means we don't save any code by the change. > > If you prefer to keep the do while, should at least get rid of the first "= 0". Done. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/url_request_adapter.h:142: bool HandleReadResult(net::URLRequest* request, int bytes_read); On 2014/12/10 20:59:14, mmenke wrote: > No need to pass in the request, since it's always just url_request_.get(). Done. https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.cc (right): https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:36: int repeat_count = 1; On 2014/12/10 20:59:14, mmenke wrote: > No need to initialize this, since we just return 1 on failure, anyways. Done. https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:37: if (!base::StringToInt(value, &repeat_count)) On 2014/12/10 20:59:14, mmenke wrote: > StringToInt and then returning unsigned is a bug. > > Suggest a DCHECK on repeat count being greater than 0, here and in MockUrl, and > just using ints everywhere. Done. https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.h (right): https://codereview.chromium.org/743713002/diff/220001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.h:49: ~URLRequestMockDataJob() override; On 2014/12/10 20:59:15, mmenke wrote: > This doesn't need to be protected, does it? Fine with it either as public or > private. Done.
quick responses https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:150: mRequestToCancelOnWrite.cancel(); On 2014/12/10 23:23:35, mef wrote: > On 2014/12/10 20:59:14, mmenke wrote: > > Should we also have a test where we do this on the current thread's executor? > > Hmm, could you elaborate on that? Run a that cancels on the current thread, and make sure we never call write after that - much like testWriteAfterCancel, but on the same thread. The big difference is that, with an executor that does stuff on a single thread, we should never call write after cancel, even if we cancel asynchronously, and we can check that. When cancelling on another thread, we could. https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/220001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:181: return; On 2014/12/10 23:23:35, mef wrote: > On 2014/12/10 20:59:14, mmenke wrote: > > Can we just DCHECK here? Looks like we make sure we call Cancel() only once > in > > ChromiumUrlRequest. > > That check happens on Java thread, which is not the same as network thread. Right....But it happens behind a lock, before calling into native code. I see no possible way for canceled_ to be true here. If it can be true, we should have a unit test that tests it.
https://codereview.chromium.org/743713002/diff/240001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/240001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:181: return; See earlier comment about this not happening - if it does happen, should have a test for it. https://codereview.chromium.org/743713002/diff/240001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.cc (right): https://codereview.chromium.org/743713002/diff/240001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:69: GetRepeatCountFromRequest(*request)); Again, you're passing an int as an unsigned here.
Oops, forgot those two, sorry about that. PTAL. https://codereview.chromium.org/743713002/diff/240001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/743713002/diff/240001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:181: return; On 2014/12/11 20:04:37, mmenke wrote: > See earlier comment about this not happening - if it does happen, should have a > test for it. Done. https://codereview.chromium.org/743713002/diff/240001/net/test/url_request/ur... File net/test/url_request/url_request_mock_data_job.cc (right): https://codereview.chromium.org/743713002/diff/240001/net/test/url_request/ur... net/test/url_request/url_request_mock_data_job.cc:69: GetRepeatCountFromRequest(*request)); On 2014/12/11 20:04:37, mmenke wrote: > Again, you're passing an int as an unsigned here. Done. Sorry.
LGTM!
Thanks!
On 2014/12/12 21:59:02, mef wrote: > Thanks! Didn't find any obvious nits. Feel free to land :)
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/743713002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/3e826cf4020a888da25f41d5fb574fa0100c5bae Cr-Commit-Position: refs/heads/master@{#308283} |
