Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
790273002:
[Cronet] Handle redirects in CronetHttpURLConnection (Closed)
Description[Cronet] Handle redirects in CronetHttpURLConnection
This CL adds redirects handling in CronetHttpURLConnection.
Specifically, embedders will be able to disable/enable redirects by using
setFollowRedirects. However, it is important to note that Cronet does not
support reading response body of a 302 response. This is different from the
default implementation.
BUG=398997
Committed: https://crrev.com/5a7adfac9f93b3c2f2f97091e45fe8e8fc59a7d7
Cr-Commit-Position: refs/heads/master@{#309104}
Patch Set 1 : Rebased #
Total comments: 13
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : Addressed comments #
Total comments: 2
Patch Set 4 : Modified a comment #
Messages
Total messages: 18 (3 generated)
Patchset #1 (id:1) has been deleted
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org
https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:191: } catch (MalformedURLException e) { Not sure if we need to handle malformed url, I guess the request will fail anyway if the new url is malformed. Thoughts?
https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:115: && mResponseInfo.getHttpStatusCode() == 302) { 301, 303, 307, and 308 may also be redirects (308s may not always be redirects, though, as some Google services use them in weird, non-HTTP compliant manners, and the response bodies of those can actually be read). Best to just not explicitly check the response code. Should instead check whether onRedirect was called while instanceFollowRedirects was false. https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:367: } What happens if we try and read the error stream here? Should hold off on landing this until the other one has landed, so we can test that. https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:370: Should have a test where HttpURLConnection.setFollowRedirects(false) is called - I assume it makes the instance equivalent false for connections created after the method is called, but should make sure we respect it, just in case.
Thanks for the review! Very useful suggestions. I will work on the other one first. On Wed, Dec 10, 2014 at 4:44 PM, <mmenke@chromium.org> wrote: > > https://codereview.chromium.org/790273002/diff/20001/ > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java > File > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java > (right): > > https://codereview.chromium.org/790273002/diff/20001/ > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java#newcode115 > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java:115: > && mResponseInfo.getHttpStatusCode() == 302) { > 301, 303, 307, and 308 may also be redirects (308s may not always be > redirects, though, as some Google services use them in weird, non-HTTP > compliant manners, and the response bodies of those can actually be > read). > > Best to just not explicitly check the response code. Should instead > check whether onRedirect was called while instanceFollowRedirects was > false. > > https://codereview.chromium.org/790273002/diff/20001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java > File > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java > (right): > > https://codereview.chromium.org/790273002/diff/20001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java#newcode367 > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java:367: > } > What happens if we try and read the error stream here? Should hold off > on landing this until the other one has landed, so we can test that. > > https://codereview.chromium.org/790273002/diff/20001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java#newcode370 > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java:370: > > Should have a test where HttpURLConnection.setFollowRedirects(false) is > called - I assume it makes the instance equivalent false for connections > created after the method is called, but should make sure we respect it, > just in case. > > https://codereview.chromium.org/790273002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:191: } catch (MalformedURLException e) { On 2014/12/10 20:27:17, xunjieli wrote: > Not sure if we need to handle malformed url, I guess the request will fail > anyway if the new url is malformed. Thoughts? I agree. https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:197: setResponseDataCompleted(); Per Matt's suggestion maybe set a flag indicating that request was canceled due to redirect? https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... File components/cronet/android/test/assets/test/redirect.html.mock-http-headers (left): https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/test/assets/test/redirect.html.mock-http-headers:2: Location: http://mock.http/success.txt Why did it have to change?
Thanks for the review! I have uploaded a new patch to address the comments. After working on the other CL (which does getErrorSteam()), I think the reviewing of this CL does not have much dependency on the other one, so I am sending this one out as well in case you have a chance to look at it next week :) https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:115: && mResponseInfo.getHttpStatusCode() == 302) { On 2014/12/10 21:44:52, mmenke wrote: > 301, 303, 307, and 308 may also be redirects (308s may not always be redirects, > though, as some Google services use them in weird, non-HTTP compliant manners, > and the response bodies of those can actually be read). > > Best to just not explicitly check the response code. Should instead check > whether onRedirect was called while instanceFollowRedirects was false. Done. Smart idea! thanks https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:191: } catch (MalformedURLException e) { On 2014/12/11 16:30:56, mef wrote: > On 2014/12/10 20:27:17, xunjieli wrote: > > Not sure if we need to handle malformed url, I guess the request will fail > > anyway if the new url is malformed. Thoughts? > > I agree. Acknowledged. I will leave it unhandled, unless Matt has any objection. https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:197: setResponseDataCompleted(); On 2014/12/11 16:30:56, mef wrote: > Per Matt's suggestion maybe set a flag indicating that request was canceled due > to redirect? Done. https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... File components/cronet/android/test/assets/test/redirect.html.mock-http-headers (left): https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/test/assets/test/redirect.html.mock-http-headers:2: Location: http://mock.http/success.txt On 2014/12/11 16:30:56, mef wrote: > Why did it have to change? Now the upload test server all serves test files, but with a url like http://127.0.0.1:1234/success.txt. The location is made relative, so we can handle both upload test server and mock urlrequestjobs. https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:367: } On 2014/12/10 21:44:52, mmenke wrote: > What happens if we try and read the error stream here? Should hold off on > landing this until the other one has landed, so we can test that. The error stream should be null according to okhttp's implementation (http://androidxref.com/4.4.3_r1.1/xref/external/okhttp/src/main/java/com/squa...). I have added a TODO. I think we can do the review of both CLs at the same time to reduce review latency :) https://codereview.chromium.org/790273002/diff/20001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:370: On 2014/12/10 21:44:53, mmenke wrote: > Should have a test where HttpURLConnection.setFollowRedirects(false) is called - > I assume it makes the instance equivalent false for connections created after > the method is called, but should make sure we respect it, just in case. Done.
+miloslav who might be able to give us some insights on common pitfalls :)
https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:357: HttpURLConnection.setFollowRedirects(false); I'd like to compare behavior if it's true initially and then set to false after the connection is created (Have to do set it to true within the test, I think, as globals are preserved between running with the two different HttpURLConnection implementations, I believe) https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:367: URL url2 = new URL(UploadTestServer.getFileURL("/multiredirect.html")); Do we get anything from this? I don't really object, just not sure if gets us any extra test coverage. https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:392: // TODO(xunjieli): test that error stream should null here. nit: test->"Test"
Thanks for the review! https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:357: HttpURLConnection.setFollowRedirects(false); On 2014/12/18 16:01:34, mmenke (OOO 12-20 to 1-4) wrote: > I'd like to compare behavior if it's true initially and then set to false after > the connection is created (Have to do set it to true within the test, I think, > as globals are preserved between running with the two different > HttpURLConnection implementations, I believe) Done. https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:367: URL url2 = new URL(UploadTestServer.getFileURL("/multiredirect.html")); On 2014/12/18 16:01:34, mmenke (OOO 12-20 to 1-4) wrote: > Do we get anything from this? I don't really object, just not sure if gets us > any extra test coverage. Yes, you are right, this does not add extra coverage. Removed. https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:392: // TODO(xunjieli): test that error stream should null here. On 2014/12/18 16:01:34, mmenke (OOO 12-20 to 1-4) wrote: > nit: test->"Test" Done.
LGTM https://codereview.chromium.org/790273002/diff/60001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/790273002/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:378: // created should not work. "should not work" is a bit ambiguous (The request shouldn't work, disabling itself should fail, or disabling should not affect the request). Maybe "Disabling redirects globally after creating the HttpURLConnection object should have no effect on the request."
Thanks for the review! https://codereview.chromium.org/790273002/diff/60001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/790273002/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:378: // created should not work. On 2014/12/18 19:41:06, mmenke (OOO 12-20 to 1-4) wrote: > "should not work" is a bit ambiguous (The request shouldn't work, disabling > itself should fail, or disabling should not affect the request). > > Maybe "Disabling redirects globally after creating the HttpURLConnection object > should have no effect on the request." Done.
lgtm with nit https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:190: url = new URL(newLocationUrl); it's a bit confusing that 'url' doesn't follow the normal naming pattern, but I guess it is inherited, so maybe we could use 'this.url' to make it more obvious that this is not a local variable?
https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/790273002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:190: url = new URL(newLocationUrl); On 2014/12/18 20:05:46, mef wrote: > it's a bit confusing that 'url' doesn't follow the normal naming pattern, but I > guess it is inherited, so maybe we could use 'this.url' to make it more obvious > that this is not a local variable? Agree, it is not very intuitive that this variable is from the parent class. but in this scope 'this' refers to CronetUrlRequestListener, so I can't do 'this.url'.
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/790273002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5a7adfac9f93b3c2f2f97091e45fe8e8fc59a7d7 Cr-Commit-Position: refs/heads/master@{#309104} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
