Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
945843003:
[Cronet] Do not call into native adapter after it is destroyed in ChromiumUrlRequest.java (Closed)
Description[Cronet] Do not call into native adapter after it is destroyed in ChromiumUrlRequest.java
If consumer calls GetHttpStatusCode/GetHttpStatusText after
the request adapter has been destroyed, we will be in
trouble. The new APIs don't have this problem. This CL adds
null checks on the Java side to make sure we won't call into
the native adapter when it is destroyed.
BUG=460161
Committed: https://crrev.com/97b833e5aeae69178c3c3088702addc88e65c747
Cr-Commit-Position: refs/heads/master@{#319522}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Do null check on java side and cache status code and text #
Total comments: 12
Patch Set 3 : Addressed Comments #
Total comments: 8
Patch Set 4 : check getAllHeaders() throw exception #
Total comments: 8
Patch Set 5 : Address comments #
Total comments: 2
Patch Set 6 : removed mRecycled and used lock when calling native methods on the adapter object #
Total comments: 5
Patch Set 7 : Addressed Misha's comments #
Total comments: 5
Patch Set 8 : Added test in the async case #
Total comments: 2
Patch Set 9 : typo #
Messages
Total messages: 32 (6 generated)
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org
Here's a possible way to mitigate the problem. Not sure if there's a better solution :( Let me know what you think. https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ch... File components/cronet/android/chromium_url_request.cc (left): https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ch... components/cronet/android/chromium_url_request.cc:43: DCHECK(request_adapter); Moving the DCHECK to caller, to make it clearer that we are checking the adapter. https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ch... File components/cronet/android/chromium_url_request.cc (right): https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ch... components/cronet/android/chromium_url_request.cc:416: DCHECK(request_adapter); This method should only be called when the adapter is alive, so I have changed it to a DCHECK.
https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ch... File components/cronet/android/chromium_url_request.cc (right): https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ch... components/cronet/android/chromium_url_request.cc:251: if (request_adapter == NULL) I think these checks make a little more sense Java side - the NULLs originate from how it manages the lifetime of C++-side objects. It may even make sense to cache these in Java, after we've destroyed the request. https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:443: public void resetAdapterForTesting() { Rather than have this method, can we run tests on a ChromiumUrlRequest that has completed - maybe both on one that succeeded and one that failed?
Patchset #2 (id:20001) has been deleted
xunjieli@chromium.org changed reviewers: + miloslav@google.com
+milo Thanks for the review! I have made the suggested changes. PTAL. https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ch... File components/cronet/android/chromium_url_request.cc (right): https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ch... components/cronet/android/chromium_url_request.cc:251: if (request_adapter == NULL) On 2015/02/20 23:19:42, mmenke wrote: > I think these checks make a little more sense Java side - the NULLs originate > from how it manages the lifetime of C++-side objects. > > It may even make sense to cache these in Java, after we've destroyed the > request. Done. https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:443: public void resetAdapterForTesting() { On 2015/02/20 23:19:43, mmenke wrote: > Rather than have this method, can we run tests on a ChromiumUrlRequest that has > completed - maybe both on one that succeeded and one that failed? Done.
https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:150: mHttpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter); Can we populate this in onResponseStarted unconditionally instead? https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:184: mErrorCode = nativeGetErrorCode(mUrlRequestAdapter); Can we populate this and mErrorString in whatever callback from net/ we normally get on failures? (OnRequestFinished?). Then we don't need the validateNotRecycled, either... Also, currently, the validateNotRecycled means we'll actually throw an exception if the adapter is 0 before we get to this line.
https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:184: mErrorCode = nativeGetErrorCode(mUrlRequestAdapter); On 2015/02/25 23:43:57, mmenke wrote: > Can we populate this and mErrorString in whatever callback from net/ we normally > get on failures? (OnRequestFinished?). Then we don't need the > validateNotRecycled, either... Also, currently, the validateNotRecycled means > we'll actually throw an exception if the adapter is 0 before we get to this > line. Also, if we care about this case, seems a problem that it wasn't caught by tests (Which I haven't looked at / thought about yet)
https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... File components/cronet/android/chromium_url_request.cc (right): https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/chromium_url_request.cc:165: DCHECK(request_adapter); It's nice to have these checks, but it would be more useful if you these throw a java exception rather than crashing the process (when enabled). Clients should not crash the process if they get the lifecycle wrong. Maybe a helper function or a macro will do the job? The alternative is to add java-side checks before calling all the native methods (and then leaving these DCHECKs just in case). https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:150: mHttpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter); On 2015/02/25 23:43:57, mmenke wrote: > Can we populate this in onResponseStarted unconditionally instead? +1 for setting in onResponseStarted. https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:164: public String getHttpStatusText() { As above. Also, it would be nice if you change the javadoc of HttpUrlRequest#getHttpStatusCode to mention that it may return null the request has not started.
Thanks for the valuable suggestions! PTAL https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... File components/cronet/android/chromium_url_request.cc (right): https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/chromium_url_request.cc:165: DCHECK(request_adapter); On 2015/02/26 15:56:49, miloslav wrote: > It's nice to have these checks, but it would be more useful if you these throw a > java exception rather than crashing the process (when enabled). Clients should > not crash the process if they get the lifecycle wrong. > > Maybe a helper function or a macro will do the job? The alternative is to add > java-side checks before calling all the native methods (and then leaving these > DCHECKs just in case). Acknowledged. Checking on java side before calling the methods might be better. Looks like we should leave DCHECKs in these JNI methods, as it seems to be the standard way in chromium (If we use @NativeClassQualifiedName, JNI generator will automatically generate those DCHECKs). https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:150: mHttpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter); On 2015/02/25 23:43:57, mmenke wrote: > Can we populate this in onResponseStarted unconditionally instead? Done. https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:150: mHttpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter); On 2015/02/26 15:56:49, miloslav wrote: > On 2015/02/25 23:43:57, mmenke wrote: > > Can we populate this in onResponseStarted unconditionally instead? > > +1 for setting in onResponseStarted. Done. Good idea! thanks! https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:164: public String getHttpStatusText() { On 2015/02/26 15:56:49, miloslav wrote: > As above. Also, it would be nice if you change the javadoc of > HttpUrlRequest#getHttpStatusCode to mention that it may return null the request > has not started. Done. https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:184: mErrorCode = nativeGetErrorCode(mUrlRequestAdapter); On 2015/02/25 23:43:57, mmenke wrote: > Can we populate this and mErrorString in whatever callback from net/ we normally > get on failures? (OnRequestFinished?). Then we don't need the > validateNotRecycled, either... Also, currently, the validateNotRecycled means > we'll actually throw an exception if the adapter is 0 before we get to this > line. Done. Thanks for clarifying about validateNotRecycled. https://codereview.chromium.org/945843003/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:184: mErrorCode = nativeGetErrorCode(mUrlRequestAdapter); On 2015/02/25 23:45:37, mmenke wrote: > On 2015/02/25 23:43:57, mmenke wrote: > > Can we populate this and mErrorString in whatever callback from net/ we > normally > > get on failures? (OnRequestFinished?). Then we don't need the > > validateNotRecycled, either... Also, currently, the validateNotRecycled means > > we'll actually throw an exception if the adapter is 0 before we get to this > > line. > > Also, if we care about this case, seems a problem that it wasn't caught by tests > (Which I haven't looked at / thought about yet) Acknowledged.
Do we need to change HttpUrlConnection-based implementation accordingly? https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... File components/cronet/android/chromium_url_request.cc (left): https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... components/cronet/android/chromium_url_request.cc:43: DCHECK(request_adapter); Why is it removed? https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:156: } check that get[All]Header[s] throw exception? https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:254: assertEquals(-1, request.getHttpStatusCode()); It is unclear why it expects -1 if default is 0. https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:255: assertEquals("", request.getHttpStatusText()); It is unclear why it expects "" if default is null.
I checked HttpUrlConnectionUrlRequest.java briefly, I think we are good there, since we are caching http status code and text, and populating them in the right place. PTAL. https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... File components/cronet/android/chromium_url_request.cc (left): https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... components/cronet/android/chromium_url_request.cc:43: DCHECK(request_adapter); On 2015/02/27 17:24:29, mef wrote: > Why is it removed? I moved it to the call sites, so it's more consistent and clearer that we are DCHECKing in all JNI functions. https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:156: } On 2015/02/27 17:24:29, mef wrote: > check that get[All]Header[s] throw exception? Done. https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:254: assertEquals(-1, request.getHttpStatusCode()); On 2015/02/27 17:24:29, mef wrote: > It is unclear why it expects -1 if default is 0. This is returned by the native stack. Not very intuitive, but I'm not sure how to make it better. https://codereview.chromium.org/945843003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:255: assertEquals("", request.getHttpStatusText()); On 2015/02/27 17:24:29, mef wrote: > It is unclear why it expects "" if default is null. The native stack never returns null. It returns empty string if there isn't a status text. So as long as the request started, getHttpStatusText() won't return null.
I'm going to defer to Misha on the tests, but LGTM, otherwise. https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:481: // onResponseStarted is often not invoked. Worth noting that other than the redirect case, these may be set on the request for AUTH and CERT requests? Otherwise, they're just generally the defaults (-1 / the empty string). https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:485: mHttpStatusText = nativeGetHttpStatusText(mUrlRequestAdapter); Hrm...If we were going to maintain this interface, I'd say we should map the net/ default values to the HttpURLConnection ones, but I don't think it's worth the effort, and could break embedders, in theory. https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/net/TestHttpUrlRequestListener.java (right): https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/net/TestHttpUrlRequestListener.java:54: // invoked (eg. when request fails or redirects are disabled). nit: e.g.
https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:485: mHttpStatusText = nativeGetHttpStatusText(mUrlRequestAdapter); On 2015/03/02 19:22:50, mmenke wrote: > Hrm...If we were going to maintain this interface, I'd say we should map the > net/ default values to the HttpURLConnection ones, but I don't think it's worth > the effort, and could break embedders, in theory. Not sure what this means. HttpURLConnection isn't doing any default mapping from http status code to status text. It just parses the status text from the status line. How does net/ use default values? when the status text part is empty?
https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:485: mHttpStatusText = nativeGetHttpStatusText(mUrlRequestAdapter); On 2015/03/02 19:36:15, xunjieli wrote: > On 2015/03/02 19:22:50, mmenke wrote: > > Hrm...If we were going to maintain this interface, I'd say we should map the > > net/ default values to the HttpURLConnection ones, but I don't think it's > worth > > the effort, and could break embedders, in theory. > > Not sure what this means. HttpURLConnection isn't doing any default mapping from > http status code to status text. It just parses the status text from the status > line. How does net/ use default values? when the status text part is empty? mHttpStatusCode has a default value of 0. If we don't get a status code, net/ uses -1. Is this currently observable somehow? I don't think so, but I'm not confident of that. Certainly would be for non-HTTP requests, but I suppose we don't support them. I was thinking mContentType had a similar behavior (NULL vs ""), but looks like I was wrong about that. mErrorString *does* have similar weirdness - it's "" if the request completed with an error, but NULL beforehand, I believe.
https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:481: // onResponseStarted is often not invoked. On 2015/03/02 19:22:50, mmenke wrote: > Worth noting that other than the redirect case, these may be set on the request > for AUTH and CERT requests? Otherwise, they're just generally the defaults (-1 > / the empty string). Done. https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:485: mHttpStatusText = nativeGetHttpStatusText(mUrlRequestAdapter); On 2015/03/02 20:41:58, mmenke wrote: > On 2015/03/02 19:36:15, xunjieli wrote: > > On 2015/03/02 19:22:50, mmenke wrote: > > > Hrm...If we were going to maintain this interface, I'd say we should map the > > > net/ default values to the HttpURLConnection ones, but I don't think it's > > worth > > > the effort, and could break embedders, in theory. > > > > Not sure what this means. HttpURLConnection isn't doing any default mapping > from > > http status code to status text. It just parses the status text from the > status > > line. How does net/ use default values? when the status text part is empty? > > mHttpStatusCode has a default value of 0. If we don't get a status code, net/ > uses -1. Is this currently observable somehow? I am not sure whether it is observable elsewhere, but it is observed in tests. If we cancel the request, we get back status code of -1, and if we use URLRequestFailedJob, we get 0 back. Not sure if we should convert -1 back to 0 in Java, so it's consistent across. But you've said that it's probably not worth the effort, so I am not sure.. > I don't think so, but I'm not > confident of that. Certainly would be for non-HTTP requests, but I suppose we > don't support them. > > I was thinking mContentType had a similar behavior (NULL vs ""), but looks like > I was wrong about that. > > mErrorString *does* have similar weirdness - it's "" if the request completed > with an error, but NULL beforehand, I believe. I was aware of this too. We can default it to empty string, but I am not sure if that gains any tangible benefit. https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/net/TestHttpUrlRequestListener.java (right): https://codereview.chromium.org/945843003/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/net/TestHttpUrlRequestListener.java:54: // invoked (eg. when request fails or redirects are disabled). On 2015/03/02 19:22:50, mmenke wrote: > nit: e.g. Done.
https://codereview.chromium.org/945843003/diff/100001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:70: // After underlying adapter request is destroyed. getAllHeaders() I think this test (exception thrown by recycled request) should go into separate test, and include getNegotiatedProtocol and getHeader() calls. https://codereview.chromium.org/945843003/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:183: try { Not sure whether having this try/catch section in every test adds any value.
Patchset #6 (id:120001) has been deleted
Talked to Misha offline. We decided to remove mRecycled, since its name is somewhat confusing, and instead to use (mUrlRequestAdapter == 0) as the condition. Also fixed the tests. PTAL. https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:414: synchronized (mLock) { I realize there can be a race when we call get* methods immediately after listener.onRequestComplete() finishes, since the adapter can be destroyed after validateNativeAdapterNotDestroyed() passes but before we actually invoke nativeGet* methods. Therefore, I have protected the access of mUrlRequesAdapter using mLock here and below.
https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... File components/cronet/android/chromium_url_request.cc (right): https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... components/cronet/android/chromium_url_request.cc:407: DCHECK(request_adapter); BUG? Java side only calls validateNotStarted(), which doesn't ensure that jrequest_adapter is valid. https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:52: request.getAllHeaders(); should 'fail' here and below in case if exception is not thrown.
Thanks for the review! https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... File components/cronet/android/chromium_url_request.cc (right): https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... components/cronet/android/chromium_url_request.cc:407: DCHECK(request_adapter); On 2015/03/06 17:15:35, mef wrote: > BUG? Java side only calls validateNotStarted(), which doesn't ensure that > jrequest_adapter is valid. Was thinking that DisableRedirects() won't be called after request finishes, so it's okay to crash. Added the check, I guess it's better to crash Java side then. Done. https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:52: request.getAllHeaders(); On 2015/03/06 17:15:35, mef wrote: > should 'fail' here and below in case if exception is not thrown. Done.
Thanks a lot, just one more thing... https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:247: assertFalse(channel.isOpen()); Should we add checkAfterAdapterDestroyed(request) here as well to test async cancel scenario?
https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:247: assertFalse(channel.isOpen()); On 2015/03/06 18:26:24, mef wrote: > Should we add checkAfterAdapterDestroyed(request) here as well to test async > cancel scenario? The problem is that we are not sure whether the request is canceled at this point. We only know the cancel task is posted.
https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:247: assertFalse(channel.isOpen()); On 2015/03/06 18:30:28, xunjieli wrote: > On 2015/03/06 18:26:24, mef wrote: > > Should we add checkAfterAdapterDestroyed(request) here as well to test async > > cancel scenario? > > The problem is that we are not sure whether the request is canceled at this > point. We only know the cancel task is posted. I see, onRequestComplete() is called before native destroy, so listener.blockForComplete() will race with that, but considering that nativeDestroyRequestAdapter() and valideateNoteDestroyed() are called under mLock, that should be safe and consistent.
Thanks for the review! https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:247: assertFalse(channel.isOpen()); On 2015/03/06 18:52:54, mef wrote: > On 2015/03/06 18:30:28, xunjieli wrote: > > On 2015/03/06 18:26:24, mef wrote: > > > Should we add checkAfterAdapterDestroyed(request) here as well to test async > > > cancel scenario? > > > > The problem is that we are not sure whether the request is canceled at this > > point. We only know the cancel task is posted. > > I see, onRequestComplete() is called before native destroy, so > listener.blockForComplete() will race with that, but considering that > nativeDestroyRequestAdapter() and valideateNoteDestroyed() are called under > mLock, that should be safe and consistent. I see. I was thinking of getHttpStatusCode and getHttpStatusText which will return different results depending on when cancel is called. checkAfterAdapterDestroyed(request) is okay, as you've mentioned it acquires mLock. Done.
lgtm https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:247: assertFalse(channel.isOpen()); On 2015/03/06 19:12:15, xunjieli wrote: > On 2015/03/06 18:52:54, mef wrote: > > On 2015/03/06 18:30:28, xunjieli wrote: > > > On 2015/03/06 18:26:24, mef wrote: > > > > Should we add checkAfterAdapterDestroyed(request) here as well to test > async > > > > cancel scenario? > > > > > > The problem is that we are not sure whether the request is canceled at this > > > point. We only know the cancel task is posted. > > > > I see, onRequestComplete() is called before native destroy, so > > listener.blockForComplete() will race with that, but considering that > > nativeDestroyRequestAdapter() and valideateNoteDestroyed() are called under > > mLock, that should be safe and consistent. > > I see. I was thinking of getHttpStatusCode and getHttpStatusText which will > return different results depending on when cancel is called. > checkAfterAdapterDestroyed(request) is okay, as you've mentioned it acquires > mLock. Done. Yep, I just wanted to ensure that it doesn't crash when request is canceled from another thread (as in the bug). https://codereview.chromium.org/945843003/diff/180001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/180001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:249: // checkAfterAdapterDestroyed() aquire mLock, so this will happen nit: ... acquire mLock this will ...
Makes sense. Thanks for the review! https://codereview.chromium.org/945843003/diff/180001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/945843003/diff/180001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:249: // checkAfterAdapterDestroyed() aquire mLock, so this will happen On 2015/03/06 19:32:18, mef wrote: > nit: ... acquire mLock this will ... Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/945843003/#ps200001 (title: "typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/945843003/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/97b833e5aeae69178c3c3088702addc88e65c747 Cr-Commit-Position: refs/heads/master@{#319522} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
