Call dismiss() on overlay Dialog for onSurfaceDestroyed.

Previously, DialogOverlayCore didn't receive a release() call from
DSImpl, since DSImpl drops the reference to DSCore immediately
after notifying the client of surface destruction.  This caused
problems on some cast devices, where the overlay wasn't being
deleted properly.

Now, DSCore calls release() synchronously after waiting for
DSImpl to close the overlay.  This keeps the dismiss() call sync
with respect to onSurfaceDestroyed.  Even if DSImpl did call
DSCore.release(), it would be after onSurfaceDestroyed completed.

Bug: 782282
Change-Id: I25f98643063545e0a44ccb7ab04145e7c6728d3a
Reviewed-on: https://chromium-review.googlesource.com/756932
Commit-Queue: Frank Liberato <liberato@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515234}

content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java

@@ -35,13 +35,13 @@ class DialogOverlayCore {
         // Notify the host that we have failed to get a surface or the surface was destroyed.
         void onOverlayDestroyed();
 
-        // Wait until the host has been told to clean up.  We are allowed to let surfaceDestroyed
+        // Wait until the host has been told to close.  We are allowed to let surfaceDestroyed
         // proceed once this happens.
-        void waitForCleanup();
+        void waitForClose();
 
-        // Notify the host that waitForCleanup() is done waiting, so that it can enforce that
-        // cleanup happens.
-        void enforceCleanup();
+        // Notify the host that waitForClose() is done waiting, so that it can enforce that cleanup
+        // always happens.
+        void enforceClose();
     }
 
     private Host mHost;
@@ -88,7 +88,7 @@ class DialogOverlayCore {
 
     /**
      * Release the underlying surface, and generally clean up, in response to
-     * the client releasing the AndroidOverlay.
+     * the client releasing the AndroidOverlay.  This may be called more than once.
      */
     public void release() {
         // If we've not released the dialog yet, then do so.
@@ -146,8 +146,8 @@ class DialogOverlayCore {
 
             // Notify the host that we've been destroyed, and wait for it to clean up or time out.
             mHost.onOverlayDestroyed();
-            mHost.waitForCleanup();
-            mHost.enforceCleanup();
+            mHost.waitForClose();
+            mHost.enforceClose();
             mHost = null;
         }
 

content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java

@@ -33,6 +33,10 @@ public class DialogOverlayImpl implements AndroidOverlay, DialogOverlayCore.Host
     // Runnable that we'll run when the overlay notifies us that it's been released.
     private Runnable mReleasedRunnable;
 
+    // Runnable that will release |mDialogCore| when posted to mOverlayHandler.  We keep this
+    // separately from mDialogCore itself so that we can call it after we've discarded the latter.
+    private Runnable mReleaseCoreRunnable;
+
     private final ThreadHoppingHost mHoppingHost;
 
     private DialogOverlayCore mDialogCore;
@@ -97,6 +101,13 @@ public class DialogOverlayImpl implements AndroidOverlay, DialogOverlayCore.Host
                 });
             }
         });
+
+        mReleaseCoreRunnable = new Runnable() {
+            @Override
+            public void run() {
+                dialogCore.release();
+            }
+        };
     }
 
     // AndroidOverlay impl.
@@ -113,19 +124,12 @@ public class DialogOverlayImpl implements AndroidOverlay, DialogOverlayCore.Host
         // that the client calls it.
 
         // Allow surfaceDestroyed to proceed, if it's waiting.
-        mHoppingHost.onCleanup();
+        mHoppingHost.onClose();
 
-        // Notify |mDialogCore| that it has been released.  This might not be called if it notifies
-        // us that it's been destroyed.  We still might send it in that case if the client closes
-        // the connection before we find out that it's been destroyed on the overlay thread.
-        if (mDialogCore != null) {
-            final DialogOverlayCore dialogCore = mDialogCore;
-            mOverlayHandler.post(new Runnable() {
-                @Override
-                public void run() {
-                    dialogCore.release();
-                }
-            });
+        // Notify |mDialogCore| that it has been released.
+        if (mReleaseCoreRunnable != null) {
+            mOverlayHandler.post(mReleaseCoreRunnable);
+            mReleaseCoreRunnable = null;
 
             // Note that we might get messagaes from |mDialogCore| after this, since they might be
             // dispatched before |r| arrives.  Clearing |mDialogCore| causes us to ignore them.
@@ -208,13 +212,13 @@ public class DialogOverlayImpl implements AndroidOverlay, DialogOverlayCore.Host
     // DialogOverlayCore.Host impl.
     // Due to threading issues, |mHoppingHost| doesn't forward this.
     @Override
-    public void waitForCleanup() {
+    public void waitForClose() {
         assert false : "Not reached";
     }
 
     // DialogOverlayCore.Host impl
     @Override
-    public void enforceCleanup() {
+    public void enforceClose() {
         // Pretend that the client closed us, even if they didn't.  It's okay if this is called more
         // than once.  The client might have already called it, or might call it later.
         close();

content/public/android/java/src/org/chromium/content/browser/androidoverlay/ThreadHoppingHost.java

@@ -14,7 +14,7 @@ import java.util.concurrent.TimeUnit;
 
 /**
  * DialogOverlayCore::Host implementation that transfers messages to the thread on which it was
- * constructed.  Due to threading concerns, waitForCleanup is not forwarded.
+ * constructed.  Due to threading concerns, waitForClose is not forwarded.
  */
 class ThreadHoppingHost implements DialogOverlayCore.Host {
     private static final String TAG = "ThreadHoppingHost";
@@ -62,7 +62,7 @@ class ThreadHoppingHost implements DialogOverlayCore.Host {
     // thread would block.  Instead, we wait here and somebody must call onCleanup() to let us know
     // that cleanup has started, and that we may return.
     @Override
-    public void waitForCleanup() {
+    public void waitForClose() {
         while (true) {
             try {
                 // TODO(liberato): in case of InterruptedException, we really should adjust the
@@ -76,17 +76,17 @@ class ThreadHoppingHost implements DialogOverlayCore.Host {
     }
 
     @Override
-    public void enforceCleanup() {
+    public void enforceClose() {
         mHandler.post(new Runnable() {
             @Override
             public void run() {
-                mHost.enforceCleanup();
+                mHost.enforceClose();
             }
         });
     }
 
     // Notify us that cleanup has started.  This is called on |mHandler|'s thread.
-    public void onCleanup() {
+    public void onClose() {
         mSemaphore.release(1);
     }
 }

content/public/android/junit/src/org/chromium/content/browser/androidoverlay/DialogOverlayCoreTest.java

@@ -131,8 +131,8 @@ public class DialogOverlayCoreTest {
     void checkOverlayDidntCall() {
         assertEquals(null, mHost.surface());
         assertEquals(0, mHost.destroyedCount());
-        assertEquals(0, mHost.waitCleanupCount());
-        assertEquals(0, mHost.enforceCleanupCount());
+        assertEquals(0, mHost.waitCloseCount());
+        assertEquals(0, mHost.enforceCloseCount());
     }
 
     // Return the SurfaceHolder callback that was provided to takeSurface(), if any.
@@ -151,8 +151,8 @@ public class DialogOverlayCoreTest {
     class HostMock implements DialogOverlayCore.Host {
         private Surface mSurface;
         private int mDestroyedCount;
-        private int mWaitCleanupCount;
-        private int mEnforceCleanupCount;
+        private int mWaitCloseCount;
+        private int mEnforceCloseCount;
 
         @Override
         public void onSurfaceReady(Surface surface) {
@@ -165,13 +165,13 @@ public class DialogOverlayCoreTest {
         }
 
         @Override
-        public void waitForCleanup() {
-            mWaitCleanupCount++;
+        public void waitForClose() {
+            mWaitCloseCount++;
         }
 
         @Override
-        public void enforceCleanup() {
-            mEnforceCleanupCount++;
+        public void enforceClose() {
+            mEnforceCloseCount++;
         }
 
         public Surface surface() {
@@ -182,12 +182,12 @@ public class DialogOverlayCoreTest {
             return mDestroyedCount;
         }
 
-        public int waitCleanupCount() {
-            return mWaitCleanupCount;
+        public int waitCloseCount() {
+            return mWaitCloseCount;
         }
 
-        public int enforceCleanupCount() {
-            return mEnforceCleanupCount;
+        public int enforceCloseCount() {
+            return mEnforceCloseCount;
         }
     };
 
@@ -264,8 +264,8 @@ public class DialogOverlayCoreTest {
 
         mCore.release();
         assertEquals(0, mHost.destroyedCount());
-        assertEquals(0, mHost.waitCleanupCount());
-        assertEquals(0, mHost.enforceCleanupCount());
+        assertEquals(0, mHost.waitCloseCount());
+        assertEquals(0, mHost.enforceCloseCount());
         checkDialogIsNotShown();
     }
 
@@ -279,11 +279,11 @@ public class DialogOverlayCoreTest {
         // Destroy the surface.
         holderCallback().surfaceDestroyed(mHolder);
         // |mCore| should have waited for cleanup during surfaceDestroyed.
-        assertEquals(1, mHost.waitCleanupCount());
+        assertEquals(1, mHost.waitCloseCount());
         // Since we waited for cleanup, also pretend that the release was posted during the wait and
         // will arrive after the wait completes.
         mCore.release();
-        assertEquals(1, mHost.enforceCleanupCount());
+        assertEquals(1, mHost.enforceCloseCount());
 
         checkOverlayWasDestroyed();
     }