binder: Fix a ServiceConnection leak (#8861)

Closes #8726
grpc · Jan 25, 2022 · 1283245 · 1283245
1 parent b29c3ec
commit 1283245
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 1 deletion.
diff --git a/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java b/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java
@@ -119,6 +119,7 @@ public synchronized void bind() {
       state = State.BINDING;
       Status bindResult = bindInternal(sourceContext, bindIntent, this, bindFlags);
       if (!bindResult.isOk()) {
+        handleBindServiceFailure(sourceContext, this);
         state = State.UNBOUND;
         mainThreadExecutor.execute(() -> notifyUnbound(bindResult));
       }
@@ -142,6 +143,19 @@ private static Status bindInternal(
     }
   }
 
+  // Over the years, the API contract for Context#bindService() has been inconsistent on the subject
+  // of error handling. But inspecting recent AOSP implementations shows that, internally,
+  // bindService() retains a reference to the ServiceConnection when it throws certain Exceptions
+  // and even when it returns false. To avoid leaks, we *always* call unbindService() in case of
+  // error and simply ignore any "Service not registered" IAE and other RuntimeExceptions.
+  private static void handleBindServiceFailure(Context context, ServiceConnection conn) {
+    try {
+      context.unbindService(conn);
+    } catch (RuntimeException e) {
+      logger.log(Level.FINE, "Could not clean up after bindService() failure.", e);
+    }
+  }
+
   @Override
   @AnyThread
   public void unbind() {

diff --git a/binder/src/test/java/io/grpc/binder/internal/ServiceBindingTest.java b/binder/src/test/java/io/grpc/binder/internal/ServiceBindingTest.java
@@ -68,6 +68,9 @@ public void setUp() {
     shadowApplication = shadowOf(appContext);
     shadowApplication.setComponentNameAndServiceForBindService(serviceComponent, mockBinder);
 
+    // Don't call onServiceDisconnected() upon unbindService(), just like the real Android doesn't.
+    shadowApplication.setUnbindServiceCallsOnServiceDisconnected(false);
+
     binding = newBuilder().build();
     shadowOf(getMainLooper()).idle();
   }
@@ -137,6 +140,7 @@ public void testBindUnbind() throws Exception {
     assertThat(observer.gotUnboundEvent).isTrue();
     assertThat(observer.unboundReason.getCode()).isEqualTo(Code.CANCELLED);
     assertThat(binding.isSourceContextCleared()).isTrue();
+    assertThat(shadowApplication.getBoundServiceConnections()).isEmpty();
   }
 
   @Test
@@ -174,6 +178,7 @@ public void testBindFailure() throws Exception {
     assertThat(observer.gotUnboundEvent).isTrue();
     assertThat(observer.unboundReason.getCode()).isEqualTo(Code.UNIMPLEMENTED);
     assertThat(binding.isSourceContextCleared()).isTrue();
+    assertThat(shadowApplication.getBoundServiceConnections()).isEmpty();
   }
 
   @Test
@@ -187,6 +192,7 @@ public void testBindSecurityException() throws Exception {
     assertThat(observer.unboundReason.getCode()).isEqualTo(Code.PERMISSION_DENIED);
     assertThat(observer.unboundReason.getCause()).isEqualTo(securityException);
     assertThat(binding.isSourceContextCleared()).isTrue();
+    assertThat(shadowApplication.getBoundServiceConnections()).isEmpty();
   }
 
   @Test
@@ -257,7 +263,8 @@ private void assertNoLockHeld() {
     } catch (IllegalMonitorStateException ime) {
       // Expected.
     } catch (InterruptedException inte) {
-      throw new AssertionError("Interrupted exception when we shouldn't have been able to wait.", inte);
+      throw new AssertionError(
+          "Interrupted exception when we shouldn't have been able to wait.", inte);
     }
   }