binder: oneway txns cannot arrive out of order (grpc#10754)

See https://developer.android.com/reference/android/os/IBinder#FLAG_ONEWAY

binder/src/androidTest/java/io/grpc/binder/internal/LeakSafeOneWayBinderTest.java

@@ -16,6 +16,7 @@
 
 package io.grpc.binder.internal;
 
+import static android.os.IBinder.FLAG_ONEWAY;
 import static com.google.common.truth.Truth.assertThat;
 
 import android.os.Parcel;
@@ -46,18 +47,28 @@ public void setUp() {
   @Test
   public void testTransaction() {
     Parcel p = Parcel.obtain();
-    assertThat(binder.onTransact(123, p, null, 0)).isTrue();
+    assertThat(binder.onTransact(123, p, null, FLAG_ONEWAY)).isTrue();
     assertThat(transactionsHandled).isEqualTo(1);
     assertThat(lastCode).isEqualTo(123);
     assertThat(lastParcel).isSameInstanceAs(p);
     p.recycle();
   }
 
+  @Test
+  public void testDropsTwoWayTransactions() {
+    Parcel p = Parcel.obtain();
+    Parcel reply = Parcel.obtain();
+    assertThat(binder.onTransact(123, p, reply, 0)).isFalse();
+    assertThat(transactionsHandled).isEqualTo(0);
+    p.recycle();
+    reply.recycle();
+  }
+
   @Test
   public void testDetach() {
     Parcel p = Parcel.obtain();
     binder.detach();
-    assertThat(binder.onTransact(456, p, null, 0)).isFalse();
+    assertThat(binder.onTransact(456, p, null, FLAG_ONEWAY)).isFalse();
 
     // The transaction shouldn't have been processed.
     assertThat(transactionsHandled).isEqualTo(0);
@@ -68,8 +79,8 @@ public void testDetach() {
   @Test
   public void testMultipleTransactions() {
     Parcel p = Parcel.obtain();
-    assertThat(binder.onTransact(123, p, null, 0)).isTrue();
-    assertThat(binder.onTransact(456, p, null, 0)).isTrue();
+    assertThat(binder.onTransact(123, p, null, FLAG_ONEWAY)).isTrue();
+    assertThat(binder.onTransact(456, p, null, FLAG_ONEWAY)).isTrue();
     assertThat(transactionsHandled).isEqualTo(2);
     assertThat(lastCode).isEqualTo(456);
     assertThat(lastParcel).isSameInstanceAs(p);

binder/src/main/java/io/grpc/binder/internal/BinderTransport.java

@@ -31,6 +31,7 @@
 import android.os.UserHandle;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Ticker;
+import com.google.common.base.Verify;
 import com.google.common.util.concurrent.ListenableFuture;
 import io.grpc.Attributes;
 import io.grpc.CallOptions;
@@ -463,14 +464,11 @@ private boolean handleTransactionInternal(int code, Parcel parcel) {
       if (inbound == null) {
         synchronized (this) {
           if (!isShutdown()) {
-            // Create a new inbound. Strictly speaking we could end up doing this twice on
-            // two threads, hence the need to use putIfAbsent, and check its result.
             inbound = createInbound(code);
             if (inbound != null) {
-              Inbound<?> inbound2 = ongoingCalls.putIfAbsent(code, inbound);
-              if (inbound2 != null) {
-                inbound = inbound2;
-              }
+              Inbound<?> existing = ongoingCalls.put(code, inbound);
+              // Can't happen as only one invocation of handleTransaction() is running at a time.
+              Verify.verify(existing == null, "impossible appearance of %s", existing);
             }
           }
         }

binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java

@@ -17,6 +17,7 @@
 package io.grpc.binder.internal;
 
 import android.os.Binder;
+import android.os.IBinder;
 import android.os.Parcel;
 import io.grpc.Internal;
 import java.util.logging.Level;
@@ -42,6 +43,21 @@ public final class LeakSafeOneWayBinder extends Binder {
 
   @Internal
   public interface TransactionHandler {
+    /**
+     * Delivers a binder transaction to this handler.
+     *
+     * <p>Implementations need not be thread-safe. Each invocation "happens-before" the next in the
+     * same order that transactions were sent ("oneway" semantics). However implementations must not
+     * be thread-hostile as different calls can come in on different threads.
+     *
+     * <p>{@code parcel} is only valid for the duration of this call. Ownership is retained by the
+     * caller.
+     *
+     * @param code the transaction code originally passed to {@link IBinder#transact}
+     * @param code a copy of the parcel originally passed to {@link IBinder#transact}.
+     * @return the value to return from {@link Binder#onTransact}. NB: "oneway" semantics mean this
+     *     result will not delivered to the caller of {@link IBinder#transact}
+     */
     boolean handleTransaction(int code, Parcel data);
   }
 
@@ -60,6 +76,10 @@ protected boolean onTransact(int code, Parcel parcel, Parcel reply, int flags) {
     TransactionHandler handler = this.handler;
     if (handler != null) {
       try {
+        if ((flags & IBinder.FLAG_ONEWAY) == 0) {
+          logger.log(Level.WARNING, "ignoring non-oneway transaction. flags=" + flags);
+          return false;
+        }
         return handler.handleTransaction(code, parcel);
       } catch (RuntimeException re) {
         logger.log(Level.WARNING, "failure sending transaction " + code, re);