Refactor getCertificate() security's lib function, in order to fix 'SSLStatus is null' js type error in security tests

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Daniel Gherasim, Assigned: Teodor Druta)

Tracking

unspecified

Firefox Tracking Flags

(firefox35 ?, firefox36 ?, firefox37 fixed, firefox38 fixed)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Module:    remote
Test:      testSecurity/testGreenLarry.js    
Failure:   securityUI.QueryInterface(...).SSLStatus is null
Branches:  Aurora de
Platforms: Ubuntu 1310 (mm-ub-1310-32-2)

http://mozmill-daily.blargon7.com/#/remote/report/6959f9a8236961096763389083e90108

Run the individial test 50 times & it didn't reproduce.
It failed 50 times since the bug was filed:
(Assignee)

Comment 2

4 years ago
Created attachment 8560384 [details] [diff] [review]
fixgreenlarry.patch

This test will continue to fail from time to time in the future, that is because we don't handle correctly nsISSLStatusProvider object, we need to wait for it to be "ready" before retrieving the server certificate.

For reference please look: https://groups.google.com/d/msg/mozilla.dev.extensions/APC3lvXaMqA/hIPa4zGCPokJ

And an example from MDN of how this should be handled:
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/How_to_check_the_secruity_state_of_an_XMLHTTPRequest_over_SSL

We have two more tests that are using the nsISSLStatusProvider object to retrieve the server certificate I think we should update all of them. We will need to update the getCertificate function the security.js lib, and use that in our tests, I'll file a new bug for that in case I'll get f+.
Assignee: nobody → teodor.druta
Status: NEW → ASSIGNED
Attachment #8560384 - Flags: feedback?(hskupin)
Comment on attachment 8560384 [details] [diff] [review]
fixgreenlarry.patch

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

I don't see the need for an extra bug. Just update this test so it makes use of the security.js method. It's better to change its parameter to expect the current browser. Instead of going through the global gBrowser object, can't we simply hand over controller.tabs.activeTab? The security method should take care of waiting. Once that landed and proved to be working the other two tests can be updated - I assume there are existing bugs for them? If not I would be even ok in fixing them all here.

Your explanation and the references don't show me why we have to wait, but if that works I'm fine with it.

::: firefox/tests/remote/testSecurity/testGreenLarry.js
@@ +41,5 @@
>  
>    // Get the information from the certificate for comparison
>    var securityUI = controller.window.getBrowser().mCurrentBrowser.securityUI;
> +  assert.waitFor(() => (securityUI instanceof Ci.nsISSLStatusProvider),
> +                 "nsISSLStatusProvider object is ready");

What is the value of securityUI when it is not ready?
Attachment #8560384 - Flags: feedback?(hskupin) → feedback+
(Assignee)

Comment 4

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #3)
> What is the value of securityUI when it is not ready?

I did a dump on a browser instance without secure connection: [xpconnect wrapped nsISecureBrowserUI]
With secure connection: [xpconnect wrapped (nsISupports, nsISecureBrowserUI, nsISSLStatusProvider)]

Actually I investigated this more and I think that my proposed solution may not be the right one afterall, between our tests in the remote testrun we open secure pages anyway, even if we are not testing them, so:
> securityUI instanceof Ci.nsISSLStatusProvider
will kinda 99.9% of the time return true. Even when the test is ran stand-alone (because of the ffx firstrun page).

So I looked through some firefox code to see how the dev team is handling it:
https://dxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js#268
https://dxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.js#114

It's the same thing as we do now in the getCertificate() function here:
http://hg.mozilla.org/qa/mozmill-tests/file/50fa044102fd/firefox/lib/security.js#l22
But it's not helping us much because instead of a failure we get null and a javacript error.

What I actually suggest to do here in order to eliminate all these 'is null' javascript errors would be to add a check for the getCertificate() function rather than returning null, so instead of this line in security.js (l22):
> return SSLStatus ? SSLStatus.serverCert : null;
we should have something like this:
> assert.waitFor(() => !!SSLStatus, "SSLStatus is not null");
> return SSLStatus.serverCert;
Flags: needinfo?(hskupin)
I think that makes sense. Do you know how long we might have to wait until SSLStatus is known? I don't want to wait forever but also not too short. Also what about the serializable code the browser itself is using? Shouldn't we do that too?
Flags: needinfo?(hskupin)
(Assignee)

Updated

4 years ago
status-firefox35: --- → ?
status-firefox36: --- → ?
status-firefox37: --- → ?
status-firefox38: --- → affected
Summary: Test failure 'securityUI.QueryInterface(...).SSLStatus is null' in testSecurity/testGreenLarry.js → Refactor getCertificate() security's lib function, in order to fix 'SSLStatus is null' js type error in security tests
(Assignee)

Comment 6

4 years ago
Created attachment 8562037 [details] [diff] [review]
refactor_get_cert.patch

(In reply to Henrik Skupin (:whimboo) from comment #5)
> I think that makes sense. Do you know how long we might have to wait until
> SSLStatus is known? I don't want to wait forever but also not too short.

It shouldn't take more than a few seconds, which explains the low failure rate of these security tests, but I suggest that we should have a timeout for about 10s just to be sure.

> Also what about the serializable code the browser itself is using? Shouldn't
> we do that too?

Unfortunately the documentation on this matter is pretty tight, I can't say for sure. Still I think we should check for the nsISSLStatusProvider interface.
I have included it in the getCertificate() function.
Attachment #8560384 - Attachment is obsolete: true
Attachment #8562037 - Flags: review?(mihaela.velimiroviciu)
Attachment #8562037 - Flags: review?(andreea.matei)
Attachment #8562037 - Flags: feedback?(hskupin)
Comment on attachment 8562037 [details] [diff] [review]
refactor_get_cert.patch

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

Just some nits, otherwise I think it's fine.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +9,5 @@
>  // Include necessary modules
>  var { assert, expect } = require("../../../../lib/assertions");
>  var utils = require("../../../../lib/utils");
>  var windows = require("../../../../lib/windows");
> +var security = require("../../../lib/security");

This lib should be placed before utils

::: firefox/tests/remote/testSecurity/testGreenLarry.js
@@ +9,5 @@
>  // Include necessary modules
>  var { assert, expect } = require("../../../../lib/assertions");
>  var utils = require("../../../../lib/utils");
>  var windows = require("../../../../lib/windows");
> +var security = require("../../../lib/security");

This lib should be placed before utils

::: firefox/tests/remote/testSecurity/testSecurityInfoViaMoreInformation.js
@@ +6,5 @@
>  
>  // Include necessary modules
>  var { assert, expect } = require("../../../../lib/assertions");
>  var windows = require("../../../../lib/windows");
> +var security = require("../../../lib/security");

This lib should be placed before windows
Attachment #8562037 - Flags: review?(mihaela.velimiroviciu) → review+
Comment on attachment 8562037 [details] [diff] [review]
refactor_get_cert.patch

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

Given that you already asked for review, this patch looks to be out of the WIP phase. So I will also give a review. Please see my inline comments.

::: firefox/lib/security.js
@@ +18,5 @@
>   * @returns {object} Certificate of the current browsing page, or null if no
>   *                   certificate exists
>   */
>  function getCertificate(aSecurityUI) {
> +  assert.ok((aSecurityUI instanceof Ci.nsISSLStatusProvider),

nit: There is no need for additional brackets here.

@@ +25,2 @@
>    var SSLStatus = aSecurityUI.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus;
> +  assert.waitFor(() => !!SSLStatus, "SSLStatus is ready", SSLSTATUS_TIMEOUT);

For readability and conformance with other code please move the message and timeout parameter to the next line.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +41,5 @@
>    controller.waitForPageLoad();
>  
>    // Get the information from the certificate
> +  var securityUI = browserWindow.tabs.securityUI;
> +  cert = security.getCertificate(securityUI);

Kill the extra variable as it looks like it is not used anywhere else. This also applies to all the other tests.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +78,5 @@
>      locationBar.waitForProxyState();
> +    // There is no security certificate for ssl-ov pages
> +    if (aIndex !== 2) {
> +      cert = security.getCertificate(tabBrowser.securityUI);
> +    }

This is clearly something we have to check! This is a SSL page and should clearly have a certificate. With that not having sorted out, we should not land this patch.

Also this makes the test somewhat broken given that the security checks would use the certificate from the EV site.
Attachment #8562037 - Flags: review?(andreea.matei)
Attachment #8562037 - Flags: review-
Attachment #8562037 - Flags: feedback?(hskupin)
(Assignee)

Comment 9

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #8)
> This is clearly something we have to check! This is a SSL page and should
> clearly have a certificate. With that not having sorted out, we should not
> land this patch.
>

I think the comment I have added may be somewhat misleading, actually that's not a SSL page it's plain http -> http://hg.mozilla.org/qa/mozmill-tests/file/195befdcc229/firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js#l28

I don't really know if this is how it supposed to be, if not then we may need to fix this as well.
Totally! As the test name is telling me "testSSLStatusAfterRestart" we test certificates for all known types. The OV one should have been broken for a long time then. Have we seen that somewhere? And if not, why it was not detected?
(Assignee)

Comment 11

4 years ago
I think this was missed. It seems that setting the url to https://ssl-ov.mozqa.com adds new failures, this test may need a little bit of refactoring, I'll look into it and will handle those in the same patch I think.
Sounds good. Thanks.
(Assignee)

Comment 13

4 years ago
Created attachment 8562648 [details] [diff] [review]
refactor_get_cert_v2.patch

Updated the SSLStatusAfterRestart test to check for the correct ssl-ov page and certificate. Fixed all reported nits.
Attachment #8562037 - Attachment is obsolete: true
Attachment #8562648 - Flags: review?(mihaela.velimiroviciu)
Attachment #8562648 - Flags: review?(andreea.matei)
Attachment #8562648 - Flags: review?(mihaela.velimiroviciu) → review+
Attachment #8562648 - Flags: review?(hskupin)
Attachment #8562648 - Flags: review?(andreea.matei)
Attachment #8562648 - Flags: review+
Comment on attachment 8562648 [details] [diff] [review]
refactor_get_cert_v2.patch

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

r=me with the comment fix.

::: firefox/lib/security.js
@@ +19,5 @@
>   *                   certificate exists
>   */
>  function getCertificate(aSecurityUI) {
> +  assert.ok(aSecurityUI instanceof Ci.nsISSLStatusProvider,
> +            "Ci.nsISSLStatusProvider interface is initialized");

What buys us this assert? If it is a check that the right object has been passed in, you should update the comment.
Attachment #8562648 - Flags: review?(hskupin) → review+
(Assignee)

Comment 15

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #14)
> What buys us this assert? If it is a check that the right object has been
> passed in, you should update the comment.

My intention for this was actually to catch any situation in which the securityUI object would not be an instance of the Ci.nsISSLStatusProvider, which could mean that there is actually something wrong with the firefox's browser securityUI object or there is no open SSL connection and there is something wrong with the test, but on second thought the possibility of this should be very low, so I think we could use the assert for checking if the right object type has been passed in. I'll update the comment.

A situation I can think of that will make this assert not pass will be:
testA.js -> uses closeAllTabs() in teardownModule.
testB.js -> starts with about:newtab, opens a secure page, something went wrong and actually the securityUI is not an instance of Ci.nsISSLStatusProvider.
But imo this is not really the case in the current situation of the remote testSecurity tests.
(Assignee)

Comment 16

4 years ago
Created attachment 8563303 [details] [diff] [review]
refactor_get_cert_v2.1.patch

Updated the comment for the assertion.
Attachment #8562648 - Attachment is obsolete: true
Attachment #8563303 - Flags: review?(mihaela.velimiroviciu)
Attachment #8563303 - Flags: review?(andreea.matei)
Attachment #8563303 - Flags: review?(mihaela.velimiroviciu) → review+
Comment on attachment 8563303 [details] [diff] [review]
refactor_get_cert_v2.1.patch

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

https://hg.mozilla.org/qa/mozmill-tests/rev/f99c8c6ae9fc (default)
Attachment #8563303 - Flags: review?(andreea.matei) → review+
status-firefox38: affected → fixed
https://hg.mozilla.org/qa/mozmill-tests/rev/99ae80464f46 (aurora)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox37: ? → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.