795398 - Mozmill test to check that MD5 hash signatures are no longer accepted

Status: RESOLVED FIXED

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

Description

[u279076]

The following is a tracking bug to develop a Mozmill test to make sure that MD5 hash signatures are no longer accepted in Firefox. This is a regression test for bug 650355.

MozTrap: https://moztrap.mozilla.org/manage/case/1292/
1. Set security.enable_md5_signatures = false (should be default as of Firefox 16)
2. Open http://kuix.de/ca/nss-test-ca.php
> "Downloading Certificate" dialog should appear
3. Check the following on the "Downloading Certificate" dialog and click OK
* Trust this CA to identify websites
* Trust this CA to identify email users
* Trust this CA to identify software developers
4. Open https://kuix.de:9450
> "Untrusted Connection" error page appears
5. Expand "I Understand the Risks" section and click "Add Exception..."
> "Add Security Exception" dialog appears
6. Click "Confirm Security Exception"
> "Kai's Ultimate Information eXchange" webpage loads

Comment 1

[u279076]

Attachment: Initial Patch

I've written up an initial patch which tests this functionality. I think I should investigate moving the two test pages used by this test to mozqa.com. However I would like some feedback on this patch before going forward.

Thanks Henrik.

Comment 2

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

Comment on attachment 666123 [details] [diff] [review]
Initial Patch

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

Looks like a good start. Lets follow-up an my comments so we can have it as part of our remote (not functional) tests hopefully soon.

::: tests/functional/testSecurity/testMD5HashSignature.js
@@ +7,5 @@
> +var modalDialog = require("../../../lib/modal-dialog");
> +var prefs = require("../../../lib/prefs");
> +var utils = require("../../../lib/utils");
> +
> +const PREF_MD5_ENABLED = prefs.preferences.getPref("security.enable_md5_signatures", false);

As constant you will only set the preference name. The retrieval of the current value you will do in the test.

@@ +10,5 @@
> +
> +const PREF_MD5_ENABLED = prefs.preferences.getPref("security.enable_md5_signatures", false);
> +const REMOTE_TEST = {"ca":"http://kuix.de/ca/nss-test-ca.php",
> +                     "page":"https://kuix.de:9450",
> +                     "title":"kuix.de"};

As you have already mentioned I would appreciate when we can get this test ported to mozqa.com. Please check back with Kaie about the necessary certificate and content in nss-test-ca.php.

@@ +31,5 @@
> +  // Verify security.enable_md5_signatures = false by default
> +  // Set to false if not
> +  if (!expect.equal(PREF_MD5_ENABLED, false,
> +                    "Expected security.enable_md5_signatures to be false"))
> +    prefs.preferences.setPref("security.enable_md5_signatures", false);

If it's not set to default we should assert on it and not overwriting its value.

@@ +34,5 @@
> +                    "Expected security.enable_md5_signatures to be false"))
> +    prefs.preferences.setPref("security.enable_md5_signatures", false);
> +
> +  // Create a listener for the "Downloading Certificate" dialog
> +  var md = new modalDialog.modalDialog(controller.window .window);

Something messed-up here with the parameter.

@@ +53,5 @@
> +  md.start(handleAddSecurityExceptionDialog);
> +
> +  // Expand the "I Understand the Risks" section
> +  var expertContentHeading = new elementslib.ID(controller.window.document,
> +                                                "expertContent");

Are you accessing chrome or content here? It also doesn't seem to be used.

@@ +54,5 @@
> +
> +  // Expand the "I Understand the Risks" section
> +  var expertContentHeading = new elementslib.ID(controller.window.document,
> +                                                "expertContent");
> +  var expertContentButton = expertContentHeading.getNode().childNodes[1];

Is there no other way to access the button? I'm afraid of such a construct.

@@ +68,5 @@
> +
> +  // Verify the test page has been loaded
> +  controller.waitForPageLoad();
> +  assert.contain(controller.window.document.title, REMOTE_TEST["title"],
> +                 "Page with title '" + REMOTE_TEST["title"] + "' has been loaded");

In general you don't have to use so many comments. Especially not for expect and assert calls which have enough information in the message. For the latter simplify it and say e.g. "Target domain is present in the window title".

@@ +81,5 @@
> +  // Check the "Trust this CA to identify websites." checkbox
> +  var trustSSLCheckbox = new elementslib.ID(controller.window.document,
> +                                            "trustSSL");
> +  if (!trustSSLCheckbox.getNode().checked)
> +      controller.click(trustSSLCheckbox);

Those should always be not selected by default. So I don't see a reason for such a check. I'm sure we can directly click on them.

@@ +98,5 @@
> +        
> +  // Verify all "Trust" checkboxes are checked
> +  expect.equal(trustSSLCheckbox.getNode().checked, true);
> +  expect.equal(trustEmailCheckbox.getNode().checked, true);
> +  expect.equal(trustObjSignCheckbox.getNode().checked, true);

Add messages to each of those expect calls because only those will be visible in the report.

Comment 3

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

Lets get this work done as it has been started by Anthony. I hope you don't have anything against it? But I kinda would like to see this in our testsuite and you have more important tasks to work on.

Comment 4

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> Lets get this work done as it has been started by Anthony. I hope you don't
> have anything against it? But I kinda would like to see this in our
> testsuite and you have more important tasks to work on.

Not at all, I think this is the right move. I'm just happy I could be of help.

Comment 5

[Kai Engert (:KaiE:)]

I've been asked what's necessary to run the test on a different server.

I assume you want to continue to connect to my server at https://kuix.de:9450

In order to setup the required files for successful connecting, if you don't want to download the files from my server, here are the necessary details:


nss-test-ca.php contains:

<?php

header("Content-Type: application/x-x509-ca-cert");
include("nss-test-ca.pem");

?>


nss-test-ca.pem contains:

-----BEGIN CERTIFICATE-----
MIICTjCCAbegAwIBAgIBADANBgkqhkiG9w0BAQQFADBgMQswCQYDVQQGEwJERTET
MBEGA1UECBMKVGVzdCBTdGF0ZTERMA8GA1UEBxMIVGVzdCBMb2MxETAPBgNVBAoT
CFRlc3QgT3JnMRYwFAYDVQQDEw1LYWkncyBUZXN0IENBMB4XDTA2MDUwNzE1MzI0
NVoXDTE0MDUwNzE1MzI0NVowYDELMAkGA1UEBhMCREUxEzARBgNVBAgTClRlc3Qg
U3RhdGUxETAPBgNVBAcTCFRlc3QgTG9jMREwDwYDVQQKEwhUZXN0IE9yZzEWMBQG
A1UEAxMNS2FpJ3MgVGVzdCBDQTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA
vGbqVvGalFqCxwL6QiOI9iUtNjmy8eJ96oK2tgk0sLT3ZhEdZv13yKD2Iud+7lVo
CwlqiYPtH7Tow6bXEL+6hJIt6zbW15GCdX0Qg3Chfbu4tOl4Ebk/gXPvphZoJFnp
C4Gx2u+K0vKOJWxjvL+YvZuX5bkZK/Als9moOlaMe5cCAwEAAaMYMBYwFAYJYIZI
AYb4QgEBAQH/BAQDAgIEMA0GCSqGSIb3DQEBBAUAA4GBAAI5GvaHZmgY9pWfwG8p
2d6DUaUlLXYNwPD5asEnIlii2deUTU+tcF1G2Edf+31rXLVcJRIcaRrQpiIB+Q72
z/BXJStV2PoscafpHtaVD8Yq9NWu7I6VUGbAuHo1xNZSEl78ErN3APvnmRAnp21q
SMpNN1I8MixMXs6LlDdNYVE+
-----END CERTIFICATE-----


If you strive for independency of my https://kuix.de:9450 test, well, that's more complicated. Let me know if you are really willing to that path (which will involve setting up a permanent virtual host on an apache server).

Comment 6

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

Thanks Kai. We will most likely go forward with the setup on mozqa.com, which is hosting all of our testcases. Andreea, you should file a bug for that under Mozilla QA / Infrastructure.

Comment 7

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

Andreea, can you please check if the new host is correctly setup? I would like to see this regression test being implemented soon. Thanks.

Comment 8

[Andreea Matei [:AndreeaMatei]]

I have checked and talked to Kaie about it cause it looked like something was missing to me. The page works (https://ssl-md5-mozqa-zlb.vips.scl3.mozilla.com/) but no CA certificate was provided, that is requested by step 2 of this testcase, for downloading.
As soon as that is fixed, I'll have a patch ready for this test.

Comment 9

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

Thanks Andreea! I would suggest whenever you have a bit of time you can already check the initial patch from Anthony and update it accordingly. We can already do a full review process and update the TEST_URL in the patch once mozqa.com has been transitioned.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

Adding the test for review, using the kuix.de page, which will be updated later when the setup is ready.

Updated, removed some comments where the code was understandable by itself, replaced the trust checkboxes block with a forEach() to make it more compact and giving that is the same check for all of them.

Comment 11

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

Comment on attachment 737468 [details] [diff] [review]
patch v1

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

Some general things we can already fix as pointed out earlier. But for final code we have to wait until we have the real domain available and customize those pages. Comments for the review you can find inline.

::: tests/functional/testSecurity/manifest.ini
@@ +2,5 @@
>  [testEnablePrivilege.js]
>  [testGreenLarry.js]
>  [testGreyLarry.js]
>  [testIdentityPopupOpenClose.js]
> +[testMD5HashSignature.js]

This works with remote content and will never live locally. So please add it to the remote testrun.

::: tests/functional/testSecurity/testMD5HashSignature.js
@@ +8,5 @@
> +var prefs = require("../../../lib/prefs");
> +
> +const CERTIFICATE_URL = {ca : "http://kuix.de/ca/nss-test-ca.php",
> +                         url : "https://kuix.de:9450/",
> +                         title : "kuix.de"};

Move `ca` to the next line and indent by 2 chars please. Also can we rename the constant to TEST_URLS and move the title out to its own constant?

@@ +18,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +
> +  // Make sure the preference is set to false
> +  prefs.preferences.setPref(PREF_MD5_ENABLED, false);

We wont have to set the pref given that it is disabled by default now. I'm sure there is an unit test which checks that it is set this way.

@@ +43,5 @@
> +  md.start(handleAddSecurityExceptionDialog);
> +
> +  // Expand the "I Understand the Risks" section
> +  var expertContentHeading = new elementslib.ID(controller.window.document,
> +                                                "expertContent");

One of our rules is to not refer to chrome elements directly but put those into an ui library.

@@ +73,5 @@
> +                                              "trustEmail");
> +  var trustObjSignCheckbox = new elementslib.ID(aController.window.document,
> +                                                "trustObjSign");
> +
> +  var trustElements = [trustSSLCheckbox, trustEmailCheckbox, trustObjSignCheckbox];

I would define a list with the elements ids at the top of the method and iterate over those.

Comment 12

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> > +  md.start(handleAddSecurityExceptionDialog);
> > +
> > +  // Expand the "I Understand the Risks" section
> > +  var expertContentHeading = new elementslib.ID(controller.window.document,
> > +                                                "expertContent");
> 
> One of our rules is to not refer to chrome elements directly but put those
> into an ui library.
It would be nice to have a security.js library giving that that we have several elements throughout all the security tests. I'm happy to work on it on a separate bug until this one gets unblocked.

Also related to moving this test in the remote testrun, should we do the same with the other security tests that use external urls? I see that are 11 out of 14:
* testDVCertificate.js
* testGreenLarry.js
* testMixedContentPage.js
* testSSLDisabledErrorPage.js
* testSafeBrowsingNotificationBar.js
* testSafeBrowsingWarningPages.js
* testSecurityInfoViaMoreInformation.js
* testSecurityNotification.js
* testSubmitUnencryptedInfoWarning.js
* testUnknownIssuer.js
* testUntrustedConnectionErrorPage.js

Comment 13

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #12)
> (In reply to Henrik Skupin (:whimboo) from comment #11)
> > > +  md.start(handleAddSecurityExceptionDialog);
> > > +
> > > +  // Expand the "I Understand the Risks" section
> > > +  var expertContentHeading = new elementslib.ID(controller.window.document,
> > > +                                                "expertContent");
> > 
> > One of our rules is to not refer to chrome elements directly but put those
> > into an ui library.
> It would be nice to have a security.js library giving that that we have
> several elements throughout all the security tests. I'm happy to work on it
> on a separate bug until this one gets unblocked.

That would be perfect. But we might have to delay that a bit given the Mozmill 2.0 work. If you have a bit spare time feel free to get started on it.

> Also related to moving this test in the remote testrun, should we do the
> same with the other security tests that use external urls? I see that are 11
> out of 14:

All that should be covered by bug 849962.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Moved the test in the remote folder as requested (as an update, I think maybe Olek will be interested in moving all related tests there in bug 849962) and we're now using the correct pages.
Reports 1.5:
Linux: http://mozmill-crowd.blargon7.com/#/remote/report/a1b02004612785c13cf7c6bf1e03fbd7
OS X: http://mozmill-crowd.blargon7.com/#/remote/report/a1b02004612785c13cf7c6bf1e0407ca
Windows: http://mozmill-crowd.blargon7.com/#/remote/report/a1b02004612785c13cf7c6bf1e04ddd3

Mozmill 2.0:
Linux: http://mozmill-crowd.blargon7.com/#/remote/report/a1b02004612785c13cf7c6bf1e04cda9
OS X: http://mozmill-crowd.blargon7.com/#/remote/report/a1b02004612785c13cf7c6bf1e04d7cd
Windows: http://mozmill-crowd.blargon7.com/#/remote/report/a1b02004612785c13cf7c6bf1e04fc32

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 768856 [details] [diff] [review]
patch v2

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

::: tests/remote/testSecurity/testMD5HashSignature.js
@@ +6,5 @@
> +var { assert, expect } = require("../../../lib/assertions");
> +var modalDialog = require("../../../lib/modal-dialog");
> +var prefs = require("../../../lib/prefs");
> +
> +const TEST_URLS = {

I think we should use TEST_DATA here.

@@ +11,5 @@
> +  ca : "http://mozqa.com/data/firefox/security/certificates/md5/importSSL.php",
> +  url : "https://ssl-md5.mozqa.com/"
> +}
> +
> +const TEST_URL_TITLE = "Untrusted Connection";

This could be added to TEST_DATA above with the key "title".

@@ +22,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +}
> +
> +function teardownModule(aModule) {
> +  prefs.preferences.clearUserPref(PREF_MD5_ENABLED);

We're no longer changing this preference, so we can remove this, and the constant for the preference name.

@@ +42,5 @@
> +  // Add a listener for the "Add Security Exception" dialog
> +  md.start(handleAddSecurityExceptionDialog);
> +
> +  // Expand the "I Understand the Risks" section
> +  var expertContentHeading = new elementslib.ID(controller.window.document,

Could we raise a separate bug for creating the suggested security.js UI library? Once done we should either block this bug or add a TODO for updating this test once the UI lib exists.

@@ +65,5 @@
> + *        MozMillController of the window to operate on
> + */
> +function handleDownloadingCertificateDialog(aController) {
> +  // Get all trusting options and make sure are checked
> +  var trustElements = ["trustSSL", "trustEmail",  "trustObjSign"];

Nit: extra whitespace

Comment 16

[Andreea Matei [:AndreeaMatei]]

(In reply to Dave Hunt (:davehunt) from comment #15)
> @@ +11,5 @@
> > +  ca : "http://mozqa.com/data/firefox/security/certificates/md5/importSSL.php",
> > +  url : "https://ssl-md5.mozqa.com/"
> > +}
> > +
> > +const TEST_URL_TITLE = "Untrusted Connection";
> 
> This could be added to TEST_DATA above with the key "title".
It was before, but Henrik asked in comment 11 to be moved separate.

> > +  // Expand the "I Understand the Risks" section
> > +  var expertContentHeading = new elementslib.ID(controller.window.document,
> 
> Could we raise a separate bug for creating the suggested security.js UI
> library? Once done we should either block this bug or add a TODO for
> updating this test once the UI lib exists.

Sure, it's filed as bug 863139, as we talked in Ask an expert, we shouldn't be blocked by this giving that all security tests have elements in and that bug has lower priority, I'll add the TODO comment.

Thanks, I'll update it as soon as possible.

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Andreea Matei [:AndreeaMatei] from comment #16)
> (In reply to Dave Hunt (:davehunt) from comment #15)
> > @@ +11,5 @@
> > > +  ca : "http://mozqa.com/data/firefox/security/certificates/md5/importSSL.php",
> > > +  url : "https://ssl-md5.mozqa.com/"
> > > +}
> > > +
> > > +const TEST_URL_TITLE = "Untrusted Connection";
> > 
> > This could be added to TEST_DATA above with the key "title".
> 
> It was before, but Henrik asked in comment 11 to be moved separate.

Hmm, okay. This seems inconsistent to me given our move to TEST_DATA.

Comment 18

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2.1

Updated as requested.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 769682 [details] [diff] [review]
patch v2.1

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

The test is passing, but I don't believe it's doing everything that it should be. For me, the exception is not added and I do not land on the correct page.

::: tests/remote/testSecurity/testMD5HashSignature.js
@@ +10,5 @@
> +  ca : "http://mozqa.com/data/firefox/security/certificates/md5/importSSL.php",
> +  url : "https://ssl-md5.mozqa.com/"
> +}
> +
> +const TEST_DATA_TITLE = "Untrusted Connection";

I still think we should move this into TEST_DATA. Could you go ahead and do that? Also, this should be the title of the page after the exception has been added. In this case, it would be "mozqa.com"

@@ +90,5 @@
> +  var exceptionButton = new elementslib.Lookup(aController.window.document,
> +                                               '/id("exceptiondialog")' +
> +                                               '/anon({"anonid":"buttons"})' +
> +                                               '/{"dlgtype":"extra1"}');
> +  aController.waitThenClick(exceptionButton);

For me, this button appears to be clicked while the dialog still says "Attempting to verify the site..." and the button is disabled. We should first wait for this button to be enabled before attempting to click on it.

Comment 20

[Andreea Matei [:AndreeaMatei]]

You're right, thanks for catching that. Still I have a strange behavior going on here, cause even if I wait 25 seconds for the button to be available, the site remains under verification. The button never gets visible.

But manually accessing the pages, I get it verified in 2 seconds, just under Mozmill is not working, tried with 2.0 as well, on Linux and OS X. Keep looking.

Comment 21

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3

Updated and it works fine now.

Linux: http://mozmill-crowd.blargon7.com/#/remote/report/5aa1ca5e7015e3740d269dc94734b37e
OS X: http://mozmill-crowd.blargon7.com/#/remote/report/5aa1ca5e7015e3740d269dc947356511
Windows: http://mozmill-crowd.blargon7.com/#/remote/report/5aa1ca5e7015e3740d269dc9473678bd

Tested with 2.0 as well.

Comment 22

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

Comment on attachment 774620 [details] [diff] [review]
patch v3

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

Looks mostly fine. Just some nits to fix.

::: tests/remote/testSecurity/testMD5HashSignature.js
@@ +6,5 @@
> +var { assert, expect } = require("../../../lib/assertions");
> +var modalDialog = require("../../../lib/modal-dialog");
> +
> +const TEST_DATA = {
> +  ca : "http://mozqa.com/data/firefox/security/certificates/md5/importSSL.php",

Please give that property a better name. Given that we have two URLs what about url_ca and url_page?

@@ +29,5 @@
> +
> +  md.waitForDialog(TIMEOUT_MODAL_DIALOG);
> +
> +  // Add a listener for the "Add Security Exception" dialog
> +  md.start(handleAddSecurityExceptionDialog);

Please move this down right before we trigger the modal dialog. This location is a bit confusing.

@@ +38,5 @@
> +  // TODO: Move all elements to the Security shared module
> +  // Expand the "I Understand the Risks" section
> +  var expertContentHeading = new elementslib.ID(controller.tabs.activeTab,
> +                                                "expertContent");
> +  var expertContentButton = expertContentHeading.getNode().childNodes[1];

I don't like that we make use of childNodes here. Can we change it to use NodeCollector with a specific anonymous id?

@@ +51,5 @@
> +
> +  // Verify the test page has been loaded
> +  controller.waitForPageLoad();
> +  assert.contain(controller.window.document.title, TEST_DATA.title,
> +                 "Page has been loaded");

I would better check for an element on that page.

@@ +76,5 @@
> +  // Click the "OK" button to close the dialog
> +  var acceptButton = new elementslib.Lookup(aController.window.document,
> +                                            '/id("download_cert")' +
> +                                            '/anon({"anonid":"buttons"})' +
> +                                            '/{"dlgtype":"accept"}');

Just a note: We should have a base class here to handle the default buttons, and which will be sub-classed by e.g the security ui dialog classes.

@@ +93,5 @@
> +                                               '/anon({"anonid":"buttons"})' +
> +                                               '/{"dlgtype":"extra1"}');
> +  assert.waitFor(function() {
> +    return !exceptionButton.getNode().disabled;
> +  }, "Add exception button is available");

Can we turn off the countdown similar to installing add-ons? I would appreciate that.

Comment 23

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

What's missing here? It's in a limbo since end of July.

Comment 24

[Andreea Matei [:AndreeaMatei]]

When I moved the line for the modal dialog start down right before we do the action to open it, I came across the same issue with the site not getting verified in the modal dialog. 

I looked over some event listeners/observer to wait for the certificate to be ready as it appears it's a timing issue there. Manually I don't see the problem, but the test moves too fast.

Comment 25

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

So please figure out the next actions which have to happen on that bug so we can get this test landed. Thanks.

Comment 26

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> So please figure out the next actions which have to happen on that bug so we
> can get this test landed. Thanks.

+1, we are still forced to run this test manually in Beta until the automated test is in place which takes time away from more valuable tasks.

Comment 27

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> @@ +29,5 @@
> > +
> > +  md.waitForDialog(TIMEOUT_MODAL_DIALOG);
> > +
> > +  // Add a listener for the "Add Security Exception" dialog
> > +  md.start(handleAddSecurityExceptionDialog);
> 
> Please move this down right before we trigger the modal dialog. This
> location is a bit confusing.
To add an update here of what I worked so far:
When I move this down, the site doesn't get verified in the modal dialog. At first I thought it's regarding the toggle after "I understand the risks" click, to wait enough for it to be completed. But it's not that, cause it's not working with a sleep there, only with a sleep before the click that triggers the modal dialog. I haven't found any attributes or css properties for that button to be any different from when the content is collapsed.

Another thing that is strange, but works when moving that line down before the click, is to speed-up the delay from nsITimer (DELAY_CHECK from modal-dialog.js) from 100ms to 30ms (above that fails). Or on the other hand, to increase it somewhere at 1000.

Comment 28

[Andreea Matei [:AndreeaMatei]]

Kai, do you happen to know if I can wait for any event so the certificate or the site to be ready for the exception adding part? I believe I'm missing something.
Our test runs too quickly so without a sleep, sometimes the dialog for adding the exception gets stuck at:
"Attempting to identify the site..." and the "Confirm Security Exception" button remains disabled.
We don't want to make use of sleep calls, that's why I ask. Thanks.

Comment 29

[Kai Engert (:KaiE:)]

I don't know

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.1

Hi, I investigated this today and I found out that for some reason the modall-dialog handler interference with the asynchronous fetch of the certificate.
http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/exceptionDialog.js#65

-As a solution I propose we fetch the certificate via "Get Certificate" button, and disabled the prefetch by setting the preference "browser.ssl_override_behavior" to 2.
http://kb.mozillazine.org/Browser.ssl_override_behavior
-We can also initialize the observer right away without a delay.
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/modal-dialog.js#l173

Reports:
http://mozmill-crowd.blargon7.com/#/remote/report/850f11101678d9fbefeb95e0bd541d6b
http://mozmill-crowd.blargon7.com/#/remote/report/850f11101678d9fbefeb95e0bd540ebc
http://mozmill-crowd.blargon7.com/#/remote/report/850f11101678d9fbefeb95e0bd540522

Comment 31

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

(In reply to Cosmin Malutan from comment #30)
> -As a solution I propose we fetch the certificate via "Get Certificate"
> button, and disabled the prefetch by setting the preference
> "browser.ssl_override_behavior" to 2.

Sounds fine to me but then it is clearly not 2, given that this enabled pre-fetching!

> 2
> Pre-populate the current URL and pre-fetch the certificate. 

> -We can also initialize the observer right away without a delay.
> http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/modal-dialog.
> js#l173

Oh, great catch! We shouldn't have set a delay for the first timer. So lets do that route.

Comment 32

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

Comment on attachment 8357180 [details] [diff] [review]
patch v3.1

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

f+ but with the proposed change to the modal dialog class.

::: firefox/tests/remote/testSecurity/testMD5HashSignature.js
@@ +6,5 @@
> +var { assert, expect } = require("../../../../lib/assertions");
> +var modalDialog = require("../../../lib/modal-dialog");
> +var prefs = require("../../../lib/prefs");
> +
> +const SSL_BEHAVIOR = "browser.ssl_override_behavior";

As said, not with this preference.

Comment 33

[Cosmin Malutan, [:cosmin-malutan]]

Andreea, would you update the patch since this is your bug?
Thanks.

Comment 34

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.2

Updated the patch. Thanks Cosmin!
Reports:
http://mozmill-crowd.blargon7.com/#/remote/report/c60df2760f166bbd2e88b871d33468d3
http://mozmill-crowd.blargon7.com/#/remote/report/c60df2760f166bbd2e88b871d3346835
http://mozmill-crowd.blargon7.com/#/remote/report/c60df2760f166bbd2e88b871d33404fe

Comment 35

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

Comment on attachment 8359277 [details] [diff] [review]
patch v3.2

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

::: firefox/lib/modal-dialog.js
@@ +167,5 @@
>    start : function modalDialog_start(aCallback) {
>      assert.ok(aCallback, arguments.callee.name + ": Callback has been specified.");
>      this._observer = new mdObserver(this._window, aCallback);
>      this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +    this._timer.init(this._observer, undefined, Ci.nsITimer.TYPE_ONE_SHOT);

This is a change I really don't want to see in this patch. It might be related because it is necessary but it should really be handled in a separate bug because it is necessary across all of our branches. The new test will only land on default.

Also the delay should be `0` and not undefined.

Comment 36

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.3

Updated for default as the dependency bug was landed for that.

Comment 37

[Andrei Eftimie]

Comment on attachment 8361066 [details] [diff] [review]
patch v3.3

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

We should not use Lookup Expressions anymore. See inline.

::: firefox/tests/remote/testSecurity/testMD5HashSignature.js
@@ +8,5 @@
> +
> +const TEST_DATA = {
> +  url_ca : "http://mozqa.com/data/firefox/security/certificates/md5/importSSL.php",
> +  url_page : "https://ssl-md5.mozqa.com",
> +  link: "http://quality.mozilla.org"

nit: consistent spacing

@@ +77,5 @@
> +  // Click the "OK" button to close the dialog
> +  var acceptButton = new elementslib.Lookup(aController.window.document,
> +                                            '/id("download_cert")' +
> +                                            '/anon({"anonid":"buttons"})' +
> +                                            '/{"dlgtype":"accept"}');

Ideally we should not be using Lookup expressions in new code anymore.
We _could_ leave this to be changed later on as part of bug 863139, but I'm weary that it might take a long time given the current backlog to address this.

It would be great if we would have this done properly here.

Comment 38

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4

Updated with nodeCollector in the 2 cases I used lookup.

Comment 39

[Andrei Eftimie]

Comment on attachment 8361608 [details] [diff] [review]
patch v4

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

Looks good.
r+ from me

Leaving Henrik's flag up if we want's to have a final review here.

Comment 40

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

(In reply to Andrei Eftimie from comment #37)
> Ideally we should not be using Lookup expressions in new code anymore.
> We _could_ leave this to be changed later on as part of bug 863139, but I'm
> weary that it might take a long time given the current backlog to address

I have marked it as mentored by Andreea, so we hopefully can see it implemented sooner.

Comment 41

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

Comment on attachment 8361608 [details] [diff] [review]
patch v4

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

r=me with a huge exception as given below. Please make sure to update the list of reviewers before landing the patch.

::: firefox/tests/remote/testSecurity/testMD5HashSignature.js
@@ +33,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Bug 863139
> +  // TODO: Move all elements to the Security shared module
> +  // Expand the "I Understand the Risks" section

Huge exception here from my side by not complaining about it. Whatever test we implement element access should happen via a lib. So lets hope we get this soon. At least for the next security test I would like to see this. Might be Daniel could have a hand on this ui module?

Comment 42

[Andrei Eftimie]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/8cf4358c4892 (default)

Indeed bug 863139 is probably a good one right now for Daniel.
But lets move the discussion for it over there.

Comment 43

[Andreea Matei [:AndreeaMatei]]

We need to backport this to aurora and beta, in order to be useful for QA now. Will check it in a bit.

Comment 44

[Andreea Matei [:AndreeaMatei]]

Attachment: [aurora, beta] patch v1

Patch for aurora and beta. Didn't apply cleanly from default because of the manifest file.

Comment 45

[Andrei Eftimie]

Comment on attachment 8362485 [details] [diff] [review]
[aurora, beta] patch v1

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

Looks good.

Lets land the backports tomorrow so we'll have a day of testruns with the new test on default.

Comment 46

[Andreea Matei [:AndreeaMatei]]

No failures, pushed to:
http://hg.mozilla.org/qa/mozmill-tests/rev/12d03ab18235 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/b1eada22cdf9 (beta)