Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1777333002:
[Cronet] Fix race in UploadDataStream.attachToRequest. (Closed)
Description[Cronet] Fix race in UploadDataStream.attachToRequest.
- Add UploadDataStream.initalizeWithRequest to call DataProvider.getLength() outside of lock.
- Use AttachNativeAdapterToRequest in UrlRequest.startInternal under the lock to ensure that request is not canceled.
BUG=593490
Committed: https://crrev.com/db433eabe1d4854125e00885fbba073ada75ad94
Cr-Commit-Position: refs/heads/master@{#380654}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Enable error prone and address its errors. #Patch Set 3 : Address Helen's comments. #
Total comments: 2
Patch Set 4 : do getLength() outside the lock. #
Total comments: 10
Patch Set 5 : Address Helen's comments. #Patch Set 6 : One more assert. #
Total comments: 3
Patch Set 7 : Move the comment. #
Messages
Total messages: 19 (5 generated)
mef@chromium.org changed reviewers: + clm@google.com, xunjieli@chromium.org
PTAL. https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (left): https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:213: mStarted = true; This was also a problem as 'isDone' is looking at this flag. https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:239: mUploadDataStream.attachToRequest(this, mUrlRequestAdapter); attach only if request is still valid.
Thanks for taking care of this. https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:302: void attachToRequest(final CronetUrlRequest request, final long requestAdapter) { Could you make a comment here to say that this method is always invoked on executor's thread? https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (left): https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:213: mStarted = true; On 2016/03/10 17:10:39, mef wrote: > This was also a problem as 'isDone' is looking at this flag. But if start() is called a second time before the runnable is executed, won't that make the request to be started a second time? https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java (right): https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java:158: // second.block(); Why is this line commented out? We can have a simpler test where we use TestUploadDataProvider with a dummy read, and do urlRequest.start(); urlRequest.cancel(); This will crash in before this CL.
Thanks, PTAL. https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (left): https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:213: mStarted = true; On 2016/03/10 17:29:53, xunjieli wrote: > On 2016/03/10 17:10:39, mef wrote: > > This was also a problem as 'isDone' is looking at this flag. > > But if start() is called a second time before the runnable is executed, won't > that make the request to be started a second time? Ah, excellent point! Done. https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java (right): https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java:158: // second.block(); On 2016/03/10 17:29:53, xunjieli wrote: > Why is this line commented out? > > We can have a simpler test where we use TestUploadDataProvider with a dummy > read, and do > > urlRequest.start(); > urlRequest.cancel(); > > This will crash in before this CL. because getLength() is now called under urlRequest.mUrlRequestAdapterLock, so blocking here causes urlRequest.cancel() to wait forever. Could you elaborate on your test suggestion?
https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java (right): https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java:158: // second.block(); On 2016/03/10 17:44:05, mef wrote: > On 2016/03/10 17:29:53, xunjieli wrote: > > Why is this line commented out? > > > > We can have a simpler test where we use TestUploadDataProvider with a dummy > > read, and do > > > > urlRequest.start(); > > urlRequest.cancel(); > > > > This will crash in before this CL. > > because getLength() is now called under urlRequest.mUrlRequestAdapterLock, so > blocking here causes urlRequest.cancel() to wait forever. > > Could you elaborate on your test suggestion? I see. But this isn't a regression test right? Since this test will pass with and without this change (given that this line is commented out). An alternative is to do a test where we immediately cancel after start(). Since start() is posted asynchronously, cancel() will be guaranteed to destroy the adapter before startInternalLocked() gets run (at least most of the time). This is a regression test because before this change the test will crash, but with this change the test won't. https://codereview.chromium.org/1777333002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:237: if (isDoneLocked()) { use isDone() instead? since startInternalLocked should be invoked with the lock.
PTAL https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java (right): https://codereview.chromium.org/1777333002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java:158: // second.block(); On 2016/03/10 18:14:19, xunjieli wrote: > On 2016/03/10 17:44:05, mef wrote: > > On 2016/03/10 17:29:53, xunjieli wrote: > > > Why is this line commented out? > > > > > > We can have a simpler test where we use TestUploadDataProvider with a dummy > > > read, and do > > > > > > urlRequest.start(); > > > urlRequest.cancel(); > > > > > > This will crash in before this CL. > > > > because getLength() is now called under urlRequest.mUrlRequestAdapterLock, so > > blocking here causes urlRequest.cancel() to wait forever. > > > > Could you elaborate on your test suggestion? > > I see. But this isn't a regression test right? Since this test will pass with > and without this change (given that this line is commented out). Yes, I've split getLength() from attach, so it is done without the lock. > > An alternative is to do a test where we immediately cancel after start(). Since > start() is posted asynchronously, cancel() will be guaranteed to destroy the > adapter before startInternalLocked() gets run (at least most of the time). This > is a regression test because before this change the test will crash, but with > this change the test won't. https://codereview.chromium.org/1777333002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:237: if (isDoneLocked()) { On 2016/03/10 18:14:19, xunjieli wrote: > use isDone() instead? since startInternalLocked should be invoked with the lock. Right, isDoneLocked() expects that lock is already held.
https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:298: * If data provider throws an exception, then it is reported to the request. Could you make a note here to say that that no native function shoudl be contained in this method? https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:219: if (isDoneLocked()) { nit: use isDone() instead to avoid grabbing lock twice. https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:241: private void startInternalLocked() { Could you add a note here saying that caller should make sure that isDone() is false before invoking startInternalLocked()? https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java (right): https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java:203: assertFalse(callback.mOnCanceledCalled); Suggest also check to see that mError is UrlRequestError.LISTENER_EXCEPTION_THROWN.
Thanks, PTAL. https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:298: * If data provider throws an exception, then it is reported to the request. On 2016/03/10 20:32:21, xunjieli wrote: > Could you make a note here to say that that no native function shoudl be > contained in this method? Done, although mUploadDataStreamAdapter doesn't exist yet and urlRequest doesn't expose its native adapter. https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:219: if (isDoneLocked()) { On 2016/03/10 20:32:21, xunjieli wrote: > nit: use isDone() instead to avoid grabbing lock twice. isDoneLocked() is @GuardedBy lock instead of grabbing it: public boolean isDone() { synchronized (mUrlRequestAdapterLock) { return isDoneLocked(); } } https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:241: private void startInternalLocked() { On 2016/03/10 20:32:21, xunjieli wrote: > Could you add a note here saying that caller should make sure that isDone() is > false before invoking startInternalLocked()? Done. https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java (right): https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java:203: assertFalse(callback.mOnCanceledCalled); On 2016/03/10 20:32:21, xunjieli wrote: > Suggest also check to see that mError is > UrlRequestError.LISTENER_EXCEPTION_THROWN. Done.
lgtm mod one minor nit below. https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:219: if (isDoneLocked()) { On 2016/03/10 22:16:30, mef wrote: > On 2016/03/10 20:32:21, xunjieli wrote: > > nit: use isDone() instead to avoid grabbing lock twice. > > isDoneLocked() is @GuardedBy lock instead of grabbing it: > > public boolean isDone() { > synchronized (mUrlRequestAdapterLock) { > return isDoneLocked(); > } > } Sorry, I got those two confused.. https://codereview.chromium.org/1777333002/diff/100001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/100001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:242: // Caller is expected to ensure that request isn't done and mUrlRequestAdapter is valid. nit: maybe make this comment to the method level (java doc) where it is more visible.
Thanks! https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:219: if (isDoneLocked()) { On 2016/03/10 22:40:54, xunjieli wrote: > On 2016/03/10 22:16:30, mef wrote: > > On 2016/03/10 20:32:21, xunjieli wrote: > > > nit: use isDone() instead to avoid grabbing lock twice. > > > > isDoneLocked() is @GuardedBy lock instead of grabbing it: > > > > public boolean isDone() { > > synchronized (mUrlRequestAdapterLock) { > > return isDoneLocked(); > > } > > } > > Sorry, I got those two confused.. NP, I'm not sure whether there is a better name suffix for this. https://codereview.chromium.org/1777333002/diff/100001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/100001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:242: // Caller is expected to ensure that request isn't done and mUrlRequestAdapter is valid. On 2016/03/10 22:40:54, xunjieli wrote: > nit: maybe make this comment to the method level (java doc) where it is more > visible. Done. I wonder whether we should just put isDoneLocked check here for good measure.
still lgtm https://codereview.chromium.org/1777333002/diff/100001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1777333002/diff/100001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:242: // Caller is expected to ensure that request isn't done and mUrlRequestAdapter is valid. On 2016/03/11 15:43:06, mef wrote: > On 2016/03/10 22:40:54, xunjieli wrote: > > nit: maybe make this comment to the method level (java doc) where it is more > > visible. > > Done. I wonder whether we should just put isDoneLocked check here for good > measure. I am not sure. Your call :)
Description was changed from ========== [Cronet] attachToRequest UploadDataStream to UrlRequest in startInternal. BUG=593490 ========== to ========== [Cronet] Fix race in UploadDataStream.attachToRequest. - Add UploadDataStream.initalizeWithRequest to call DataProvider.getLength() outside of lock. - Use AttachNativeAdapterToRequest in UrlRequest.startInternal under the lock to ensure that request is not canceled. BUG=593490 ==========
On 2016/03/11 15:44:32, xunjieli wrote: > still lgtm > > https://codereview.chromium.org/1777333002/diff/100001/components/cronet/andr... > File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java > (right): > > https://codereview.chromium.org/1777333002/diff/100001/components/cronet/andr... > components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:242: > // Caller is expected to ensure that request isn't done and mUrlRequestAdapter > is valid. > On 2016/03/11 15:43:06, mef wrote: > > On 2016/03/10 22:40:54, xunjieli wrote: > > > nit: maybe make this comment to the method level (java doc) where it is more > > > visible. > > > > Done. I wonder whether we should just put isDoneLocked check here for good > > measure. > > I am not sure. Your call :) Current code seems a tiny bit more performant , so I'll leave it as is.
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/1777333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1777333002/120001
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Fix race in UploadDataStream.attachToRequest. - Add UploadDataStream.initalizeWithRequest to call DataProvider.getLength() outside of lock. - Use AttachNativeAdapterToRequest in UrlRequest.startInternal under the lock to ensure that request is not canceled. BUG=593490 ========== to ========== [Cronet] Fix race in UploadDataStream.attachToRequest. - Add UploadDataStream.initalizeWithRequest to call DataProvider.getLength() outside of lock. - Use AttachNativeAdapterToRequest in UrlRequest.startInternal under the lock to ensure that request is not canceled. BUG=593490 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Fix race in UploadDataStream.attachToRequest. - Add UploadDataStream.initalizeWithRequest to call DataProvider.getLength() outside of lock. - Use AttachNativeAdapterToRequest in UrlRequest.startInternal under the lock to ensure that request is not canceled. BUG=593490 ========== to ========== [Cronet] Fix race in UploadDataStream.attachToRequest. - Add UploadDataStream.initalizeWithRequest to call DataProvider.getLength() outside of lock. - Use AttachNativeAdapterToRequest in UrlRequest.startInternal under the lock to ensure that request is not canceled. BUG=593490 Committed: https://crrev.com/db433eabe1d4854125e00885fbba073ada75ad94 Cr-Commit-Position: refs/heads/master@{#380654} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/db433eabe1d4854125e00885fbba073ada75ad94 Cr-Commit-Position: refs/heads/master@{#380654} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
