From 46172ffd0f473e1ba7c80f75436371bfdddad965 Mon Sep 17 00:00:00 2001 From: falhassen Date: Tue, 31 Oct 2017 12:30:25 -0700 Subject: [PATCH] Preserve original on redirect with the set-cookie flow. We need to make sure the original header is retained when we redirect. I filed a request on Cronet to allow headers to be provided to the UrlRequest#followRedirect method: https://bugs.chromium.org/p/chromium/issues/detail?id=779611 Until that API is changed, i.e., pulled into GMSCore, and most clients are using the version of GMSCore with the API change, we can stick with this approach. FYI Cronet generally uses the original headers on redirect: http://[] but modifies the headers for these special cases: hhttp://[] ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=174074572 --- .../ext/cronet/CronetDataSourceTest.java | 36 ++++++++++++++++++- .../ext/cronet/CronetDataSource.java | 34 ++++++++++++------ 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java b/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java index 62b5fb261a..4c6a42849f 100644 --- a/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java +++ b/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java @@ -714,7 +714,7 @@ public final class CronetDataSourceTest { } @Test - public void testRedirectParseAndAttachCookie_dataSourceHandlesSetCookie() + public void testRedirectParseAndAttachCookie_dataSourceHandlesSetCookie_andPreservesOriginalRequestHeaders() throws HttpDataSourceException { dataSourceUnderTest = spy( new CronetDataSource( @@ -728,12 +728,46 @@ public final class CronetDataSourceTest { mockClock, null, true)); + dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE); + mockSingleRedirectSuccess(); testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video"); dataSourceUnderTest.open(testDataSpec); verify(mockUrlRequestBuilder).addHeader(eq("Cookie"), any(String.class)); + verify(mockUrlRequestBuilder, never()).addHeader(eq("Range"), any(String.class)); + verify(mockUrlRequestBuilder, times(2)).addHeader("Content-Type", TEST_CONTENT_TYPE); + verify(mockUrlRequest, never()).followRedirect(); + verify(mockUrlRequest, times(2)).start(); + } + + @Test + public void testRedirectParseAndAttachCookie_dataSourceHandlesSetCookie_andPreservesOriginalRequestHeadersIncludingByteRangeHeader() + throws HttpDataSourceException { + testDataSpec = new DataSpec(Uri.parse(TEST_URL), 1000, 5000, null); + dataSourceUnderTest = spy( + new CronetDataSource( + mockCronetEngine, + mockExecutor, + mockContentTypePredicate, + mockTransferListener, + TEST_CONNECT_TIMEOUT_MS, + TEST_READ_TIMEOUT_MS, + true, // resetTimeoutOnRedirects + mockClock, + null, + true)); + dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE); + + mockSingleRedirectSuccess(); + + testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video"); + + dataSourceUnderTest.open(testDataSpec); + verify(mockUrlRequestBuilder).addHeader(eq("Cookie"), any(String.class)); + verify(mockUrlRequestBuilder, times(2)).addHeader("Range", "bytes=1000-5999"); + verify(mockUrlRequestBuilder, times(2)).addHeader("Content-Type", TEST_CONTENT_TYPE); verify(mockUrlRequest, never()).followRedirect(); verify(mockUrlRequest, times(2)).start(); } diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java index 546aba159d..536155a70f 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java @@ -263,7 +263,11 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou operation.close(); resetConnectTimeout(); currentDataSpec = dataSpec; - currentUrlRequest = buildRequest(dataSpec); + try { + currentUrlRequest = buildRequestBuilder(dataSpec).build(); + } catch (IOException e) { + throw new OpenException(e, currentDataSpec, Status.IDLE); + } currentUrlRequest.start(); try { @@ -439,9 +443,18 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou if (!handleSetCookieRequests || isEmpty(headers.get(SET_COOKIE))) { request.followRedirect(); } else { - UrlRequest.Builder requestBuilder = - cronetEngine.newUrlRequestBuilder(newLocationUrl, /* callback= */ this, executor); currentUrlRequest.cancel(); + DataSpec redirectUrlDataSpec = new DataSpec(Uri.parse(newLocationUrl), + currentDataSpec.postBody, currentDataSpec.absoluteStreamPosition, + currentDataSpec.position, currentDataSpec.length, currentDataSpec.key, + currentDataSpec.flags); + UrlRequest.Builder requestBuilder; + try { + requestBuilder = buildRequestBuilder(redirectUrlDataSpec); + } catch (IOException e) { + exception = e; + return; + } String cookieHeadersValue = parseCookies(headers.get(SET_COOKIE)); attachCookies(requestBuilder, cookieHeadersValue); currentUrlRequest = requestBuilder.build(); @@ -494,7 +507,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou // Internal methods. - private UrlRequest buildRequest(DataSpec dataSpec) throws OpenException { + private UrlRequest.Builder buildRequestBuilder(DataSpec dataSpec) throws IOException { UrlRequest.Builder requestBuilder = cronetEngine.newUrlRequestBuilder( dataSpec.uri.toString(), this, executor).allowDirectExecutor(); // Set the headers. @@ -513,17 +526,16 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou requestBuilder.addHeader(key, headerEntry.getValue()); } if (dataSpec.postBody != null && dataSpec.postBody.length != 0 && !isContentTypeHeaderSet) { - throw new OpenException("POST request with non-empty body must set Content-Type", dataSpec, - Status.IDLE); + throw new IOException("POST request with non-empty body must set Content-Type"); } // Set the Range header. - if (currentDataSpec.position != 0 || currentDataSpec.length != C.LENGTH_UNSET) { + if (dataSpec.position != 0 || dataSpec.length != C.LENGTH_UNSET) { StringBuilder rangeValue = new StringBuilder(); rangeValue.append("bytes="); - rangeValue.append(currentDataSpec.position); + rangeValue.append(dataSpec.position); rangeValue.append("-"); - if (currentDataSpec.length != C.LENGTH_UNSET) { - rangeValue.append(currentDataSpec.position + currentDataSpec.length - 1); + if (dataSpec.length != C.LENGTH_UNSET) { + rangeValue.append(dataSpec.position + dataSpec.length - 1); } requestBuilder.addHeader("Range", rangeValue.toString()); } @@ -541,7 +553,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou executor); } } - return requestBuilder.build(); + return requestBuilder; } private boolean blockUntilConnectTimeout() throws InterruptedException {