From 128324540fc2f19fb6472aa3f87d9f71c87210b9 Mon Sep 17 00:00:00 2001
From: John Cormie <jdcormie@google.com>
Date: Mon, 24 Jan 2022 19:54:21 -0800
Subject: [PATCH] binder: Fix a ServiceConnection leak (#8861)

Closes #8726
---
 .../io/grpc/binder/internal/ServiceBinding.java    | 14 ++++++++++++++
 .../grpc/binder/internal/ServiceBindingTest.java   |  9 ++++++++-
 2 files changed, 22 insertions(+), 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
index 11480b36a3..650ead9bcd 100644
--- 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 @@ final class ServiceBinding implements Bindable, ServiceConnection {
       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 @@ final class ServiceBinding implements Bindable, ServiceConnection {
     }
   }
 
+  // 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
index 44a3898e53..967e91b820 100644
--- 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 final class ServiceBindingTest {
     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 final class ServiceBindingTest {
     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 final class ServiceBindingTest {
     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 final class ServiceBindingTest {
     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 @@ public final class ServiceBindingTest {
     } 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);
     }
   }