aw: Destroy ContentViewCore and WebContents together

android_webview/native/aw_contents.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "android_webview/native/aw_contents.h"
 
 #include <limits>
 
 #include "android_webview/browser/aw_browser_context.h"
 #include "android_webview/browser/aw_browser_main_parts.h"
@@ skipping 270 matching lines @@
 }
 
 jlong AwContents::GetWebContents(JNIEnv* env, jobject obj) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(web_contents_);
   return reinterpret_cast<intptr_t>(web_contents_.get());
 }
 
 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.

[benm (inactive), 2014/10/29 14:56:15]

Can you check the repro case from this internal bug is still fixed?

[boliu, 2014/10/29 15:14:33]

Will do. I'll let you the result afterwards.

[boliu, 2014/10/29 18:51:59]

Not conclusive :(

Nothing appeared wrong when I followed the steps in b/15074651. But then I
reverted this change, and then commented out these two lines and still no
issues, which means I was never able to reproduce the original problem..

-  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);

[sgurun-gerrit only, 2014/10/31 19:01:43]

Have you tried the condition here? Does it work fine with your changes? (note
that I just saw this and only skimmed it for a few minutes)

[boliu, 2014/10/31 19:49:01]

Not sure what you are asking here. What condition?

[hush (inactive), 2014/10/31 20:58:16]

Will an app that calls WebView#destroy() in ShouldOverrideUrlLoading work after
your patch?

[boliu, 2014/10/31 21:47:05]

I think it's pretty obvious it will work. Will add a test to make certain, as I
can't find the app that does this from code archeology.

+  delete this;
 }
 
 static jlong Init(JNIEnv* env, jclass, jobject browser_context) {
   // TODO(joth): Use |browser_context| to get the native BrowserContext, rather
   // than hard-code the default instance lookup here.
   scoped_ptr<WebContents> web_contents(content::WebContents::Create(
       content::WebContents::CreateParams(AwBrowserContext::GetDefault())));
   // Return an 'uninitialized' instance; most work is deferred until the
   // subsequent SetJavaPeers() call.
   return reinterpret_cast<intptr_t>(new AwContents(web_contents.Pass()));
@@ skipping 852 matching lines @@
   }
 
   browser_view_renderer_.TrimMemory(level, visible);
 }
 
 void SetShouldDownloadFavicons(JNIEnv* env, jclass jclazz) {
   g_should_download_favicons = true;
 }
 
 }  // namespace android_webview