aw: Destroy ContentViewCore and WebContents together

Even though it's having WebContents without a ContentViewCore
is a support scenario in //content, it's not a well tested
path, especially the webview only code. This makes sure
ContentViewCore and WebContents are destroyed together.

Instead of posting on native side to destroy WebContents,
post on Java side and have one call destroy both Java and
native side.

More specific details:
* All caller except destroy to AwContents.destroyNatives
  either have already detached webview so ok to move detach
  to destroy.
* Need to remove some asserts that objects are null when
  mIsDestroyed is true.
* Reordered some native AwContents members so they are
  naturally destroyed in the correct order.

BUG=

Review URL: https://codereview.chromium.org/690553002

Cr-Commit-Position: refs/heads/master@{#302513}

android_webview/browser/aw_contents_client_bridge_base.cc

@@ -47,11 +47,6 @@ void AwContentsClientBridgeBase::Associate(
                             new UserData(handler));
 }
 
-void AwContentsClientBridgeBase::Disassociate(
-  WebContents* web_contents) {
-  web_contents->RemoveUserData(kAwContentsClientBridgeBase);
-}
-
 // static
 AwContentsClientBridgeBase* AwContentsClientBridgeBase::FromWebContents(
     WebContents* web_contents) {

android_webview/browser/aw_contents_client_bridge_base.h

@@ -34,7 +34,6 @@ class AwContentsClientBridgeBase {
   // Adds the handler to the UserData registry.
   static void Associate(content::WebContents* web_contents,
                         AwContentsClientBridgeBase* handler);
-  static void Disassociate(content::WebContents* web_contents);
   static AwContentsClientBridgeBase* FromWebContents(
       content::WebContents* web_contents);
   static AwContentsClientBridgeBase* FromID(int render_process_id,

android_webview/java/src/org/chromium/android_webview/AwContents.java

@@ -20,6 +20,7 @@ import android.net.http.SslCertificate;
 import android.os.AsyncTask;
 import android.os.Build;
 import android.os.Bundle;
+import android.os.Handler;
 import android.os.Message;
 import android.text.TextUtils;
 import android.util.Log;
@@ -889,8 +890,19 @@ public class AwContents {
      * Destroys this object and deletes its native counterpart.
      */
     public void destroy() {
+        if (isDestroyed()) return;
+        // If we are attached, we have to call native detach to clean up
+        // hardware resources.
+        if (mIsAttachedToWindow) {
+            nativeOnDetachedFromWindow(mNativeAwContents);
+        }
         mIsDestroyed = true;
-        destroyNatives();
+        new Handler().post(new Runnable() {
+            @Override
+            public void run() {
+                destroyNatives();
+            }
+        });
     }
 
     /**
@@ -899,11 +911,6 @@ public class AwContents {
     private void destroyNatives() {
         if (mCleanupReference != null) {
             assert mNativeAwContents != 0;
-            // If we are attached, we have to call native detach to clean up
-            // hardware resources.
-            if (mIsAttachedToWindow) {
-                nativeOnDetachedFromWindow(mNativeAwContents);
-            }
 
             mWebContentsObserver.detachFromWebContents();
             mWebContentsObserver = null;
@@ -924,12 +931,7 @@ public class AwContents {
     }
 
     private boolean isDestroyed() {
-        if (mIsDestroyed) {
-            assert mContentViewCore == null;
-            assert mWebContents == null;
-            assert mNavigationController == null;
-            assert mNativeAwContents == 0;
-        } else {
+        if (!mIsDestroyed) {
             assert mContentViewCore != null;
             assert mWebContents != null;
             assert mNavigationController != null;

android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java

@@ -88,15 +88,15 @@ public class AwContentsClientShouldOverrideUrlLoadingTest extends AwTestBase {
     }
 
     private String getHtmlForPageWithJsRedirectTo(String url, String method, int timeout) {
-        return makeHtmlPageFrom(
-                "<script>" +
-                  "function doRedirectAssign() {" +
-                    "location.href = '" + url + "';" +
-                  "} " +
-                  "function doRedirectReplace() {" +
-                    "location.replace('" + url + "');" +
-                  "} " +
-                "</script>",
+        return makeHtmlPageFrom(""
+                + "<script>"
+                +   "function doRedirectAssign() {"
+                +     "location.href = '" + url + "';"
+                +   "} "
+                +   "function doRedirectReplace() {"
+                +     "location.replace('" + url + "');"
+                +   "} "
+                + "</script>",
                 String.format("<iframe onLoad=\"setTimeout('doRedirect%s()', %d);\" />",
                     method, timeout));
     }
@@ -500,9 +500,9 @@ public class AwContentsClientShouldOverrideUrlLoadingTest extends AwTestBase {
     @Feature({"AndroidWebView", "Navigation"})
     public void testCalledForDataUrl() throws Throwable {
         final String dataUrl =
-                "data:text/html;base64," +
-                "PGh0bWw+PGhlYWQ+PHRpdGxlPmRhdGFVcmxUZXN0QmFzZTY0PC90aXRsZT48" +
-                "L2hlYWQ+PC9odG1sPg==";
+                "data:text/html;base64,"
+                        + "PGh0bWw+PGhlYWQ+PHRpdGxlPmRhdGFVcmxUZXN0QmFzZTY0PC90aXRsZT48"
+                        + "L2hlYWQ+PC9odG1sPg==";
         final TestAwContentsClient contentsClient = new TestAwContentsClient();
         final AwTestContainerView testContainerView =
                 createAwTestContainerViewOnMainSync(contentsClient);
@@ -516,8 +516,8 @@ public class AwContentsClientShouldOverrideUrlLoadingTest extends AwTestBase {
         clickOnLinkUsingJs(awContents, contentsClient);
 
         shouldOverrideUrlLoadingHelper.waitForCallback(callCount);
-        assertTrue("Expected URL that starts with 'data:' but got: <" +
-                   shouldOverrideUrlLoadingHelper.getShouldOverrideUrlLoadingUrl() + "> instead.",
+        assertTrue("Expected URL that starts with 'data:' but got: <"
+                   + shouldOverrideUrlLoadingHelper.getShouldOverrideUrlLoadingUrl() + "> instead.",
                    shouldOverrideUrlLoadingHelper.getShouldOverrideUrlLoadingUrl().startsWith(
                            "data:"));
     }
@@ -816,4 +816,48 @@ public class AwContentsClientShouldOverrideUrlLoadingTest extends AwTestBase {
 
         assertEquals(0, shouldOverrideUrlLoadingHelper.getCallCount());
     }
+
+    @SmallTest
+    @Feature({"AndroidWebView"})
+    public void testCallDestroyInCallback() throws Throwable {
+        class DestroyInCallbackClient extends TestAwContentsClient {
+            private AwContents mAwContents;
+
+            public void setAwContents(AwContents awContents) {
+                mAwContents = awContents;
+            }
+
+            @Override
+            public boolean shouldOverrideUrlLoading(String url) {
+                mAwContents.destroy();
+                return super.shouldOverrideUrlLoading(url);
+            }
+        }
+
+        DestroyInCallbackClient contentsClient = new DestroyInCallbackClient();
+        AwTestContainerView testContainerView =
+                createAwTestContainerViewOnMainSync(contentsClient);
+        AwContents awContents = testContainerView.getAwContents();
+        contentsClient.setAwContents(awContents);
+
+        OnReceivedErrorHelper onReceivedErrorHelper = contentsClient.getOnReceivedErrorHelper();
+        int onReceivedErrorCallCount = onReceivedErrorHelper.getCallCount();
+
+        loadDataSync(awContents, contentsClient.getOnPageFinishedHelper(),
+                CommonResources.makeHtmlPageWithSimpleLinkTo(DATA_URL), "text/html", false);
+
+        TestAwContentsClient.ShouldOverrideUrlLoadingHelper shouldOverrideUrlLoadingHelper =
+                contentsClient.getShouldOverrideUrlLoadingHelper();
+        int shouldOverrideUrlLoadingCallCount = shouldOverrideUrlLoadingHelper.getCallCount();
+        setShouldOverrideUrlLoadingReturnValueOnUiThread(shouldOverrideUrlLoadingHelper, true);
+        clickOnLinkUsingJs(awContents, contentsClient);
+        shouldOverrideUrlLoadingHelper.waitForCallback(shouldOverrideUrlLoadingCallCount);
+
+        pollOnUiThread(new Callable<Boolean>() {
+            @Override
+            public Boolean call() {
+                return AwContents.getNativeInstanceCount() == 0;
+            }
+        });
+    }
 }

android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java

@@ -103,10 +103,6 @@ public class AwContentsTest extends AwTestBase {
                 createAwTestContainerView(mContentsClient).getAwContents();
         awContents.destroy();
 
-        assertNull(awContents.getWebContents());
-        assertNull(awContents.getContentViewCore());
-        assertNull(awContents.getNavigationController());
-
         // The documentation for WebView#destroy() reads "This method should be called
         // after this WebView has been removed from the view system. No other methods
         // may be called on this WebView after destroy".
@@ -342,8 +338,8 @@ public class AwContentsTest extends AwTestBase {
             pollOnUiThread(new Callable<Boolean>() {
                 @Override
                 public Boolean call() {
-                    return awContents.getFavicon() != null &&
-                        !awContents.getFavicon().sameAs(defaultFavicon);
+                    return awContents.getFavicon() != null
+                            && !awContents.getFavicon().sameAs(defaultFavicon);
                 }
             });
 

android_webview/native/aw_contents.cc

@@ -286,22 +286,7 @@ jlong AwContents::GetWebContents(JNIEnv* env, jobject obj) {
 
 void AwContents::Destroy(JNIEnv* env, jobject obj) {
   java_ref_.reset();
-
-  // We clear the contents_client_bridge_ here so that we break the link with
-  // the java peer. This is important for the popup window case, where we are
-  // swapping AwContents out that share the same java AwContentsClientBridge.
-  // See b/15074651.
-  AwContentsClientBridgeBase::Disassociate(web_contents_.get());
-  contents_client_bridge_.reset();
-
-  // Do not wait until the WebContents are deleted asynchronously to clear
-  // the delegate and stop sending callbacks.
-  web_contents_->SetDelegate(NULL);
-
-  // We do not delete AwContents immediately. Some applications try to delete
-  // Webview in ShouldOverrideUrlLoading callback, which is a sync IPC from
-  // Webkit.
-  BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
+  delete this;
 }
 
 static jlong Init(JNIEnv* env, jclass, jobject browser_context) {

android_webview/native/aw_contents.h

@@ -233,9 +233,9 @@ class AwContents : public FindHelper::Listener,
   void HideGeolocationPrompt(const GURL& origin);
 
   JavaObjectWeakGlobalRef java_ref_;
-  scoped_ptr<content::WebContents> web_contents_;
   scoped_ptr<AwWebContentsDelegate> web_contents_delegate_;
   scoped_ptr<AwContentsClientBridge> contents_client_bridge_;
+  scoped_ptr<content::WebContents> web_contents_;
   scoped_ptr<AwRenderViewHostExt> render_view_host_ext_;
   scoped_ptr<FindHelper> find_helper_;
   scoped_ptr<IconHelper> icon_helper_;