Fix SocketTimeoutException in CronetDataSource.

In some versions of Cronet, the request provided by the callback is not always the same. This leads to callback not completing and request timing out.

The fix changes from having one `UrlRequestCallback` per `CronetDataSource` to one `UrlRequestCallback` per `UrlRequest`. Everytime a current request is canceled, the current callback is closed.

PiperOrigin-RevId: 627379153

RELEASENOTES.md

@@ -46,6 +46,10 @@
 *   Downloads:
 *   OkHttp Extension:
 *   Cronet Extension:
+    *   Fix `SocketTimeoutException` in `CronetDataSource`. In some versions of
+        Cronet, the request provided by the callback is not always the same.
+        This leads to callback not completing and request timing out
+        (https://issuetracker.google.com/328442628).
 *   RTMP Extension:
 *   HLS Extension:
     *   Fix bug where pending EMSG samples waiting for a discontinuity were

libraries/datasource_cronet/src/main/java/androidx/media3/datasource/cronet/CronetDataSource.java

@@ -22,6 +22,7 @@ import static org.chromium.net.UrlRequest.Builder.REQUEST_PRIORITY_MEDIUM;
 import android.net.Uri;
 import android.text.TextUtils;
 import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
 import androidx.media3.common.C;
 import androidx.media3.common.MediaLibraryInfo;
 import androidx.media3.common.PlaybackException;
@@ -55,6 +56,9 @@ import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.concurrent.Executor;
+import org.checkerframework.checker.nullness.qual.EnsuresNonNull;
+import org.checkerframework.checker.nullness.qual.RequiresNonNull;
+import org.checkerframework.dataflow.qual.SideEffectFree;
 import org.chromium.net.CronetEngine;
 import org.chromium.net.CronetException;
 import org.chromium.net.NetworkException;
@@ -441,8 +445,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
   // The size of read buffer passed to cronet UrlRequest.read().
   private static final int DEFAULT_READ_BUFFER_SIZE_BYTES = 32 * 1024;
 
-  /* package */ final UrlRequest.Callback urlRequestCallback;
-
   private final CronetEngine cronetEngine;
   private final Executor executor;
   private final int requestPriority;
@@ -467,6 +469,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
   // Written from the calling thread only. currentUrlRequest.start() calls ensure writes are visible
   // to reads made by the Cronet thread.
   @Nullable private UrlRequest currentUrlRequest;
+  @VisibleForTesting @Nullable /* package */ UrlRequestCallback currentUrlRequestCallback;
   @Nullable private DataSpec currentDataSpec;
 
   // Reference written and read by calling thread only. Passed to Cronet thread as a local variable.
@@ -510,7 +513,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
     this.keepPostFor302Redirects = keepPostFor302Redirects;
     clock = Clock.DEFAULT;
     this.readBufferSize = readBufferSize;
-    urlRequestCallback = new UrlRequestCallback();
     requestProperties = new RequestProperties();
     operation = new ConditionVariable();
   }
@@ -576,8 +578,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
     currentDataSpec = dataSpec;
     UrlRequest urlRequest;
     try {
-      urlRequest = buildRequestBuilder(dataSpec).build();
+      createCurrentUrlRequestAndCallback(dataSpec);
-      currentUrlRequest = urlRequest;
+      urlRequest = currentUrlRequest;
     } catch (IOException e) {
       if (e instanceof HttpDataSourceException) {
         throw (HttpDataSourceException) e;
@@ -817,10 +819,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
   @UnstableApi
   @Override
   public synchronized void close() {
-    if (currentUrlRequest != null) {
+    closeCurrentUrlRequestAndCallback();
-      currentUrlRequest.cancel();
-      currentUrlRequest = null;
-    }
     if (readBuffer != null) {
       readBuffer.limit(0);
     }
@@ -834,6 +833,18 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
     }
   }
 
+  private void closeCurrentUrlRequestAndCallback() {
+    if (currentUrlRequest != null) {
+      currentUrlRequest.cancel();
+      currentUrlRequest = null;
+    }
+
+    if (currentUrlRequestCallback != null) {
+      currentUrlRequestCallback.close();
+      currentUrlRequestCallback = null;
+    }
+  }
+
   /** Returns current {@link UrlRequest}. May be null if the data source is not opened. */
   @UnstableApi
   @Nullable
@@ -848,11 +859,26 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
     return responseInfo;
   }
 
+  // The nullness checker can't prove that UrlRequest.Builder.build() doesn't null out
+  // this.currentUrlRequestCallback.
+  // TODO: Add @SideEffectFree to the UrlRequest.Builder.build() stub
+  @SuppressWarnings("nullness:contracts.postcondition.not.satisfied")
+  @EnsuresNonNull({"this.currentUrlRequestCallback", "this.currentUrlRequest"})
+  private void createCurrentUrlRequestAndCallback(DataSpec dataSpec) throws IOException {
+    currentUrlRequestCallback = new UrlRequestCallback();
+    currentUrlRequest = buildRequestBuilder(dataSpec).build();
+  }
+
+  /**
+   * Returns {@link UrlRequest.Builder} from dataSpec. Would not work if data source is not opened.
+   */
   @UnstableApi
+  @SideEffectFree
+  @RequiresNonNull("this.currentUrlRequestCallback")
   protected UrlRequest.Builder buildRequestBuilder(DataSpec dataSpec) throws IOException {
     UrlRequest.Builder requestBuilder =
         cronetEngine
-            .newUrlRequestBuilder(dataSpec.uri.toString(), urlRequestCallback, executor)
+            .newUrlRequestBuilder(dataSpec.uri.toString(), currentUrlRequestCallback, executor)
             .setPriority(requestPriority)
             .allowDirectExecutor();
 
@@ -1068,13 +1094,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
     return TextUtils.join(";", setCookieHeaders);
   }
 
-  private static void attachCookies(UrlRequest.Builder requestBuilder, @Nullable String cookies) {
-    if (TextUtils.isEmpty(cookies)) {
-      return;
-    }
-    requestBuilder.addHeader(HttpHeaders.COOKIE, cookies);
-  }
-
   private static int getStatus(UrlRequest request) throws InterruptedException {
     final ConditionVariable conditionVariable = new ConditionVariable();
     final int[] statusHolder = new int[1];
@@ -1107,15 +1126,23 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
     return remaining;
   }
 
-  private final class UrlRequestCallback extends UrlRequest.Callback {
+  @VisibleForTesting
+  /* package */ final class UrlRequestCallback extends UrlRequest.Callback {
+
+    private volatile boolean isClosed = false;
+
+    public void close() {
+      this.isClosed = true;
+    }
 
     @Override
     public synchronized void onRedirectReceived(
         UrlRequest request, UrlResponseInfo info, String newLocationUrl) {
-      if (request != currentUrlRequest) {
+      if (isClosed) {
         return;
       }
-      UrlRequest urlRequest = Assertions.checkNotNull(currentUrlRequest);
+      Assertions.checkNotNull(currentUrlRequest);
+      Assertions.checkNotNull(currentUrlRequestCallback);
       DataSpec dataSpec = Assertions.checkNotNull(currentDataSpec);
       int responseCode = info.getHttpStatusCode();
       if (dataSpec.httpMethod == DataSpec.HTTP_METHOD_POST) {
@@ -1156,7 +1183,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
         return;
       }
 
-      urlRequest.cancel();
       DataSpec redirectUrlDataSpec;
       if (!shouldKeepPost && dataSpec.httpMethod == DataSpec.HTTP_METHOD_POST) {
         // For POST redirects that aren't 307 or 308, the redirect is followed but request is
@@ -1171,21 +1197,29 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
       } else {
         redirectUrlDataSpec = dataSpec.withUri(Uri.parse(newLocationUrl));
       }
-      UrlRequest.Builder requestBuilder;
+
+      if (!TextUtils.isEmpty(cookieHeadersValue)) {
+        Map<String, String> requestHeaders = new HashMap<>();
+        requestHeaders.putAll(dataSpec.httpRequestHeaders);
+        requestHeaders.put(HttpHeaders.COOKIE, cookieHeadersValue);
+        redirectUrlDataSpec =
+            redirectUrlDataSpec.buildUpon().setHttpRequestHeaders(requestHeaders).build();
+      }
+
+      closeCurrentUrlRequestAndCallback();
       try {
-        requestBuilder = buildRequestBuilder(redirectUrlDataSpec);
+        createCurrentUrlRequestAndCallback(redirectUrlDataSpec);
       } catch (IOException e) {
         exception = e;
         return;
       }
-      attachCookies(requestBuilder, cookieHeadersValue);
+
-      currentUrlRequest = requestBuilder.build();
       currentUrlRequest.start();
     }
 
     @Override
     public synchronized void onResponseStarted(UrlRequest request, UrlResponseInfo info) {
-      if (request != currentUrlRequest) {
+      if (isClosed) {
         return;
       }
       responseInfo = info;
@@ -1195,7 +1229,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
     @Override
     public synchronized void onReadCompleted(
         UrlRequest request, UrlResponseInfo info, ByteBuffer buffer) {
-      if (request != currentUrlRequest) {
+      if (isClosed) {
         return;
       }
       operation.open();
@@ -1203,7 +1237,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
 
     @Override
     public synchronized void onSucceeded(UrlRequest request, UrlResponseInfo info) {
-      if (request != currentUrlRequest) {
+      if (isClosed) {
         return;
       }
       finished = true;
@@ -1213,7 +1247,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
     @Override
     public synchronized void onFailed(
         UrlRequest request, UrlResponseInfo info, CronetException error) {
-      if (request != currentUrlRequest) {
+      if (isClosed) {
         return;
       }
       if (error instanceof NetworkException

libraries/datasource_cronet/src/test/java/androidx/media3/datasource/cronet/CronetDataSourceTest.java

@@ -43,6 +43,7 @@ import androidx.media3.datasource.HttpDataSource;
 import androidx.media3.datasource.HttpDataSource.HttpDataSourceException;
 import androidx.media3.datasource.HttpDataSource.InvalidResponseCodeException;
 import androidx.media3.datasource.TransferListener;
+import androidx.media3.datasource.cronet.CronetDataSource.UrlRequestCallback;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
 import java.io.IOException;
 import java.io.InterruptedIOException;
@@ -234,6 +235,7 @@ public final class CronetDataSourceTest {
     mockResponseStartSuccess();
 
     dataSourceUnderTest.open(testDataSpec);
+    UrlRequestCallback previousRequestCallback = dataSourceUnderTest.currentUrlRequestCallback;
     dataSourceUnderTest.close();
     // Prepare a mock UrlRequest to be used in the second open() call.
     final UrlRequest mockUrlRequest2 = mock(UrlRequest.class);
@@ -241,13 +243,13 @@ public final class CronetDataSourceTest {
     doAnswer(
             invocation -> {
               // Invoke the callback for the previous request.
-              dataSourceUnderTest.urlRequestCallback.onFailed(
+              previousRequestCallback.onFailed(
                   mockUrlRequest,
                   testUrlResponseInfo,
                   createNetworkException(
                       /* errorCode= */ Integer.MAX_VALUE,
                       /* cause= */ new IllegalArgumentException()));
-              dataSourceUnderTest.urlRequestCallback.onResponseStarted(
+              dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted(
                   mockUrlRequest2, testUrlResponseInfo);
               return null;
             })
@@ -1094,7 +1096,8 @@ public final class CronetDataSourceTest {
         /* nowMs= */ startTimeMs + TEST_CONNECT_TIMEOUT_MS - 1);
     assertNotCountedDown(openLatch);
     // The response arrives just in time.
-    dataSourceUnderTest.urlRequestCallback.onResponseStarted(mockUrlRequest, testUrlResponseInfo);
+    dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted(
+        mockUrlRequest, testUrlResponseInfo);
     openLatch.await();
     assertThat(exceptionOnTestThread.get()).isNull();
   }
@@ -1130,7 +1133,7 @@ public final class CronetDataSourceTest {
         /* nowMs= */ startTimeMs + TEST_CONNECT_TIMEOUT_MS - 1);
     assertNotCountedDown(timedOutLatch);
     // A redirect arrives just in time.
-    dataSourceUnderTest.urlRequestCallback.onRedirectReceived(
+    dataSourceUnderTest.currentUrlRequestCallback.onRedirectReceived(
         mockUrlRequest, testUrlResponseInfo, "RandomRedirectedUrl1");
 
     long newTimeoutMs = 2 * TEST_CONNECT_TIMEOUT_MS - 1;
@@ -1138,7 +1141,7 @@ public final class CronetDataSourceTest {
     // We should still be trying to open as we approach the new timeout.
     assertNotCountedDown(timedOutLatch);
     // A redirect arrives just in time.
-    dataSourceUnderTest.urlRequestCallback.onRedirectReceived(
+    dataSourceUnderTest.currentUrlRequestCallback.onRedirectReceived(
         mockUrlRequest, testUrlResponseInfo, "RandomRedirectedUrl2");
 
     newTimeoutMs = 3 * TEST_CONNECT_TIMEOUT_MS - 2;
@@ -1223,6 +1226,30 @@ public final class CronetDataSourceTest {
     verify(mockUrlRequest, times(2)).start();
   }
 
+  @Test
+  public void testRedirect_dataSourceHandlesSetCookie_previousRequestIsCanceled()
+      throws HttpDataSourceException {
+    dataSourceUnderTest =
+        (CronetDataSource)
+            new CronetDataSource.Factory(mockCronetEngine, executorService)
+                .setConnectionTimeoutMs(TEST_CONNECT_TIMEOUT_MS)
+                .setReadTimeoutMs(TEST_READ_TIMEOUT_MS)
+                .setResetTimeoutOnRedirects(true)
+                .setHandleSetCookieRequests(true)
+                .createDataSource();
+    dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
+
+    mockSingleRedirectSuccess(/* responseCode= */ 300);
+
+    testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video");
+
+    dataSourceUnderTest.open(testDataSpec);
+
+    verify(mockUrlRequest, never()).followRedirect();
+    verify(mockUrlRequest).cancel();
+    verify(mockUrlRequest, times(2)).start();
+  }
+
   @Test
   public void redirectNoSetCookieFollowsRedirect() throws HttpDataSourceException {
     mockSingleRedirectSuccess(/* responseCode= */ 300);
@@ -1560,7 +1587,7 @@ public final class CronetDataSourceTest {
   private void mockResponseStartSuccess() {
     doAnswer(
             invocation -> {
-              dataSourceUnderTest.urlRequestCallback.onResponseStarted(
+              dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted(
                   mockUrlRequest, testUrlResponseInfo);
               return null;
             })
@@ -1571,7 +1598,7 @@ public final class CronetDataSourceTest {
   private void mockResponseStartRedirect() {
     doAnswer(
             invocation -> {
-              dataSourceUnderTest.urlRequestCallback.onRedirectReceived(
+              dataSourceUnderTest.currentUrlRequestCallback.onRedirectReceived(
                   mockUrlRequest,
                   createUrlResponseInfo(307), // statusCode
                   "http://redirect.location.com");
@@ -1586,12 +1613,12 @@ public final class CronetDataSourceTest {
             invocation -> {
               if (!redirectCalled) {
                 redirectCalled = true;
-                dataSourceUnderTest.urlRequestCallback.onRedirectReceived(
+                dataSourceUnderTest.currentUrlRequestCallback.onRedirectReceived(
                     mockUrlRequest,
                     createUrlResponseInfoWithUrl("http://example.com/video", responseCode),
                     "http://example.com/video/redirect");
               } else {
-                dataSourceUnderTest.urlRequestCallback.onResponseStarted(
+                dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted(
                     mockUrlRequest, testUrlResponseInfo);
               }
               return null;
@@ -1603,7 +1630,7 @@ public final class CronetDataSourceTest {
   private void mockFollowRedirectSuccess() {
     doAnswer(
             invocation -> {
-              dataSourceUnderTest.urlRequestCallback.onResponseStarted(
+              dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted(
                   mockUrlRequest, testUrlResponseInfo);
               return null;
             })
@@ -1614,7 +1641,7 @@ public final class CronetDataSourceTest {
   private void mockResponseStartFailure(int errorCode, Throwable cause) {
     doAnswer(
             invocation -> {
-              dataSourceUnderTest.urlRequestCallback.onFailed(
+              dataSourceUnderTest.currentUrlRequestCallback.onFailed(
                   mockUrlRequest,
                   createUrlResponseInfo(500), // statusCode
                   createNetworkException(errorCode, cause));
@@ -1629,7 +1656,7 @@ public final class CronetDataSourceTest {
     doAnswer(
             invocation -> {
               if (positionAndRemaining[1] == 0) {
-                dataSourceUnderTest.urlRequestCallback.onSucceeded(
+                dataSourceUnderTest.currentUrlRequestCallback.onSucceeded(
                     mockUrlRequest, testUrlResponseInfo);
               } else {
                 ByteBuffer inputBuffer = (ByteBuffer) invocation.getArguments()[0];
@@ -1637,7 +1664,7 @@ public final class CronetDataSourceTest {
                 inputBuffer.put(buildTestDataBuffer(positionAndRemaining[0], readLength));
                 positionAndRemaining[0] += readLength;
                 positionAndRemaining[1] -= readLength;
-                dataSourceUnderTest.urlRequestCallback.onReadCompleted(
+                dataSourceUnderTest.currentUrlRequestCallback.onReadCompleted(
                     mockUrlRequest, testUrlResponseInfo, inputBuffer);
               }
               return null;
@@ -1649,7 +1676,7 @@ public final class CronetDataSourceTest {
   private void mockReadFailure() {
     doAnswer(
             invocation -> {
-              dataSourceUnderTest.urlRequestCallback.onFailed(
+              dataSourceUnderTest.currentUrlRequestCallback.onFailed(
                   mockUrlRequest,
                   createUrlResponseInfo(500), // statusCode
                   createNetworkException(