1100687 - Fix browser_addCertException.js and browser_blockHPKP.js to work under e10s

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[:Gijs (he/him)]

Currently it tries to access the button in an executeSoon off STATE_STOP, which doesn't work in e10s (not 100% sure why, but I suspect the progress listener magic doesn't work the same way as in non-e10s - in any case, button is null when the callback fires). Hence, the test times out.

However, executing button.click() from the console also seems to do precious little. Manually opening the dialog also doesn't trigger the other code that is meant to wait for this dialog, so there may be a sequence of issues here...

Comment 1

[:Gijs (he/him)]

blockHPKP.js has the same issues and fails with:

0 INFO *** Start BrowserChrome Test Results ***
1 INFO checking window state
2 INFO TEST-START | chrome://mochitests/content/browser/browser/base/content/test/general/browser_blockHPKP.js
3 INFO Wait tab event: load
4 INFO Tab event received: load
5 INFO Console message: [JavaScript Error: "textElement is null" {file: "chrome://mochitests/content/browser/browser/base/content/test/general/browser_blockHPKP.js" line: 75}]
6 INFO Console message: [JavaScript Error: "textElement is null" {file: "chrome://mochitests/content/browser/browser/base/content/test/general/browser_blockHPKP.js" line: 75}]

Just like browser_addException.js, trying to get the button from content doesn't seem to work here, and the test grinds to a halt and times out.

I actually suspect that maybe it's the shim instead of the state listener? Well, either way, it doesn't work. :-)

Comment 2

[Chris Peterson [:cpeterson]]

e10s test bugs should block tracking-e10s=+

Comment 3

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1100687 - part 1: fix browser_addCertException.js, r?jaws

Review commit: https://reviewboard.mozilla.org/r/36849/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36849/

Comment 4

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1100687 - part 2: fix browser_blockHPKP.js, r?jaws

Review commit: https://reviewboard.mozilla.org/r/36851/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36851/

Comment 5

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/36849/#review33411

::: browser/base/content/test/general/browser_addCertException.js:113
(Diff revision 1)
> +function promiseErrorPageLoaded(browser) {

Copy-pasted from browser_ssl_error_reports.js, fwiw. Could potentially stick this in head.js instead?

::: browser/base/content/test/general/head.js:961
(Diff revision 1)
> -  ok(is_visible(element), msg);
> +  ok(is_visible(element), msg || "Element should be visible");

Not strictly necessary, but if you use `is_element_visible` with no second argument, you get a 'no assertion msg' or something in the log, and so if these are ever intermittent, it'll just be non-descript, so I fixed this in these functions while I was here.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8724078 [details]
MozReview Request: Bug 1100687 - part 1: fix browser_addCertException.js, r?jaws

https://reviewboard.mozilla.org/r/36849/#review33459

::: browser/base/content/test/general/browser_addCertException.js:20
(Diff revision 1)
>    gBrowser.selectedBrowser.loadURI("https://expired.example.com");

This should use BrowserTestUtils.loadURI

::: browser/base/content/test/general/browser_addCertException.js:113
(Diff revision 1)
> +function promiseErrorPageLoaded(browser) {
> +  return new Promise(resolve => {
> +    browser.addEventListener("DOMContentLoaded", function onLoad() {
> +      browser.removeEventListener("DOMContentLoaded", onLoad, false, true);
> +      resolve();
> +    }, false, true);
> +  });

Why not just use BrowserTestUtils.browserLoaded in place of this?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8724079 [details]
MozReview Request: Bug 1100687 - part 2: fix browser_blockHPKP.js, r?jaws

https://reviewboard.mozilla.org/r/36851/#review33461

::: browser/base/content/test/general/browser_blockHPKP.js:87
(Diff revision 1)
> -    gBrowser.selectedBrowser.loadURI("https://" + kBadPinningDomain);
> +  gBrowser.selectedBrowser.loadURI("https://" + kBadPinningDomain);

Pleae use BrowserTestUtils.loadURI here and other places in this file.

::: browser/base/content/test/general/browser_blockHPKP.js:99
(Diff revision 1)
> +function promiseErrorPageLoaded(browser) {
> +  return new Promise(resolve => {
> +    browser.addEventListener("DOMContentLoaded", function onLoad() {
> +      browser.removeEventListener("DOMContentLoaded", onLoad, false, true);
> +      resolve();
> +    }, false, true);
> +  });

You used BrowserTestUtils.browserLoaded above on line 86, why do we need to add this function?

Comment 8

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> You used BrowserTestUtils.browserLoaded above on line 86, why do we need to
> add this function?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Why not just use BrowserTestUtils.browserLoaded in place of this?

Because error pages do not fire the "load" event, and so we need ugly hacks like this.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/083380e8d36f
https://hg.mozilla.org/integration/fx-team/rev/2fa086d51b6a

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/083380e8d36f
https://hg.mozilla.org/mozilla-central/rev/2fa086d51b6a