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

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks 1 bug)

Trunk
Firefox 47
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(e10s+, firefox47 fixed)

Details

Attachments

(2 attachments)

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+
Assignee

Comment 1

5 years ago
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

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee

Comment 5

3 years ago
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.
Attachment #8724078 - Flags: review?(jaws) → review+
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+
Assignee

Comment 8

3 years ago
(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 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/083380e8d36f
https://hg.mozilla.org/mozilla-central/rev/2fa086d51b6a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

3 years ago
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.