Closed Bug 1100687 Opened 6 years ago Closed 4 years ago

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

Categories

(Firefox :: Security, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
e10s + ---
firefox47 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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...
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
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. :-)
Summary: Fix browser_addCertException.js to work under e10s → Fix browser_addCertException.js and browser_blockKPKP.js to work under e10s
e10s test bugs should block tracking-e10s=+
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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 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 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?
Attachment #8724079 - Flags: review?(jaws) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/083380e8d36f
https://hg.mozilla.org/mozilla-central/rev/2fa086d51b6a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Summary: Fix browser_addCertException.js and browser_blockKPKP.js to work under e10s → Fix browser_addCertException.js and browser_blockHPKP.js to work under e10s
You need to log in before you can comment on or make changes to this bug.