[remoteobjects] Update RemoteObjectImpl#close() to unreference the object

This CL updates RemoteObjectImpl#close() to unreference the object from
the registry for the case that RemoteObject outlives RemoteObjectHost.

Bug: 1105937
Change-Id: I74f91d0267ade082fc4ecce350bab448c3b37560
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2567022Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Miyoung Shin <myid.shin@igalia.com>
Cr-Commit-Position: refs/heads/master@{#843403}

content/browser/renderer_host/render_frame_host_impl_browsertest.cc

@@ -6492,6 +6492,7 @@ class MockInnerObject : public blink::mojom::RemoteObject {
         kInnerObject.id);
     std::move(callback).Run(std::move(result));
   }
+  void NotifyReleasedObject() override {}
 };
 
 class MockObject : public blink::mojom::RemoteObject {
@@ -6532,6 +6533,8 @@ class MockObject : public blink::mojom::RemoteObject {
     std::move(callback).Run(std::move(result));
   }
 
+  void NotifyReleasedObject() override {}
+
   int get_num_elements_received() const { return num_elements_received_; }
 
  private:

content/public/android/java/src/org/chromium/content/browser/remoteobjects/RemoteObjectImpl.java

@@ -56,6 +56,7 @@ class RemoteObjectImpl implements RemoteObject {
     interface ObjectIdAllocator {
         int getObjectId(Object object, Class<? extends Annotation> safeAnnotationClass);
         Object getObjectById(int id);
+        void unrefObjectByObject(Object object);
     }
 
     /**
@@ -109,6 +110,8 @@ class RemoteObjectImpl implements RemoteObject {
      */
     private final boolean mAllowInspection;
 
+    private boolean mNotifiedReleasedObject;
+
     public RemoteObjectImpl(Object target, Class<? extends Annotation> safeAnnotationClass,
             Auditor auditor, ObjectIdAllocator objectIdAllocator, boolean allowInspection) {
         mTarget = new WeakReference<>(target);
@@ -116,6 +119,7 @@ class RemoteObjectImpl implements RemoteObject {
         mAuditor = auditor;
         mObjectIdAllocator = new WeakReference<>(objectIdAllocator);
         mAllowInspection = allowInspection;
+        mNotifiedReleasedObject = false;
 
         for (Method method : target.getClass().getMethods()) {
             if (safeAnnotationClass != null && !method.isAnnotationPresent(safeAnnotationClass)) {
@@ -215,8 +219,22 @@ class RemoteObjectImpl implements RemoteObject {
         callback.call(mojoResult);
     }
 
+    @Override
+    public void notifyReleasedObject() {
+        mNotifiedReleasedObject = true;
+    }
+
     @Override
     public void close() {
+        Object target = mTarget.get();
+        ObjectIdAllocator objectIdAllocator = mObjectIdAllocator.get();
+        // If |mNotifiedReleasedObject| is false, this outlives the RemoteObjectHost and the object
+        // is not unreferenced by the RemoteObjectHost. So, we should unreference the object when
+        // the mojo pipe is closed.
+        if (target != null && objectIdAllocator != null && !mNotifiedReleasedObject) {
+            objectIdAllocator.unrefObjectByObject(target);
+        }
+
         mTarget.clear();
     }
 

content/public/android/java/src/org/chromium/content/browser/remoteobjects/RemoteObjectRegistry.java

@@ -16,6 +16,17 @@ import java.util.Set;
  *
  * These objects could contain references which would keep the WebContents alive
  * longer than expected, and so must not be held alive by any other GC root.
+ *
+ * The object's reference count is changed in the following cases:
+ *   - When the wrapper object is created in the renderer, it is increased.
+ *   - When the wrapper object is destroyed in the renderer, it is decreased via RemoteObjectHost
+ *     in general. If the wrapper object, RemoteObject, outlives RemoteObjectHost, the object could
+ *     not drop the reference via RemoteObjectHost. By explicitly notifying {@link RemoteObjectImpl}
+ *     that the object has been released, it ensures that the object's reference is released by
+ *     {@link RemoteObjectImpl} when the Mojo pipe is closed.
+ *   - When the object is named in Java code, it is increased.
+ *   - When the object is removed from the named objects in Java code, it is decreased.
+ *
  */
 final class RemoteObjectRegistry implements RemoteObjectImpl.ObjectIdAllocator {
     private final Set<? super RemoteObjectRegistry> mRetainingSet;
@@ -86,6 +97,11 @@ final class RemoteObjectRegistry implements RemoteObjectImpl.ObjectIdAllocator {
         return entry != null ? entry.object : null;
     }
 
+    @Override
+    public synchronized void unrefObjectByObject(Object object) {
+        unrefObject(mEntriesByObject.get(object));
+    }
+
     public synchronized void refObjectById(int id) {
         Entry entry = mEntriesById.get(id);
         if (entry == null) return;
@@ -97,10 +113,6 @@ final class RemoteObjectRegistry implements RemoteObjectImpl.ObjectIdAllocator {
         unrefObject(mEntriesById.get(id));
     }
 
-    public synchronized void unrefObjectByObject(Object object) {
-        unrefObject(mEntriesByObject.get(object));
-    }
-
     private synchronized void unrefObject(Entry entry) {
         if (entry == null) return;
 

content/public/android/javatests/src/org/chromium/content/browser/remoteobjects/RemoteObjectHostImplTest.java

@@ -160,4 +160,27 @@ public final class RemoteObjectHostImplTest {
         host.close();
         Assert.assertThat(mRegistry, not(isIn(mRetainingSet)));
     }
+
+    @Test
+    @SmallTest
+    @Feature({"AndroidWebView", "Android-JavaBridge"})
+    public void testClosePipeAfterHostClosesWithoutRelease() {
+        Object o = new Object();
+        int id = mRegistry.getObjectId(o, TestJavascriptInterface.class);
+
+        RemoteObjectHostImpl host = new RemoteObjectHostImpl(
+                /* auditor */ null, mRegistry, /* allowInspection */ true);
+
+        Pair<RemoteObject.Proxy, InterfaceRequest<RemoteObject>> result =
+                RemoteObject.MANAGER.getInterfaceRequest(CoreImpl.getInstance());
+        RemoteObject.Proxy remoteObject = result.first;
+        host.getObject(id, result.second);
+        host.close();
+
+        Assert.assertSame(o, mRegistry.getObjectById(id));
+        remoteObject.close();
+
+        mMojoTestRule.runLoopUntilIdle();
+        Assert.assertNull(mRegistry.getObjectById(id));
+    }
 }

third_party/blink/public/mojom/remote_objects/remote_objects.mojom

@@ -37,6 +37,10 @@ interface RemoteObject {
   [Sync] GetMethods() => (array<string> method_names);
   [Sync] InvokeMethod(string name, array<RemoteInvocationArgument> arguments)
       => (RemoteInvocationResult result);
+
+  // Notifies ReleaseObject is called by RemoteObjectHost interface to ensure
+  // the object's unreference when the pipe is closed.
+  NotifyReleasedObject();
 };
 
 // Implemented in the renderer.

third_party/blink/renderer/modules/remote_objects/remote_object.cc

@@ -167,11 +167,12 @@ RemoteObject::RemoteObject(v8::Isolate* isolate,
       object_id_(object_id) {}
 
 RemoteObject::~RemoteObject() {
-  // TODO(https://crbug.com/794320): if |this| outlives |gateway_|, the
-  // browser (RemoteObjectImpl.java) needs to handle object ID invalidation when
-  // the RemoteObject mojo pipe is closed.
-  if (gateway_)
+  if (gateway_) {
     gateway_->ReleaseObject(object_id_);
+
+    if (object_)
+      object_->NotifyReleasedObject();
+  }
 }
 
 gin::ObjectTemplateBuilder RemoteObject::GetObjectTemplateBuilder(