Plausible fix for crash in GetCertificateChain.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java b/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
index a4400576d91b8d9042c8e7fc115a2631fdb19c71..cb6195e175e7f381c541f5be7599d47e3f820564 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
+++ b/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() {

[Yaron, 2014/06/11 22:50:33]

I think this leak was always present. It's true that I moved construction from
c++ to java, but if you look at the previous static |Show| method, it was
already unmanaged.

+        // 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. */
@@ -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,11 @@ class WebsiteSettingsPopup implements OnClickListener {
         }
     }
 
-    @CalledByNative
-    private static WebsiteSettingsPopup create(Context context, ContentViewCore contentViewCore,
-            long nativeWebsiteSettingsPopup) {
-        return new WebsiteSettingsPopup(context, contentViewCore, nativeWebsiteSettingsPopup);
+    public static WebsiteSettingsPopup create(Context context, WebContents webContents) {
+        return new WebsiteSettingsPopup(context, webContents);

[Ted C, 2014/06/11 23:32:41]

any reason for having this return the popup?  It isn't really actionable and is
driven entirely by native.

I would probably just say call this method show (rename the native method to
showDialog) and make it a bit easier to read in the embedders.  Also, javadoc
:-) 

[Yaron, 2014/06/12 01:04:28]

Done.

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