Bug 1312152 - Stop using nsIDialogParamBlock in the client auth UI. r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Sun, 23 Oct 2016 12:57:41 +0800
changeset 319450 35996f91e760b614ce2e3e5992968bc7465df4bc
parent 319449 262687aa0bf271c01809f54c6cccd71e3ae052ae
child 319451 dba96457edcdae832cb9e55fa17c80566a43e29f
push id83172
push userphilringnalda@gmail.com
push dateWed, 26 Oct 2016 05:07:25 +0000
treeherdermozilla-inbound@c141993d03ee [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1312152
milestone52.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1312152 - Stop using nsIDialogParamBlock in the client auth UI. r=keeler nsIDialogParamBlock isn't a great API, and is best avoided. MozReview-Commit-ID: 2B0HkKNJizo
security/manager/pki/nsNSSDialogs.cpp
security/manager/pki/resources/content/clientauthask.js
security/manager/ssl/tests/mochitest/browser/browser_clientAuth_ui.js
--- a/security/manager/pki/nsNSSDialogs.cpp
+++ b/security/manager/pki/nsNSSDialogs.cpp
@@ -6,31 +6,31 @@
 
 /*
  * Dialog services for PIP.
  */
 
 #include "nsNSSDialogs.h"
 
 #include "mozIDOMWindow.h"
-#include "mozilla/Assertions.h"
-#include "mozilla/Casting.h"
 #include "nsArray.h"
 #include "nsEmbedCID.h"
+#include "nsHashPropertyBag.h"
 #include "nsIDialogParamBlock.h"
 #include "nsIInterfaceRequestor.h"
 #include "nsIInterfaceRequestorUtils.h"
 #include "nsIKeygenThread.h"
 #include "nsIPromptService.h"
 #include "nsIProtectedAuthThread.h"
 #include "nsIWindowWatcher.h"
 #include "nsIX509CertDB.h"
 #include "nsIX509Cert.h"
 #include "nsNSSDialogHelper.h"
 #include "nsString.h"
+#include "nsVariant.h"
 
 #define PIPSTRING_BUNDLE_URL "chrome://pippki/locale/pippki.properties"
 
 nsNSSDialogs::nsNSSDialogs()
 {
 }
 
 nsNSSDialogs::~nsNSSDialogs()
@@ -160,96 +160,100 @@ nsNSSDialogs::ChooseCertificate(nsIInter
 {
   NS_ENSURE_ARG_POINTER(ctx);
   NS_ENSURE_ARG_POINTER(certList);
   NS_ENSURE_ARG_POINTER(selectedIndex);
   NS_ENSURE_ARG_POINTER(certificateChosen);
 
   *certificateChosen = false;
 
-  nsCOMPtr<nsIDialogParamBlock> block =
-    do_CreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID);
-  if (!block) {
+  nsCOMPtr<nsIMutableArray> argArray = nsArrayBase::Create();
+  if (!argArray) {
     return NS_ERROR_FAILURE;
   }
 
-  // SetObjects() expects an nsIMutableArray, which is why we can't directly use
-  // |certList| and have to add an extra layer of indirection.
-  nsCOMPtr<nsIMutableArray> paramBlockArray = nsArrayBase::Create();
-  if (!paramBlockArray) {
-    return NS_ERROR_FAILURE;
-  }
-  nsresult rv = paramBlockArray->AppendElement(certList, false);
+  nsCOMPtr<nsIWritableVariant> hostnameVariant = new nsVariant();
+  nsresult rv = hostnameVariant->SetAsAUTF8String(hostname);
   if (NS_FAILED(rv)) {
     return rv;
   }
-  rv = block->SetObjects(paramBlockArray);
+  rv = argArray->AppendElement(hostnameVariant, false);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  nsCOMPtr<nsIWritableVariant> organizationVariant = new nsVariant();
+  rv = organizationVariant->SetAsAUTF8String(organization);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  rv = argArray->AppendElement(organizationVariant, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  rv = block->SetNumberStrings(3);
+  nsCOMPtr<nsIWritableVariant> issuerOrgVariant = new nsVariant();
+  rv = issuerOrgVariant->SetAsAUTF8String(issuerOrg);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  rv = argArray->AppendElement(issuerOrgVariant, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  rv = block->SetString(0, NS_ConvertUTF8toUTF16(hostname).get());
+  nsCOMPtr<nsIWritableVariant> portVariant = new nsVariant();
+  rv = portVariant->SetAsInt32(port);
   if (NS_FAILED(rv)) {
     return rv;
   }
-  rv = block->SetString(1, NS_ConvertUTF8toUTF16(organization).get());
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-  rv = block->SetString(2, NS_ConvertUTF8toUTF16(issuerOrg).get());
+  rv = argArray->AppendElement(portVariant, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  rv = block->SetInt(0, port);
+  rv = argArray->AppendElement(certList, false);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  nsCOMPtr<nsIWritablePropertyBag2> retVals = new nsHashPropertyBag();
+  rv = argArray->AppendElement(retVals, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   rv = nsNSSDialogHelper::openDialog(nullptr,
                                      "chrome://pippki/content/clientauthask.xul",
-                                     block);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-
-  int32_t status;
-  rv = block->GetInt(0, &status);
+                                     argArray);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   nsCOMPtr<nsIClientAuthUserDecision> extraResult = do_QueryInterface(ctx);
   if (extraResult) {
-    int32_t rememberSelection;
-    rv = block->GetInt(2, &rememberSelection);
+    bool rememberSelection = false;
+    rv = retVals->GetPropertyAsBool(NS_LITERAL_STRING("rememberSelection"),
+                                    &rememberSelection);
     if (NS_SUCCEEDED(rv)) {
-      extraResult->SetRememberClientAuthCertificate(rememberSelection!=0);
+      extraResult->SetRememberClientAuthCertificate(rememberSelection);
     }
   }
 
-  *certificateChosen = (status != 0);
+  rv = retVals->GetPropertyAsBool(NS_LITERAL_STRING("certChosen"),
+                                  certificateChosen);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
   if (*certificateChosen) {
-    int32_t index = 0;
-    rv = block->GetInt(1, &index);
+    rv = retVals->GetPropertyAsUint32(NS_LITERAL_STRING("selectedIndex"),
+                                      selectedIndex);
     if (NS_FAILED(rv)) {
       return rv;
     }
-
-    if (index < 0) {
-      MOZ_ASSERT_UNREACHABLE("Selected index should never be negative");
-      return NS_ERROR_FAILURE;
-    }
-
-    *selectedIndex = mozilla::AssertedCast<uint32_t>(index);
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP 
 nsNSSDialogs::SetPKCS12FilePassword(nsIInterfaceRequestor *ctx, 
                                     nsAString &_password,
--- a/security/manager/pki/resources/content/clientauthask.js
+++ b/security/manager/pki/resources/content/clientauthask.js
@@ -1,68 +1,94 @@
 /* -*- tab-width: 2; indent-tabs-mode: nil; js-indent-level: 2 -*-
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 /* import-globals-from pippki.js */
 "use strict";
 
+/**
+ * @file Implements the functionality of clientauthask.xul: a dialog that allows
+ *       a user pick a client certificate for TLS client authentication.
+ * @argument {String} window.arguments[0]
+ *           The hostname of the server requesting client authentication.
+ * @argument {String} window.arguments[1]
+ *           The Organization of the server cert.
+ * @argument {String} window.arguments[2]
+ *           The Organization of the issuer of the server cert.
+ * @argument {Number} window.arguments[3]
+ *           The port of the server.
+ * @argument {nsISupports} window.arguments[4]
+ *           List of certificates the user can choose from, queryable to
+ *           nsIArray<nsIX509Cert>.
+ * @argument {nsISupports} window.arguments[5]
+ *           Object to set the return values of calling the dialog on, queryable
+ *           to the underlying type of ClientAuthAskReturnValues.
+ */
+
+/**
+ * @typedef ClientAuthAskReturnValues
+ * @type nsIWritablePropertyBag2
+ * @property {Boolean} certChosen
+ *           Set to true if the user chose a cert and accepted the dialog, false
+ *           otherwise.
+ * @property {Boolean} rememberSelection
+ *           Set to true if the user wanted their cert selection to be
+ *           remembered, false otherwise.
+ * @property {Number} selectedIndex
+ *           The index the chosen cert is at for the given cert list. Undefined
+ *           value if |certChosen| is not true.
+ */
+
 const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
 
 const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
 
 /**
  * The pippki <stringbundle> element.
  * @type <stringbundle>
  */
 var bundle;
 /**
  * The array of certs the user can choose from.
  * @type nsIArray<nsIX509Cert>
  */
 var certArray;
 /**
- * The param block to get params from and set results on.
- * @type nsIDialogParamBlock
- */
-var dialogParams;
-/**
  * The checkbox storing whether the user wants to remember the selected cert.
  * @type nsIDOMXULCheckboxElement
  */
 var rememberBox;
 
 function onLoad() {
-  dialogParams = window.arguments[0].QueryInterface(Ci.nsIDialogParamBlock);
-
   bundle = document.getElementById("pippki_bundle");
   let rememberSetting =
     Services.prefs.getBoolPref("security.remember_cert_checkbox_default_setting");
 
   rememberBox = document.getElementById("rememberBox");
   rememberBox.label = bundle.getString("clientAuthRemember");
   rememberBox.checked = rememberSetting;
 
-  let hostname = dialogParams.GetString(0);
-  let org = dialogParams.GetString(1);
-  let issuerOrg = dialogParams.GetString(2);
-  let port = dialogParams.GetInt(0);
+  let hostname = window.arguments[0];
+  let org = window.arguments[1];
+  let issuerOrg = window.arguments[2];
+  let port = window.arguments[3];
   let formattedOrg = bundle.getFormattedString("clientAuthMessage1", [org]);
   let formattedIssuerOrg = bundle.getFormattedString("clientAuthMessage2",
                                                      [issuerOrg]);
   let formattedHostnameAndPort =
     bundle.getFormattedString("clientAuthHostnameAndPort",
                               [hostname, port.toString()]);
   setText("hostname", formattedHostnameAndPort);
   setText("organization", formattedOrg);
   setText("issuer", formattedIssuerOrg);
 
   let selectElement = document.getElementById("nicknames");
-  certArray = dialogParams.objects.queryElementAt(0, Ci.nsIArray);
+  certArray = window.arguments[4].QueryInterface(Ci.nsIArray);
   for (let i = 0; i < certArray.length; i++) {
     let menuItemNode = document.createElement("menuitem");
     let cert = certArray.queryElementAt(i, Ci.nsIX509Cert);
     let nickAndSerial =
       bundle.getFormattedString("clientAuthNickAndSerial",
                                 [cert.nickname, cert.serialNumber]);
     menuItemNode.setAttribute("value", i);
     menuItemNode.setAttribute("label", nickAndSerial); // This is displayed.
@@ -108,26 +134,22 @@ function setDetails() {
   document.getElementById("details").value = detailLines.join("\n");
 }
 
 function onCertSelected() {
   setDetails();
 }
 
 function doOK() {
-  // Signal that the user accepted.
-  dialogParams.SetInt(0, 1);
+  let retVals = window.arguments[5].QueryInterface(Ci.nsIWritablePropertyBag2);
+  retVals.setPropertyAsBool("certChosen", true);
   let index = parseInt(document.getElementById("nicknames").value);
-  // Signal the index of the selected cert in the list of cert nicknames
-  // provided.
-  dialogParams.SetInt(1, index);
-  // Signal whether the user wanted to remember the selection.
-  dialogParams.SetInt(2, rememberBox.checked);
+  retVals.setPropertyAsUint32("selectedIndex", index);
+  retVals.setPropertyAsBool("rememberSelection", rememberBox.checked);
   return true;
 }
 
 function doCancel() {
-  // Signal that the user cancelled.
-  dialogParams.SetInt(0, 0);
-  // Signal whether the user wanted to remember the "selection".
-  dialogParams.SetInt(2, rememberBox.checked);
+  let retVals = window.arguments[5].QueryInterface(Ci.nsIWritablePropertyBag2);
+  retVals.setPropertyAsBool("certChosen", false);
+  retVals.setPropertyAsBool("rememberSelection", rememberBox.checked);
   return true;
 }
--- a/security/manager/ssl/tests/mochitest/browser/browser_clientAuth_ui.js
+++ b/security/manager/ssl/tests/mochitest/browser/browser_clientAuth_ui.js
@@ -22,39 +22,31 @@ var cert;
 /**
  * Opens the client auth cert chooser dialog.
  *
  * @param {nsIX509Cert} cert The cert to pass to the dialog for display.
  * @returns {Promise}
  *          A promise that resolves when the dialog has finished loading, with
  *          an array consisting of:
  *            1. The window of the opened dialog.
- *            2. The nsIDialogParamBlock passed to the dialog.
+ *            2. The return value nsIWritablePropertyBag2 passed to the dialog.
  */
 function openClientAuthDialog(cert) {
-  let params = Cc["@mozilla.org/embedcomp/dialogparam;1"]
-                 .createInstance(Ci.nsIDialogParamBlock);
-
   let certList = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
   certList.appendElement(cert, false);
-  let array = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
-  array.appendElement(certList, false);
-  params.objects = array;
 
-  params.SetString(0, TEST_HOSTNAME);
-  params.SetString(1, TEST_ORG);
-  params.SetString(2, TEST_ISSUER_ORG);
-  params.SetInt(0, TEST_PORT);
-
+  let returnVals = Cc["@mozilla.org/hash-property-bag;1"]
+                     .createInstance(Ci.nsIWritablePropertyBag2);
   let win = window.openDialog("chrome://pippki/content/clientauthask.xul", "",
-                              "", params);
+                              "", TEST_HOSTNAME, TEST_ORG, TEST_ISSUER_ORG,
+                              TEST_PORT, certList, returnVals);
   return new Promise((resolve, reject) => {
     win.addEventListener("load", function onLoad() {
       win.removeEventListener("load", onLoad);
-      resolve([win, params]);
+      resolve([win, returnVals]);
     });
   });
 }
 
 /**
  * Checks that the contents of the given cert chooser dialog match the details
  * of build/pgo/certs/mochitest.client.
  *
@@ -101,45 +93,45 @@ function checkDialogContents(win, notBef
 add_task(function* setup() {
   cert = certDB.findCertByNickname("test client certificate");
   Assert.notEqual(cert, null, "Should be able to find the test client cert");
 });
 
 // Test that the contents of the dialog correspond to the details of the
 // provided cert.
 add_task(function* testContents() {
-  let [win, params] = yield openClientAuthDialog(cert);
+  let [win, retVals] = yield openClientAuthDialog(cert);
   checkDialogContents(win, cert.validity.notBeforeLocalTime,
                       cert.validity.notAfterLocalTime);
   yield BrowserTestUtils.closeWindow(win);
 });
 
 // Test that the right values are returned when the dialog is accepted.
 add_task(function* testAcceptDialogReturnValues() {
-  let [win, params] = yield openClientAuthDialog(cert);
+  let [win, retVals] = yield openClientAuthDialog(cert);
   win.document.getElementById("rememberBox").checked = true;
   info("Accepting dialog");
   win.document.getElementById("certAuthAsk").acceptDialog();
   yield BrowserTestUtils.windowClosed(win);
 
-  Assert.equal(params.GetInt(0), 1,
-               "1 should be returned to signal user accepted");
-  Assert.equal(params.GetInt(1), 0,
+  Assert.ok(retVals.get("certChosen"),
+            "Return value should signal user chose a certificate");
+  Assert.equal(retVals.get("selectedIndex"), 0,
                "0 should be returned as the selected index");
-  Assert.equal(params.GetInt(2), 1,
-               "1 should be returned as the state of the 'Remember this " +
-               "decision' checkbox");
+  Assert.ok(retVals.get("rememberSelection"),
+            "Return value should signal 'Remember this decision' checkbox was" +
+            "checked");
 });
 
 // Test that the right values are returned when the dialog is canceled.
 add_task(function* testCancelDialogReturnValues() {
-  let [win, params] = yield openClientAuthDialog(cert);
+  let [win, retVals] = yield openClientAuthDialog(cert);
   win.document.getElementById("rememberBox").checked = false;
   info("Canceling dialog");
   win.document.getElementById("certAuthAsk").cancelDialog();
   yield BrowserTestUtils.windowClosed(win);
 
-  Assert.equal(params.GetInt(0), 0,
-               "0 should be returned to signal user canceled");
-  Assert.equal(params.GetInt(2), 0,
-               "0 should be returned as the state of the 'Remember this " +
-               "decision' checkbox");
+  Assert.ok(!retVals.get("certChosen"),
+            "Return value should signal user did not choose a certificate");
+  Assert.ok(!retVals.get("rememberSelection"),
+            "Return value should signal 'Remember this decision' checkbox was" +
+            "unchecked");
 });