Skip to content

Commit c558ae4

Browse files
Googlertonihei
authored andcommitted
Fix a race condition in HttpEngineDataSource
Pass the temporary CookieHandler as a parameter instead of setting it as a temporary process-default. This avoids a rare race condition, where the player is sending a request with preserveCookies option and runs the `CookieHandler.getDefault()` before a different thread sets a default cookie handler. Over 5 or so lines of code the new default might be reverted to null - this is now fixed. PiperOrigin-RevId: 738052152 (cherry picked from commit d0d76f2)
1 parent 8212b96 commit c558ae4

File tree

2 files changed

+43
-33
lines changed

2 files changed

+43
-33
lines changed

libraries/datasource/src/main/java/androidx/media3/datasource/HttpEngineDataSource.java

Lines changed: 31 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -956,32 +956,38 @@ private static int copyByteBuffer(ByteBuffer src, ByteBuffer dst) {
956956
return remaining;
957957
}
958958

959-
/**
960-
* Stores the cookie headers from the response in the default {@link CookieHandler}.
961-
*
962-
* <p>This is a no-op if the {@link CookieHandler} is not set .
963-
*/
964-
@VisibleForTesting
965-
/* private */ static void storeCookiesFromHeaders(UrlResponseInfo info) {
966-
CookieHandler cookieHandler = CookieHandler.getDefault();
967-
if (cookieHandler != null) {
968-
try {
969-
cookieHandler.put(new URI(info.getUrl()), info.getHeaders().getAsMap());
970-
} catch (Exception e) {
971-
Log.w(TAG, "Failed to store cookies in CookieHandler", e);
972-
}
959+
// Stores the cookie headers from the response in the default {@link CookieHandler}.
960+
private static void storeCookiesFromHeaders(UrlResponseInfo info) {
961+
storeCookiesFromHeaders(info, CookieHandler.getDefault());
962+
}
963+
964+
// Stores the cookie headers from the response in the provided {@link CookieHandler}.
965+
private static void storeCookiesFromHeaders(
966+
UrlResponseInfo info, @Nullable CookieHandler cookieHandler) {
967+
if (cookieHandler == null) {
968+
return;
969+
}
970+
971+
try {
972+
cookieHandler.put(new URI(info.getUrl()), info.getHeaders().getAsMap());
973+
} catch (Exception e) {
974+
Log.w(TAG, "Failed to store cookies in CookieHandler", e);
973975
}
974976
}
975977

976978
@VisibleForTesting
977979
/* private */ static String getCookieHeader(String url) {
978-
return getCookieHeader(url, ImmutableMap.of());
980+
return getCookieHeader(url, ImmutableMap.of(), CookieHandler.getDefault());
979981
}
980982

981-
// getCookieHeader maps Set-Cookie2 (RFC 2965) to Cookie just like CookieManager does.
982983
@VisibleForTesting
983-
/* private */ static String getCookieHeader(String url, Map<String, List<String>> headers) {
984-
CookieHandler cookieHandler = CookieHandler.getDefault();
984+
/* private */ static String getCookieHeader(String url, @Nullable CookieHandler cookieHandler) {
985+
return getCookieHeader(url, ImmutableMap.of(), cookieHandler);
986+
}
987+
988+
// getCookieHeader maps Set-Cookie2 (RFC 2965) to Cookie just like CookieManager does.
989+
private static String getCookieHeader(
990+
String url, Map<String, List<String>> headers, @Nullable CookieHandler cookieHandler) {
985991
if (cookieHandler == null) {
986992
return "";
987993
}
@@ -1059,11 +1065,6 @@ public void close() {
10591065
this.isClosed = true;
10601066
}
10611067

1062-
@SuppressWarnings("argument.type.incompatible")
1063-
private void resetDefaultCookieHandler() {
1064-
CookieHandler.setDefault(null);
1065-
}
1066-
10671068
@Override
10681069
public synchronized void onRedirectReceived(
10691070
UrlRequest request, UrlResponseInfo info, String newLocationUrl) {
@@ -1092,20 +1093,17 @@ public synchronized void onRedirectReceived(
10921093
resetConnectTimeout();
10931094
}
10941095

1095-
boolean hasDefaultCookieHandler = CookieHandler.getDefault() != null;
1096-
if (!hasDefaultCookieHandler && handleSetCookieRequests) {
1096+
CookieHandler cookieHandler = CookieHandler.getDefault();
1097+
1098+
if (cookieHandler == null && handleSetCookieRequests) {
10971099
// a temporary CookieManager is created for the duration of this request - this guarantees
10981100
// redirects preserve the cookies correctly.
1099-
CookieManager cm = new CookieManager();
1100-
CookieHandler.setDefault(cm);
1101+
cookieHandler = new CookieManager();
11011102
}
11021103

1103-
storeCookiesFromHeaders(info);
1104-
String cookieHeaders = getCookieHeader(info.getUrl(), info.getHeaders().getAsMap());
1105-
1106-
if (!hasDefaultCookieHandler && handleSetCookieRequests) {
1107-
resetDefaultCookieHandler();
1108-
}
1104+
storeCookiesFromHeaders(info, cookieHandler);
1105+
String cookieHeaders =
1106+
getCookieHeader(info.getUrl(), info.getHeaders().getAsMap(), cookieHandler);
11091107

11101108
boolean shouldKeepPost =
11111109
keepPostFor302Redirects

libraries/datasource/src/test/java/androidx/media3/datasource/HttpEngineDataSourceTest.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1486,6 +1486,18 @@ public void getCookieHeader_cookieHandler() throws Exception {
14861486
.isEqualTo(TEST_REQUEST_COOKIE + "; " + TEST_REQUEST_COOKIE_2 + ";");
14871487
}
14881488

1489+
@Test
1490+
public void getCookieHeader_cookieHandlerCustomHandler() throws Exception {
1491+
CookieManager cm = new CookieManager();
1492+
cm.put(
1493+
new URI(TEST_URL),
1494+
ImmutableMap.of(
1495+
"Set-Cookie", ImmutableList.of(TEST_RESPONSE_SET_COOKIE, TEST_RESPONSE_SET_COOKIE_2)));
1496+
1497+
assertThat(HttpEngineDataSource.getCookieHeader(TEST_URL, cm))
1498+
.isEqualTo(TEST_REQUEST_COOKIE + "; " + TEST_REQUEST_COOKIE_2 + ";");
1499+
}
1500+
14891501
@Test
14901502
public void getCookieHeader_cookieHandlerCookie2() throws Exception {
14911503
CookieManager cm = new CookieManager();

0 commit comments

Comments
 (0)