1025849 - Create Mozmill Test for checking EV certificate status after a restart

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Bug 995801 was a really bad regression in Firefox, which existed for three releases (28.0 - 30.0) unfixed. Given that there can't be implemented a test run via buildbot, we should get this done ASAP via Mozmill.

Steps:
1. Open pages like https://addons.mozilla.org/en-US/firefox/ and/or https://bugzilla.mozilla.org/
2. Ensure that the EV status is shown for the SSL certificate
3. Restart Firefox
4. Check again for the EV status of the given pages

Andreea, can we get someone to work on that test please?

Marking as wanted for all releases except Firefox 30.0, given that the test would still fail with that version. Maybe use it for testing the patch.

Comment 1

[Andreea Matei [:AndreeaMatei]]

Cosmin will look at this tomorrow, after updating the PR for TPS jenkins. Daniel has duty updates, myself and Andrei are working on the other goals.

Comment 2

[Andreea Matei [:AndreeaMatei]]

As established later, Mihaela will work on this so Cosmin will focus on TPS CI.

Comment 3

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v1

Comment 4

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8441362 [details] [diff] [review]
v1

Review of attachment 8441362 [details] [diff] [review]:
-----------------------------------------------------------------

We should have this under a single test, with different functions.

::: firefox/tests/remote/restartTests/testEV_displayAfterRestart/test1.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +var toolbars = require("../../../../lib/toolbars");
> +var prefs = require("../../../../lib/prefs");

Please order them alphabetically

@@ +7,5 @@
> +var toolbars = require("../../../../lib/toolbars");
> +var prefs = require("../../../../lib/prefs");
> +
> +const TEST_PAGE = "https://addons.mozilla.org/en-US/firefox/";
> +const PREF_STARTUP = "browser.startup.page";

We should separate them because are different scopes.

@@ +32,5 @@
> +                       "Extended certificate is verified");
> +
> +  var identityLabel = locationBar.getElement({type: "identityIconLabel"});
> +  assert.equal(identityLabel.getNode().value, "Mozilla Foundation",
> +                       "Identity name is correct");

Messages are not intended properly, should start after '('

Comment 5

[Andrei Eftimie]

Comment on attachment 8441362 [details] [diff] [review]
v1

Review of attachment 8441362 [details] [diff] [review]:
-----------------------------------------------------------------

Test itself looks good to me, thanks Mihaela.
Just some refactoring and a few nits to fix.

Please use only 1 file and do the restart with `restartApplication(%second_test_name%)`

::: firefox/tests/remote/restartTests/testEV_displayAfterRestart/test1.js
@@ +21,5 @@
> +/**
> + * Test that browser shows the EV status for the SSL certificate
> + */
> +function testEV_displayEVStatus() {
> +   // Open a  website which shows the EV certificate

nit: indent and extra whitespaces

@@ +26,5 @@
> +  controller.open(TEST_PAGE);
> +  controller.waitForPageLoad();
> +
> +  // Check that the EV status is shown for the SSL certificate
> +  var identityBox = locationBar.getElement({type:"identityBox"});

nit: missing whitespace after the colon

@@ +28,5 @@
> +
> +  // Check that the EV status is shown for the SSL certificate
> +  var identityBox = locationBar.getElement({type:"identityBox"});
> +  assert.equal(identityBox.getNode().getAttribute("class"), "verifiedIdentity",
> +                       "Extended certificate is verified");

nit: indentation

@@ +32,5 @@
> +                       "Extended certificate is verified");
> +
> +  var identityLabel = locationBar.getElement({type: "identityIconLabel"});
> +  assert.equal(identityLabel.getNode().value, "Mozilla Foundation",
> +                       "Identity name is correct");

nit: indentation

::: firefox/tests/remote/restartTests/testEV_displayAfterRestart/test2.js
@@ +18,5 @@
> + * Test that browser shows the EV status for the SSL certificate after
> + * a browser restart
> + **/
> +function testEV_displayEVStatusafterRestart() {
> +  var identityBox = locationBar.getElement({type:"identityBox"});

nit: missing space after the colon

@@ +20,5 @@
> + **/
> +function testEV_displayEVStatusafterRestart() {
> +  var identityBox = locationBar.getElement({type:"identityBox"});
> +  assert.equal(identityBox.getNode().getAttribute("class"), "verifiedIdentity",
> +                       "Extended certificate is verified");

nit: indentation

@@ +24,5 @@
> +                       "Extended certificate is verified");
> +
> +  var identityLabel = locationBar.getElement({type: "identityIconLabel"});
> +  assert.equal(identityLabel.getNode().value, "Mozilla Foundation",
> +                       "Identity name is correct");

nit: indentation

@@ +27,5 @@
> +  assert.equal(identityLabel.getNode().value, "Mozilla Foundation",
> +                       "Identity name is correct");
> +}
> +
> +var teardownModule = function (aModule) {

Please use `function teardownModule`

Comment 6

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v2

Updated based on feedback: made the test in a single file, fixed indentation nits.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8442764 [details] [diff] [review]
v2

Review of attachment 8442764 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good now for me.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8442764 [details] [diff] [review]
v2

Review of attachment 8442764 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/tests/remote/restartTests/manifest.ini
@@ +14,5 @@
>  [include:testDiscoveryPane_installPickOfMonthAddon/manifest.ini]
>  disabled = Bug 657492 - 'Pick of the Month' add-ons are only compatible with Release and Beta builds
>  [include:testAddons_InstallAddonWithoutEULA/manifest.ini]
>  disabled = Bug 992187 - Test failure 'addButton is undefined'
> +[include:testEV_displayAfterRestart/manifest.ini]

There is no need to put this test under restartTests. This folder only exists for the old-style restart tests with Mozmill. Add it directly to the testSecurity subfolder.

::: firefox/tests/remote/restartTests/testEV_displayAfterRestart/manifest.ini
@@ +1,1 @@
> +[test1.js]

Once moved please give it a better name, which not only contains EV but more like: testSSLStatusAfterRestart.js.

::: firefox/tests/remote/restartTests/testEV_displayAfterRestart/test1.js
@@ +10,5 @@
> +const PREF_STARTUP = "browser.startup.page";
> +
> +const TEST_PAGE = {url: "https://addons.mozilla.org/en-US/firefox/",
> +                   identity: "Mozilla Foundation"
> +                  };

nit: Please fix the indentation here, by moving the first entry to the next line and use 2 spaces.

@@ +23,5 @@
> +  aModule.locationBar = new toolbars.locationBar(controller);
> +}
> +
> +/**
> + * Test that browser shows the EV status for the SSL certificate

Please add the reference to the bug.

@@ +25,5 @@
> +
> +/**
> + * Test that browser shows the EV status for the SSL certificate
> + */
> +function testEV_displayEVStatus() {

Can you please check if that also applies to DV, and OV? We might want to enhance this test appropriately.

@@ +28,5 @@
> + */
> +function testEV_displayEVStatus() {
> +  // Open a website which shows the EV certificate
> +  controller.open(TEST_PAGE.url);
> +  controller.waitForPageLoad("before restart");

What is that string expected to do? We do not support a message here.

@@ +42,5 @@
> +  // Check that the EV status is shown for the SSL certificate
> +  verifyEVCertificateStatus("after restart");
> +}
> +
> +function teardownModule (aModule) {

nit: no space between function name and opening brackets.

@@ +47,5 @@
> +  // reset the startup preference
> +  prefs.preferences.clearUserPref(PREF_STARTUP);
> +}
> +
> +function verifyEVCertificateStatus (aMessage) {

Same here.

@@ +54,5 @@
> +               "Extended certificate is verified " + aMessage);
> +
> +  var identityLabel = locationBar.getElement({type: "identityIconLabel"});
> +  assert.equal(identityLabel.getNode().value, TEST_PAGE.identity,
> +               "Identity name is correct " + aMessage);

I would propose you check the other security related tests under remote, and do checks against the retrieved SSL certificate too.

Comment 9

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v3

updated based on Henrik's review

Comment 10

[Andrei Eftimie]

Comment on attachment 8448032 [details] [diff] [review]
v3

Review of attachment 8448032 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't apply cleanly anymore, please update the patch.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +13,5 @@
> +const TEST_PAGE_EV = {
> +                     url: "https://addons.mozilla.org/en-US/firefox/",
> +                     identity: "Mozilla Foundation",
> +                     vtype: "verifiedIdentity"
> +                  };

Please indent these like:
> const TEST_PAGE_EV = {url: "https://addons.mozilla.org/en-US/firefox/",
>                       identity: "Mozilla Foundation",
>                       vtype: "verifiedIdentity"};

@@ +51,5 @@
> + */
> +function testEV_displayCertificateStatus() {
> +  // Open a website which shows the EV certificate
> +  controller.open(TEST_PAGE_EV.url);
> +  controller.waitForPageLoad(TEST_PAGE_EV.url);

waitForPageLoad() doesn't accept any arguments. This won't do anything.

@@ +117,5 @@
> +
> +  verifyCertificateStatus(TEST_PAGE_OV, checkSecurityTab_OV);
> +}
> +
> +function getCertificate() {

We should make this function as part of a library.
Please create a security library and move this function over, then also create a bug to move this code from other places where its used in the same way.
(I've found it used in: testSecurityInfoViaMoreInformation.js, testGreenLarry.js and testDVCertificate.js)

Edit: We actually have this library created as part of bug 863139 (see /lib/security.js).
As that bug appears to be on hold, I propose to take only this library file and land it part of this bug.

@@ +175,5 @@
> +  var securityTab = new elementslib.ID(aController.window.document, "securityTab");
> +  expect.ok(securityTab.getNode().selected, "The Security tab is selected by default");
> +
> +  // Check the Web Site label against the Cert CName
> +  var webIDDomainLabel = new elementslib.ID(aController.window.document,

Instead of elementslib please use findElement.

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v4

Updated based on previous review

Comment 12

[Andrei Eftimie]

Comment on attachment 8449394 [details] [diff] [review]
v4

Review of attachment 8449394 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few nits from me and we're good.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +41,5 @@
> +  aModule.cert = null;
> +}
> +
> +function teardownModule(aModule) {
> +  // reset the startup preference

I think we can remove this comment

@@ +119,5 @@
> +               "Extended certificate is verified for " + aPage.url);
> +
> +  var identityLabel = locationBar.getElement({type: "identityIconLabel"});
> +  expect.equal(identityLabel.getNode().value, aPage.identity,
> +               "Identity name is correct  for " + aPage.url);

nit: there's an extra space

@@ +122,5 @@
> +  expect.equal(identityLabel.getNode().value, aPage.identity,
> +               "Identity name is correct  for " + aPage.url);
> +
> +  //check the retrieved SSL certificate
> +  var moreInfoButton = new findElement.ID(controller.window.document,

findElement doesn't need the `new` keyword, please update all instances.

::: lib/security.js
@@ +1,5 @@
> +/* 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/. */
> +
> + "use strict";

nit: extra space

@@ +30,5 @@
> + * @param {Object} aCert
> + *        The certificate for which we get the property
> + * @param {String} aPrefName
> + *        Poperty of the certificate needed
> + * @param {String} [aLocationProp]

nit: please make native types lowercase (also applies to the `object` type)

Comment 13

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v4.1

Comment 14

[Andrei Eftimie]

Comment on attachment 8451724 [details] [diff] [review]
v4.1

Review of attachment 8451724 [details] [diff] [review]:
-----------------------------------------------------------------

Please ask a review from Henrik with the nits updated.
Looks good to me otherwise.

Thanks!

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +122,5 @@
> +               "Identity name is correct for " + aPage.url);
> +
> +  //check the retrieved SSL certificate
> +  var moreInfoButton = findElement.ID(controller.window.document,
> +                                          "identity-popup-more-info-button");

nit: indentation

Please check all instances where you removed the `new` keyword to also update the indentation for the next row, they are all misaligned right now.

::: lib/security.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> + // Include required modules

nit: extra space

Comment 15

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v4.2

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8453149 [details] [diff] [review]
v4.2

Review of attachment 8453149 [details] [diff] [review]:
-----------------------------------------------------------------

Why am I getting asked for review? I don't see a passed review by Andrei yet. Anyway, my comments inline.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +10,5 @@
> +var utils = require("../../../lib/utils");
> +
> +const PREF_STARTUP = "browser.startup.page";
> +
> +const TEST_PAGE_EV = {

Please use a TEST_DATA constant here, and fix the indentation so it applies to the coding style we make use of, which is:

const CONST = {
  url: ...,
  ...
};

@@ +11,5 @@
> +
> +const PREF_STARTUP = "browser.startup.page";
> +
> +const TEST_PAGE_EV = {
> +                        url: "https://addons.mozilla.org/en-US/firefox/",

Please don't load that page, but use the same page as we make use of in the green larry security test. It loads way faster.

@@ +13,5 @@
> +
> +const TEST_PAGE_EV = {
> +                        url: "https://addons.mozilla.org/en-US/firefox/",
> +                        identity: "Mozilla Foundation",
> +                        vtype: "verifiedIdentity"

'v' sounds like a Hungerian notication, which is not here. So just use 'type'.

@@ +23,5 @@
> +                        vtype: "verifiedDomain"
> +                     };
> +
> +const TEST_PAGE_OV = {
> +                        url: "http://mozqa.com/",

Please use explicitly the ssl-ov subdomain.

@@ +45,5 @@
> +  prefs.preferences.clearUserPref(PREF_STARTUP);
> +}
> +
> +/**
> + * Bug 995801 - EV identifier is no longer displayed in Location Bar/Arrow

Move the description to the next line, so we don't have an indentation of unnecessary 15 chars.

@@ +58,5 @@
> +  cert = security.getCertificate(controller);
> +  verifyCertificateStatus(TEST_PAGE_EV, checkSecurityTab_EV);
> +
> +  // restart the browser
> +  controller.restartApplication("test_displayEVStatusAfterRestart");

We never want to have those lines in the test itself. Otherwise we do not restart the browser for the next test, and have invalid test results.

I would have done only two single test methods. The first loads all necessary pages into different tabs, and checks the cert status. Then Firefox gets restarted, and the second test function checks for the cert status of all open tabs again.

@@ +72,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Check that the DV status is shown for the SSL certificate
> +  cert = security.getCertificate(controller);
> +  verifyCertificateStatus(TEST_PAGE_DV, checkSecurityTab_DV);

Well, that doesn't match the function name. So lets do the 2 function approach?

@@ +101,5 @@
> +  cert = security.getCertificate(controller);;
> +  verifyCertificateStatus(TEST_PAGE_OV, checkSecurityTab_OV);
> +}
> +
> +function verifyCertificateStatus(aPage, aFunction) {

'aFunction' doesn't say anything here. Please use a proper name like aSSLCheckCallback.

@@ +120,5 @@
> +  var identityLabel = locationBar.getElement({type: "identityIconLabel"});
> +  expect.equal(identityLabel.getNode().value, aPage.identity,
> +               "Identity name is correct for " + aPage.url);
> +
> +  //check the retrieved SSL certificate

nit: Missing blank and capital letter please.

@@ +130,5 @@
> +
> +function checkSecurityTab_EV(aController) {
> +  var securityTab = findElement.ID(aController.window.document, "securityTab");
> +  expect.ok(securityTab.getNode().selected,
> +            "The security tab is selected by default");

Same line? It varies between the different callbacks.

@@ +138,5 @@
> +                                        "security-identity-domain-value");
> +  expect.notEqual(webIDDomainLabel.getNode().value.indexOf(cert.commonName), -1,
> +                  "Found certificate common name '" + cert.commonName + "'");
> +
> +  var webIDOwnerLabel = findElement.ID(aController.window.document,

We should avoid using 'Label' in variable names. If it is changed to another element type, we have semantically broken tests. What about a simple 'ownerElem'?

@@ +144,5 @@
> +  expect.equal(webIDOwnerLabel.getNode().value, cert.organization,
> +         "Owner matches certificate's organization");
> +
> +  var webIDVerifierLabel = findElement.ID(aController.window.document,
> +                                          "security-identity-verifier-value");

Same here, and others.

@@ +148,5 @@
> +                                          "security-identity-verifier-value");
> +  expect.equal(webIDVerifierLabel.getNode().value, cert.issuerOrganization,
> +               "Verifier matches certificate's issuer");
> +
> +  aController.keypress(null, 'VK_ESCAPE', {});

If a line above fails with an exception, this line will not be executed and the windows stays open.

@@ +193,5 @@
> +  var securityOwner = utils.getProperty("chrome://browser/locale/pageInfo.properties",
> +                                        "securityNoOwner");
> +
> +  expect.equal(webIDOwnerLabel.getNode().value, securityOwner,
> +               "Expected owner label found");

What about a check for security-identity-verifier-value?

::: lib/security.js
@@ +14,5 @@
> + *         Controller for which to get the certificate
> + *
> + * @returns {Object} Certificate of the current browsing page
> + */
> +function getCertificate(aController) {

This is a back-end module. We should avoid passing in a controller.

@@ +17,5 @@
> + */
> +function getCertificate(aController) {
> +  assert.ok(aController, "Certifcate controller is present");
> +
> +  var securityUI = aController.window.getBrowser().mCurrentBrowser.securityUI;

Why can't we add a new method to the TabBrowser class, which returns that? Like `getSecurityUI()`.

@@ +20,5 @@
> +
> +  var securityUI = aController.window.getBrowser().mCurrentBrowser.securityUI;
> +  var SSLStatus = securityUI.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus;
> +
> +  return (SSLStatus === null) ? null : SSLStatus.serverCert;

You can do `SSLStatus ? SSLStatus.serverCert : SSLStatus`

@@ +29,5 @@
> + *
> + * @param {Object} aCert
> + *        The certificate for which we get the property
> + * @param {string} aPrefName
> + *        Poperty of the certificate needed

I don't see where 'Pref' in that parameter name comes from.

@@ +46,5 @@
> +                                         aCert.subjectName.indexOf(",postalCode="));
> +    case "state" :
> +      return aCert.subjectName.substring(aCert.subjectName.indexOf("ST=") + 3,
> +                                         aCert.subjectName.indexOf(",C="));
> +    case "location":

Alphabetical sort order please.

@@ +47,5 @@
> +    case "state" :
> +      return aCert.subjectName.substring(aCert.subjectName.indexOf("ST=") + 3,
> +                                         aCert.subjectName.indexOf(",C="));
> +    case "location":
> +      var city = getCertificateProperty(aCert,"city");

nit: indentations necessary.

@@ +51,5 @@
> +      var city = getCertificateProperty(aCert,"city");
> +      var state = getCertificateProperty(aCert,"state");
> +      var country = getCertificateProperty(aCert,"country");
> +      var updateLocationLabel = aLocationProp.replace("%S", state).replace("%S", country);
> +      var location = city + '\n' + updateLocationLabel;

Are state and country also in separate lines above? Also you can return that directly.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Can we please get this test finished up and landed? Looks like there is a new regression on Nightly, which shows a similar behavior we want to test here.

Comment 18

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v5

Updated based on Henriks review, except the following:

> ::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
> @@ +11,5 @@
> > +
> > +const PREF_STARTUP = "browser.startup.page";
> > +
> > +const TEST_PAGE_EV = {
> > +                        url: "https://addons.mozilla.org/en-US/firefox/",
> 
> Please don't load that page, but use the same page as we make use of in the
> green larry security test. It loads way faster.

I used bugzilla.mozilla.org instead because there are some issues with displayiing the certficate for addons.mozilla.org ATM.

> @@ +130,5 @@
> > +
> > +function checkSecurityTab_EV(aController) {
> > +  var securityTab = findElement.ID(aController.window.document, "securityTab");
> > +  expect.ok(securityTab.getNode().selected,
> > +            "The security tab is selected by default");
> 
> Same line? It varies between the different callbacks.
This is the same for all 3 cases: clicking "More info" button in the certificate doorhanger opens the Page info window with focus on the "Security" section.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yes, lets go with bugzilla.mozilla.org for now. We can add the AMO domain, once the dependent bug is fixed.

Comment 20

[Andrei Eftimie]

Comment on attachment 8458532 [details] [diff] [review]
v5

Review of attachment 8458532 [details] [diff] [review]:
-----------------------------------------------------------------

With the mentioned item updated, please ask a review from Henrik.
Looks good to me otherwise.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +177,5 @@
> +    var domainElem = findElement.ID(aController.window.document,
> +                                    "security-identity-domain-value");
> +
> +    var certName = (cert.commonName.replace(/\./g, "\\\.")).replace(/\*/g, ".*");
> +    var certNameRegExp = new RegExp("^" + certName + "$");

This should be in a security library, but since we'll refactor the calls to get the security elements in a library in bug 863139, I'm fine to go ahead as it is with this one.

@@ +197,5 @@
> +    expect.equal(verifierElem.getNode().value, cert.issuerOrganization,
> +                 "Verifier matches certificate's issuer");
> +  }
> +  finally {
> +    aController.keypress(null, 'VK_ESCAPE', {});

Please issue the keypress event on an element, not on the controller. Since we are using the locationbar, this could be `locationbar.urlbar`.
Since controller.%action% is deprecated, we shouldn't introduce new code that depends on it.

Update all instances please.

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v5.1

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8464545 [details] [diff] [review]
v5.1

Review of attachment 8464545 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/security.js
@@ +17,5 @@
> + */
> +function getCertificate(aSecurityUI) {
> +  var SSLStatus = aSecurityUI.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus;
> +
> +  return SSLStatus ? SSLStatus.serverCert : SSLStatus;

In case the condition equals to false, will SSLStatus be null or undefined? To have a defined return value it would be better to directly return null then. That should also be mentioned in the jsdoc.

@@ +33,5 @@
> + *
> + * @returns {string} A string representing properties value
> + */
> +function getCertificateProperty(aCert, aLocation, aLocationProp) {
> +  switch(aLocation) {

nit: missing blank.

::: firefox/lib/tabs.js
@@ +676,5 @@
> +   * Get the security UI
> +   *
> +   * @returns {object} Reference to the security UI
> +   */
> +  getSecurityUI : function tabBrowser_getSecurityUI() {

I don't see a reason for a method here. There is no parameter we have to specify. So please make it a property.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +7,5 @@
> +var prefs = require("../../../lib/prefs");
> +var security = require("../../../lib/security");
> +var toolbars = require("../../../lib/toolbars");
> +var utils = require("../../../lib/utils");
> +var tabs = require("../../../lib/tabs");

nit: Please fix the ordering.

@@ +9,5 @@
> +var toolbars = require("../../../lib/toolbars");
> +var utils = require("../../../lib/utils");
> +var tabs = require("../../../lib/tabs");
> +
> +const PREF_STARTUP = "browser.startup.page";

STARTUP is not that descriptive. Please see the homepage tests for a better name like PREF_STARTUP_PAGE.

@@ +12,5 @@
> +
> +const PREF_STARTUP = "browser.startup.page";
> +
> +const TEST_DATA = {
> +  evPage : {

nit: the blank only after the colon.

@@ +18,5 @@
> +    identity: "Mozilla Foundation",
> +    type: "verifiedIdentity"
> +  },
> +
> +  dvPage : {

nit: before the evPage entry please.

@@ +41,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.tabBrowser = new tabs.tabBrowser(aModule.controller);
> +
> +  aModule.cert = null;

Please add a comment that this variable is shared between different methods because of async callback.

@@ +61,5 @@
> +}
> +
> +/**
> + * Bug 995801
> + * EV identifier is no longer displayed in Location Bar/Arrow Panel after a

Our test is not only about EV, but all types of SSL certs. So you cannot simply c&p the bug summary.

@@ +70,5 @@
> +  tabBrowser.closeAllTabs();
> +
> +  // Open a website which shows the EV certificate
> +  controller.open(TEST_DATA.evPage.url);
> +  controller.waitForPageLoad();

I would improve the test by loading all the pages simultaneously. Once the load has been triggered for each of them, go back to the first tab, and do your testing.

@@ +74,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Check that the EV status is shown for the SSL certificate
> +  cert = security.getCertificate(tabBrowser.getSecurityUI());
> +  verifyCertificateStatus(TEST_DATA.evPage, checkSecurityTab_EV);

I would put the callback name also into the TEST_DATA constant. So you won't have to specify it here.

@@ +83,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Check that the DV status is shown for the SSL certificate
> +  cert = security.getCertificate(tabBrowser.getSecurityUI());
> +  verifyCertificateStatus(TEST_DATA.dvPage, checkSecurityTab_DV);

With the above change we could really do a forEach here like: ['dvPage', 'evPage', 'ovPage'].forEach(type => { ... }); That will simplify the test a lot.

@@ +111,5 @@
> +
> +  // Check that there is no certificate available for the OV page
> +  tabBrowser.selectedIndex = 2;
> +  cert = security.getCertificate(tabBrowser.getSecurityUI());
> +  verifyCertificateStatus(TEST_DATA.ovPage, checkSecurityTab_OV);

Same for this method. Make it a forEach and it will become easy to read, without code duplication.

@@ +135,5 @@
> +               "Identity name is correct for " + aPage.url);
> +
> +  // Check the retrieved SSL certificate
> +  var moreInfoButton = findElement.ID(controller.window.document,
> +                                      "identity-popup-more-info-button");

Why is that not part of the locationBar class similar to the other two above?

@@ +147,5 @@
> +    expect.ok(securityTab.getNode().selected, "The security tab is selected by default");
> +
> +    // Check the Web Site label against the Cert CName
> +    var domainElem = findElement.ID(aController.window.document,
> +                                    "security-identity-domain-value");

One of our rules is not to retrieve elements directly in tests. What we would need here is a new ui module for the page-info window. Given the importance of this test, I would be ok to get this done in a follow-up bug. But please promise to fix that.

@@ +163,5 @@
> +                 "Verifier matches certificate's issuer");
> +  }
> +  finally {
> +    locationBar.urlbar.keypress('VK_ESCAPE', {});
> +  }

Instead of putting thte try/finally in each of those callbacks, it would be easier to have it around the handleWindow() call.

Comment 23

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v6

Updated based on Henrik's review.

> @@ +147,5 @@
> > +    expect.ok(securityTab.getNode().selected, "The security tab is selected by default");
> > +
> > +    // Check the Web Site label against the Cert CName
> > +    var domainElem = findElement.ID(aController.window.document,
> > +                                    "security-identity-domain-value");
> 
> One of our rules is not to retrieve elements directly in tests. What we
> would need here is a new ui module for the page-info window. Given the
> importance of this test, I would be ok to get this done in a follow-up bug.
> But please promise to fix that.

Logged bug 1055453

Comment 24

[Andrei Eftimie]

Comment on attachment 8475107 [details] [diff] [review]
v6

Review of attachment 8475107 [details] [diff] [review]:
-----------------------------------------------------------------

There are some conflicts in toolbars.js

Other that that this looks good.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +72,5 @@
> +  persisted.nextTest = "testDisplayCertificateStatusAfterRestart";
> +  tabBrowser.closeAllTabs();
> +
> +  // Open a website for each certificate type
> +  [TEST_DATA.evPage, TEST_DATA.dvPage, TEST_DATA.ovPage].forEach(aPage => {

Instead of doing this, just declare TEST_DATA as an array instead of on object, then you can do directly: TEST_DATA.forEach

You can probably also remove the names dvPage, evPage, ovPage from the TEST_DATA objects.

Comment 25

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v6.1

It looks like some updates were made to the toolbars.js lib in the meantime. Resolved the conflict and updated the test.

Comment 26

[Andrei Eftimie]

Comment on attachment 8476613 [details] [diff] [review]
v6.1

Review of attachment 8476613 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8476613 [details] [diff] [review]
v6.1

Review of attachment 8476613 [details] [diff] [review]:
-----------------------------------------------------------------

With those fixes you have my r=me. Ask for review from Andreea or Andrei once the update has been done. Thanks for your work here!

::: firefox/lib/toolbars.js
@@ +891,5 @@
>  // Export of classes
>  exports.locationBar = locationBar;
>  exports.editBookmarksPanel = editBookmarksPanel;
>  exports.autoCompleteResults = autoCompleteResults;
> +exports.IdentityPopup = IdentityPopup;

I don't see why this is necessary. The IdentityPopup is part of the locationbar, and there is already a property for it. So please get rid of this change. We don't want that tests can directly instantiate it.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +12,5 @@
> +
> +const PREF_STARTUP_PAGE = "browser.startup.page";
> +
> +const TEST_DATA = [
> +  {

nit: we have opening brackets at the end of the former line, but not in a new line.

@@ +19,5 @@
> +    type: "verifiedDomain",
> +    callback: checkSecurityTab_DV
> +  },
> +
> +  {

nit: Same here for indentation of the brackets, and no empty line please.

TEST_DATA = [{
  abc: yxz,
  def: stu
}, {
  ..
}];

@@ +75,5 @@
> +
> +  // Open a website for each certificate type
> +  TEST_DATA.forEach(aPage => {
> +    controller.open(aPage.url);
> +    controller.waitForPageLoad();

This is not what I meant. Here you are still waiting for a page to be loaded before continuing loading the next page. You want this waitForPageLoad call in the next for loop, which you can then merge with the code in testDisplayCertificateStatusAfterRestart()

@@ +126,5 @@
> +  try {
> +    utils.handleWindow("type", "Browser:page-info", aPage.callback);
> +  }
> +  finally {
> +    locationBar.urlbar.keypress('VK_ESCAPE', {});

Shouldn't we better issue this against the doorhanger?

@@ +132,5 @@
> +}
> +
> +function checkSecurityTab_EV(aController) {
> +    var securityTab = findElement.ID(aController.window.document, "securityTab");
> +    expect.ok(securityTab.getNode().selected, "The security tab is selected by default");

broken indentation for all of the callback functions.

Comment 28

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v6.2

Updated based on Henrik's review

Comment 29

[Andrei Eftimie]

Comment on attachment 8480366 [details] [diff] [review]
v6.2

Review of attachment 8480366 [details] [diff] [review]:
-----------------------------------------------------------------

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/b3efef6759d9 (default)

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +84,5 @@
> +  tabBrowser.selectedIndex = 0;
> +
> +  // Check that the correct status is shown for each certificate type
> +  TEST_DATA.forEach(aPage => {
> +    try {

Not a huge fan of a try here, but with Henrik's requested move of waitForPageLoad away from the initial opening, I've gotten multiple false positives, so this indeed will fail if the page is already loaded (which can happen even for the first run).
With keeping this here, I don't see any other better solution.

Comment 30

[Andrei Eftimie]

From the flags I gather we want to backport this across branches.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There is no Firefox 35 yet, given no version bump happened so far.

(In reply to Andrei Eftimie from comment #29)
> > +  // Check that the correct status is shown for each certificate type
> > +  TEST_DATA.forEach(aPage => {
> > +    try {
> 
> Not a huge fan of a try here, but with Henrik's requested move of
> waitForPageLoad away from the initial opening, I've gotten multiple false
> positives, so this indeed will fail if the page is already loaded (which can
> happen even for the first run).
> With keeping this here, I don't see any other better solution.

It would have been good to not seeing it landed then, if things are not clear. And that seems to be the case here. You know that I'm not a fan of workarounds, especially if situations can be avoided.

So in general waitForPageLoad should NOT fail in those cases. For each load a new unique id for the content window is created. So each tab has its own id. Then when you iterate through each tab, waitForPageLoad should directly return, because we have an id and not null anymore.

So lets revert those changes, and directly wait for the page being loaded. We should create a testcase and file a Mozmill bug for exactly that issue.

Comment 32

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #31)
> So lets revert those changes, and directly wait for the page being loaded.
> We should create a testcase and file a Mozmill bug for exactly that issue.

I'm not sure I fully understand. Currently we can't have 2 waitForPageLoad() calls on the same page, the second one will fail.
Although in this particular case we do have a restart between them (not sure how this interacts with waitForPageLoad).

So this is interesting. I've run the test without the try call multiple times and all of them passed. I did have failures yesterday in what seemed like race conditions of some sort (in regards to the loading of the pane and the showing of the panels on the second loop).

I've backed out the landing for now:
https://hg.mozilla.org/qa/mozmill-tests/rev/35990c6fe864 (default)

Mihaela, what kind of failures did you see that prompted you to include the try/finally block?
Given that we have a restart, this should work as Henrik mentioned in comment 31

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrei Eftimie from comment #32)
> I'm not sure I fully understand. Currently we can't have 2 waitForPageLoad()
> calls on the same page, the second one will fail.

Correct, and that is how it should work because nothing has changed. You only get a new id when an unload and load event happens.

> Although in this particular case we do have a restart between them (not sure
> how this interacts with waitForPageLoad).

Right. After a restart the ids are again undefined and will be set once the page has been loaded from the cache. So all has to be fine here.

Comment 34

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v6.3

I removed the try-catch block, ran the test several times on Nightly and got no failures, although it failed a lot before (when I added the try-catch).

Comment 35

[Andrei Eftimie]

Comment on attachment 8482605 [details] [diff] [review]
v6.3

Review of attachment 8482605 [details] [diff] [review]:
-----------------------------------------------------------------

Lets get this back in, without the try/catch ;)
Landed: https://hg.mozilla.org/qa/mozmill-tests/rev/70bdf4f1d5bb (default)

Comment 36

[Andrei Eftimie]

Seems we forgot to add the test in the manifest file!

Comment 37

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v6.4

Added test in manifest. 
Tested on default and mozilla-aurora.

Comment 38

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: add test to manifest

Comment 39

[Andrei Eftimie]

Comment on attachment 8492028 [details] [diff] [review]
add test to manifest

Review of attachment 8492028 [details] [diff] [review]:
-----------------------------------------------------------------

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/e05e9db8dec1 (default)

Comment 40

[Andrei Eftimie]

Comment on attachment 8492026 [details] [diff] [review]
v6.4

Review of attachment 8492026 [details] [diff] [review]:
-----------------------------------------------------------------

Works nicely on Aurora: http://mozmill-crowd.blargon7.com/#/remote/report/2f982f72826307fed840a3b11c5a0552

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/9af964c8c5f0 (mozilla-aurora)

Comment 41

[Andrei Eftimie]

Mihaela, please check to backport this next week.
We do want it down to ESR31.

Thanks :)

Comment 42

[Mihaela Velimiroviciu (:mihaelav)]

The patch applies successfully and the tests passes on all the other branches (beta, esr31, release)

Comment 43

[Andrei Eftimie]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #42)
> The patch applies successfully and the tests passes on all the other
> branches (beta, esr31, release)

This test is currently failing, so we disabled it in bug 1069897. We'll have to wait with backporting it until we figure out the failure.

I guess we could land it in a disabled state directly... but I wouldn't rush it, probably better to wait a bit for a fix.

Comment 44

[Andreea Matei [:AndreeaMatei]]

We want to figure out what's left here.

Comment 45

[Daniela Domnici]

The tests have passed on the following branches: beta, esr31, release. 
Here are the results:
Beta(local machine): http://mozmill-crowd.blargon7.com/#/remote/report/1b293578c647246d510f3b059f36544b 
ESR31(local machine): http://mozmill-crowd.blargon7.com/#/remote/report/1b293578c647246d510f3b059f3657cf
Release(local machine): http://mozmill-crowd.blargon7.com/#/remote/report/1b293578c647246d510f3b059f3afc50

Comment 46

[Daniela Domnici]

Attachment: esr31_V1

I`ve created the patch for esr31 branch.

Comment 47

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8538516 [details] [diff] [review]
esr31_V1

Review of attachment 8538516 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/50e2f739e996 (esr31)
Thanks Daniela.