From d0d76f214a3417ec39f86b1003dd0850a88638d9 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 18 Mar 2025 10:14:07 -0700 Subject: [PATCH] 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 --- .../datasource/HttpEngineDataSource.java | 64 +++++++++---------- .../datasource/HttpEngineDataSourceTest.java | 12 ++++ 2 files changed, 43 insertions(+), 33 deletions(-) diff --git a/libraries/datasource/src/main/java/androidx/media3/datasource/HttpEngineDataSource.java b/libraries/datasource/src/main/java/androidx/media3/datasource/HttpEngineDataSource.java index fe9f098934..971d96f4c0 100644 --- a/libraries/datasource/src/main/java/androidx/media3/datasource/HttpEngineDataSource.java +++ b/libraries/datasource/src/main/java/androidx/media3/datasource/HttpEngineDataSource.java @@ -956,32 +956,38 @@ public final class HttpEngineDataSource extends BaseDataSource implements HttpDa return remaining; } - /** - * Stores the cookie headers from the response in the default {@link CookieHandler}. - * - *

This is a no-op if the {@link CookieHandler} is not set . - */ - @VisibleForTesting - /* private */ static void storeCookiesFromHeaders(UrlResponseInfo info) { - CookieHandler cookieHandler = CookieHandler.getDefault(); - if (cookieHandler != null) { - try { - cookieHandler.put(new URI(info.getUrl()), info.getHeaders().getAsMap()); - } catch (Exception e) { - Log.w(TAG, "Failed to store cookies in CookieHandler", e); - } + // Stores the cookie headers from the response in the default {@link CookieHandler}. + private static void storeCookiesFromHeaders(UrlResponseInfo info) { + storeCookiesFromHeaders(info, CookieHandler.getDefault()); + } + + // Stores the cookie headers from the response in the provided {@link CookieHandler}. + private static void storeCookiesFromHeaders( + UrlResponseInfo info, @Nullable CookieHandler cookieHandler) { + if (cookieHandler == null) { + return; + } + + try { + cookieHandler.put(new URI(info.getUrl()), info.getHeaders().getAsMap()); + } catch (Exception e) { + Log.w(TAG, "Failed to store cookies in CookieHandler", e); } } @VisibleForTesting /* private */ static String getCookieHeader(String url) { - return getCookieHeader(url, ImmutableMap.of()); + return getCookieHeader(url, ImmutableMap.of(), CookieHandler.getDefault()); + } + + @VisibleForTesting + /* private */ static String getCookieHeader(String url, @Nullable CookieHandler cookieHandler) { + return getCookieHeader(url, ImmutableMap.of(), cookieHandler); } // getCookieHeader maps Set-Cookie2 (RFC 2965) to Cookie just like CookieManager does. - @VisibleForTesting - /* private */ static String getCookieHeader(String url, Map> headers) { - CookieHandler cookieHandler = CookieHandler.getDefault(); + private static String getCookieHeader( + String url, Map> headers, @Nullable CookieHandler cookieHandler) { if (cookieHandler == null) { return ""; } @@ -1059,11 +1065,6 @@ public final class HttpEngineDataSource extends BaseDataSource implements HttpDa this.isClosed = true; } - @SuppressWarnings("argument.type.incompatible") - private void resetDefaultCookieHandler() { - CookieHandler.setDefault(null); - } - @Override public synchronized void onRedirectReceived( UrlRequest request, UrlResponseInfo info, String newLocationUrl) { @@ -1092,20 +1093,17 @@ public final class HttpEngineDataSource extends BaseDataSource implements HttpDa resetConnectTimeout(); } - boolean hasDefaultCookieHandler = CookieHandler.getDefault() != null; - if (!hasDefaultCookieHandler && handleSetCookieRequests) { + CookieHandler cookieHandler = CookieHandler.getDefault(); + + if (cookieHandler == null && handleSetCookieRequests) { // a temporary CookieManager is created for the duration of this request - this guarantees // redirects preserve the cookies correctly. - CookieManager cm = new CookieManager(); - CookieHandler.setDefault(cm); + cookieHandler = new CookieManager(); } - storeCookiesFromHeaders(info); - String cookieHeaders = getCookieHeader(info.getUrl(), info.getHeaders().getAsMap()); - - if (!hasDefaultCookieHandler && handleSetCookieRequests) { - resetDefaultCookieHandler(); - } + storeCookiesFromHeaders(info, cookieHandler); + String cookieHeaders = + getCookieHeader(info.getUrl(), info.getHeaders().getAsMap(), cookieHandler); boolean shouldKeepPost = keepPostFor302Redirects diff --git a/libraries/datasource/src/test/java/androidx/media3/datasource/HttpEngineDataSourceTest.java b/libraries/datasource/src/test/java/androidx/media3/datasource/HttpEngineDataSourceTest.java index 81d6baf417..02d95740a0 100644 --- a/libraries/datasource/src/test/java/androidx/media3/datasource/HttpEngineDataSourceTest.java +++ b/libraries/datasource/src/test/java/androidx/media3/datasource/HttpEngineDataSourceTest.java @@ -1486,6 +1486,18 @@ public final class HttpEngineDataSourceTest { .isEqualTo(TEST_REQUEST_COOKIE + "; " + TEST_REQUEST_COOKIE_2 + ";"); } + @Test + public void getCookieHeader_cookieHandlerCustomHandler() throws Exception { + CookieManager cm = new CookieManager(); + cm.put( + new URI(TEST_URL), + ImmutableMap.of( + "Set-Cookie", ImmutableList.of(TEST_RESPONSE_SET_COOKIE, TEST_RESPONSE_SET_COOKIE_2))); + + assertThat(HttpEngineDataSource.getCookieHeader(TEST_URL, cm)) + .isEqualTo(TEST_REQUEST_COOKIE + "; " + TEST_REQUEST_COOKIE_2 + ";"); + } + @Test public void getCookieHeader_cookieHandlerCookie2() throws Exception { CookieManager cm = new CookieManager();