Plausible fix for crash in GetCertificateChain.

The call to get the certificate chain from native assumes a valid
ContentViewCore. Switch to using WebContents which will get properly
invalidated/handled if the tab which launches this dialog closes before
the dialog is presented.

Also includes a slight change to avoid bouncing Java->native->Java by
allowing the dialog to be created from Java.

BUG=379928

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@276653 0039d316-1c4b-4281-b951-d872f2087c98

chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java

@@ -24,42 +24,45 @@ import android.widget.TextView;
 
 import org.chromium.base.CalledByNative;
 import org.chromium.chrome.R;
-import org.chromium.content.browser.ContentViewCore;
+import org.chromium.content_public.browser.WebContents;
 
 import java.net.URISyntaxException;
 
 /**
  * Java side of Android implementation of the website settings UI.
  */
-class WebsiteSettingsPopup implements OnClickListener {
+public class WebsiteSettingsPopup implements OnClickListener {
     private static final String HELP_URL =
             "http://www.google.com/support/chrome/bin/answer.py?answer=95617";
     private final Context mContext;
     private final Dialog mDialog;
     private final LinearLayout mContainer;
-    private final ContentViewCore mContentViewCore;
+    private final WebContents mWebContents;
     private final int mPadding;
     private TextView mCertificateViewer, mMoreInfoLink;
     private String mLinkUrl;
 
-    private WebsiteSettingsPopup(Context context, ContentViewCore contentViewCore,
-            final long nativeWebsiteSettingsPopup) {
+    private WebsiteSettingsPopup(Context context, WebContents webContents) {
         mContext = context;
+        mWebContents = webContents;
+
+        mContainer = new LinearLayout(mContext);
+        mContainer.setOrientation(LinearLayout.VERTICAL);
+        mPadding = (int) context.getResources().getDimension(R.dimen.certificate_viewer_padding);
+        mContainer.setPadding(mPadding, 0, mPadding, 0);
+
         mDialog = new Dialog(mContext);
         mDialog.requestWindowFeature(Window.FEATURE_NO_TITLE);
         mDialog.setCanceledOnTouchOutside(true);
-        mDialog.setOnCancelListener(new DialogInterface.OnCancelListener() {
+        // This needs to come after other member initialization.
+        final long nativeWebsiteSettingsPopup = nativeInit(this, webContents);
+        mDialog.setOnDismissListener(new DialogInterface.OnDismissListener() {
             @Override
-            public void onCancel(DialogInterface dialogInterface) {
+            public void onDismiss(DialogInterface dialog) {
                 assert nativeWebsiteSettingsPopup != 0;
                 nativeDestroy(nativeWebsiteSettingsPopup);
             }
         });
-        mContainer = new LinearLayout(mContext);
-        mContainer.setOrientation(LinearLayout.VERTICAL);
-        mContentViewCore = contentViewCore;
-        mPadding = (int) context.getResources().getDimension(R.dimen.certificate_viewer_padding);
-        mContainer.setPadding(mPadding, 0, mPadding, 0);
     }
 
     /** Adds a section, which contains an icon, a headline, and a description. */
@@ -118,7 +121,7 @@ class WebsiteSettingsPopup implements OnClickListener {
 
     /** Displays the WebsiteSettingsPopup. */
     @CalledByNative
-    private void show() {
+    private void showDialog() {
         ScrollView scrollView = new ScrollView(mContext);
         scrollView.addView(mContainer);
         mDialog.addContentView(scrollView,
@@ -131,7 +134,12 @@ class WebsiteSettingsPopup implements OnClickListener {
     public void onClick(View v) {
         mDialog.dismiss();
         if (mCertificateViewer == v) {
-            byte[][] certChain = nativeGetCertificateChain(mContentViewCore);
+            byte[][] certChain = nativeGetCertificateChain(mWebContents);
+            if (certChain == null) {
+                // The WebContents may have been destroyed/invalidated. If so,
+                // ignore this request.
+                return;
+            }
             CertificateViewer.showCertificateChain(mContext, certChain);
         } else if (mMoreInfoLink == v) {
             try {
@@ -143,12 +151,22 @@ class WebsiteSettingsPopup implements OnClickListener {
         }
     }
 
-    @CalledByNative
-    private static WebsiteSettingsPopup create(Context context, ContentViewCore contentViewCore,
-            long nativeWebsiteSettingsPopup) {
-        return new WebsiteSettingsPopup(context, contentViewCore, nativeWebsiteSettingsPopup);
+    /**
+     * Shows a WebsiteSettings dialog for the provided WebContents.
+     *
+     * The popup adds itself to the view hierarchy which owns the reference while it's
+     * visible.
+     *
+     * @param context Context which is used for launching a dialog.
+     * @param webContents The WebContents for which to show Website information. This
+     *         information is retrieved for the visible entry.
+     */
+    @SuppressWarnings("unused")
+    public static void show(Context context, WebContents webContents) {
+        new WebsiteSettingsPopup(context, webContents);
     }
 
+    private static native long nativeInit(WebsiteSettingsPopup popup, WebContents webContents);
     private native void nativeDestroy(long nativeWebsiteSettingsPopupAndroid);
-    private native byte[][] nativeGetCertificateChain(ContentViewCore contentViewCore);
-}
+    private native byte[][] nativeGetCertificateChain(WebContents webContents);
+}
\ No newline at end of file

chrome/browser/ui/android/website_settings_popup_android.cc

@@ -11,7 +11,6 @@
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/website_settings/website_settings.h"
-#include "content/public/browser/android/content_view_core.h"
 #include "content/public/browser/cert_store.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
@@ -32,11 +31,14 @@ using content::WebContents;
 
 static jobjectArray GetCertificateChain(JNIEnv* env,
                                         jobject obj,
-                                        jobject view) {
-  content::WebContents* contents =
-      content::ContentViewCore::GetNativeContentViewCore(env, view)->
-          GetWebContents();
-  int cert_id = contents->GetController().GetVisibleEntry()->GetSSL().cert_id;
+                                        jobject java_web_contents) {
+  content::WebContents* web_contents =
+      content::WebContents::FromJavaWebContents(java_web_contents);
+  if (!web_contents)
+    return NULL;
+
+  int cert_id =
+      web_contents->GetController().GetVisibleEntry()->GetSSL().cert_id;
   scoped_refptr<net::X509Certificate> cert;
   bool ok = CertStore::GetInstance()->RetrieveCert(cert_id, &cert);
   CHECK(ok);
@@ -64,18 +66,20 @@ static jobjectArray GetCertificateChain(JNIEnv* env,
 }
 
 // static
-void WebsiteSettingsPopupAndroid::Show(JNIEnv* env,
-                                       jobject context,
-                                       jobject java_content_view,
-                                       WebContents* web_contents) {
-  new WebsiteSettingsPopupAndroid(env, context, java_content_view,
-                                  web_contents);
+static jlong Init(JNIEnv* env,
+                  jclass clazz,
+                  jobject obj,
+                  jobject java_web_contents) {
+  content::WebContents* web_contents =
+      content::WebContents::FromJavaWebContents(java_web_contents);
+
+  return reinterpret_cast<intptr_t>(
+      new WebsiteSettingsPopupAndroid(env, obj, web_contents));
 }
 
 WebsiteSettingsPopupAndroid::WebsiteSettingsPopupAndroid(
     JNIEnv* env,
-    jobject context,
-    jobject java_content_view,
+    jobject java_website_settings_pop,
     WebContents* web_contents) {
   // Important to use GetVisibleEntry to match what's showing in the omnibox.
   content::NavigationEntry* nav_entry =
@@ -83,9 +87,7 @@ WebsiteSettingsPopupAndroid::WebsiteSettingsPopupAndroid(
   if (nav_entry == NULL)
     return;
 
-  popup_jobject_.Reset(
-      Java_WebsiteSettingsPopup_create(env, context, java_content_view,
-                                       reinterpret_cast<intptr_t>(this)));
+  popup_jobject_.Reset(env, java_website_settings_pop);
 
   presenter_.reset(new WebsiteSettings(
       this,
@@ -152,7 +154,7 @@ void WebsiteSettingsPopupAndroid::SetIdentityInfo(
   Java_WebsiteSettingsPopup_addMoreInfoLink(env, popup_jobject_.obj(),
       ConvertUTF8ToJavaString(
           env, l10n_util::GetStringUTF8(IDS_PAGE_INFO_HELP_CENTER_LINK)).obj());
-  Java_WebsiteSettingsPopup_show(env, popup_jobject_.obj());
+  Java_WebsiteSettingsPopup_showDialog(env, popup_jobject_.obj());
 }
 
 void WebsiteSettingsPopupAndroid::SetCookieInfo(

chrome/browser/ui/android/website_settings_popup_android.h

@@ -16,14 +16,12 @@ namespace content {
 class WebContents;
 }
 
-// Androidimplementation of the website settings UI.
+// Android implementation of the website settings UI.
 class WebsiteSettingsPopupAndroid : public WebsiteSettingsUI {
  public:
-  static void Show(JNIEnv* env,
-                   jobject context,
-                   jobject java_content_view,
-                   content::WebContents* web_contents);
-
+  WebsiteSettingsPopupAndroid(JNIEnv* env,
+                              jobject java_website_settings,
+                              content::WebContents* web_contents);
   virtual ~WebsiteSettingsPopupAndroid();
   void Destroy(JNIEnv* env, jobject obj);
 
@@ -38,11 +36,6 @@ class WebsiteSettingsPopupAndroid : public WebsiteSettingsUI {
   static bool RegisterWebsiteSettingsPopupAndroid(JNIEnv* env);
 
  private:
-  WebsiteSettingsPopupAndroid(JNIEnv* env,
-                              jobject context,
-                              jobject java_content_view,
-                              content::WebContents* web_contents);
-
   // The presenter that controlls the Website Settings UI.
   scoped_ptr<WebsiteSettings> presenter_;