Replace promiseErrorPageLoaded with BrowserTestUtils.waitForErrorPage in browser_ssl_error_reports.js

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Security
P5
normal
RESOLVED FIXED
a month ago
28 days ago

People

(Reporter: johannh, Assigned: Leni Kadali, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a month ago
The promiseErrorPageLoaded[0] function is used in browser_ssl_error_reports.js[1] to wait until cert errors are loaded. We should replace it with the more reliable BrowserTestUtils.waitForErrorPage[2] function and make sure the affected tests still run correctly.

[0] http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/browser/base/content/test/general/head.js#986
[1] http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_ssl_error_reports.js
[2] http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#689

You can run the test with

./mach mochitest browser/base/content/test/general/browser_ssl_error_reports.js

and learn more about Mochitest here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest
(Assignee)

Comment 1

a month ago
Hello. Would like to be assigned this bug. 
Thanks.
(Reporter)

Comment 2

a month ago
Thanks! Have fun!
Assignee: nobody → lenikmutungi
Status: NEW → ASSIGNED
Priority: -- → P5
(Assignee)

Comment 3

a month ago
So I've replaced the promiseErrorLoaded function with the BrowserTestUtils.waitForErrorPage function in my local copy of mozilla-central/browser/base/content/test/general/head.js as follows: 

/**
   * Like browserLoaded, but waits for an error page to appear.
   * This explicitly deals with cases where the browser is not currently remote and a
   * remoteness switch will occur before the error page is loaded, which is tricky
   * because error pages don't fire 'regular' load events that we can rely on.
   *
   * @param {xul:browser} browser
   *        A xul:browser.
   *
   * @return {Promise}
   * @resolves When an error page has been loaded in the browser.
   */
  waitForErrorPage(browser) {
    let waitForLoad = () =>
      this.waitForContentEvent(browser, "AboutNetErrorLoad", false, null, true);

    let win = browser.ownerDocument.defaultView;
    let tab = win.gBrowser.getTabForBrowser(browser);
    if (!tab || browser.isRemoteBrowser || !win.gMultiProcessBrowser) {
      return waitForLoad();
    }

    // We're going to switch remoteness when loading an error page. We need to be
    // quite careful in order to make sure we're adding the listener in time to
    // get this event:
    return new Promise((resolve, reject) => {
      tab.addEventListener("TabRemotenessChange", function() {
        waitForLoad().then(resolve, reject);
      }, {once: true});
    });
  },

I've left in the comments because I feel they would be useful to someone reading the code. If that's overkill, let me know. 

I've also changed the reference in my local copy of browser_ssl_error_reports.js from |yield promiseErrorPageLoaded(browser);| to |yield BrowserTestUtils.waitForErrorPage(browser);|

Preparing to submit the patch(es). 
|
(Assignee)

Comment 4

a month ago
Adding the above code to my local copies of head.js and browser_ssl_error_reports.js and running `./mach mochitest browser/base/content/test/general/browser_ssl_error_reports.js` results in the following output:

TEST-INFO | checking window state
Browser Chrome Test Summary
        Passed: 20
        Failed: 1
        Todo: 0
        Mode: e10s
*** End BrowserChrome Test Results ***
The following tests failed:
43 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_ssl_error_reports.js | head.js import threw an exception - at chrome://mochitests/content/browser/browser/base/content/test/general/head.js:807 - SyntaxError: missing ; before statement
Buffered messages finished
SUITE-END | took 20s

I have failed to locate where the missing ; should be placed. Any advice would be welcome.
(Assignee)

Comment 5

a month ago
Running ./mach eslint browser/base/content/test/general/head.js gives me the following output:

/home/herabus/mozilla-central/browser/base/content/test/general/head.js
  807:29  error  Parsing error: Unexpected token {  (eslint)
(Reporter)

Comment 6

a month ago
Hey Leni,

you don't need to replace promiseErrorPageLoaded with anything, you can simply delete it from head.js. The BrowserTestUtils.waitForErrorPage function gets automatically loaded into your test file, no need to add it to head.js yourself :)

The errors come from the fact that you're adding the function as an object property instead of a function declaration. But again, you don't need to add it at all.

Thanks!
(Assignee)

Comment 7

29 days ago
Created attachment 8851886 [details]
The head.js with the promiseErrorPageLoaded function removed.

Removed the promiseErrorPageLoaded function from head.js. Unfortunately, I messed up while trying to use Mercurial to commit, so I'm attaching it as a patch here since that's more straightforward for the time being.
Flags: needinfo?(jhofmann)
Attachment #8851886 - Flags: review?(jhofmann)
(Assignee)

Comment 8

29 days ago
Created attachment 8851890 [details]
The browser_ssl_error_reports.js file references BrowserTestUtils.waitForErrorPage

References in browser_ssl_error_reports.js to promiseErrorPageLoaded removed and replaced with references to BrowserTestUtils.waitForErrorPage

Unfortunately, I messed up while trying to use Mercurial to commit, so I'm attaching it as a patch here since that's more straightforward for the time being.
Flags: needinfo?(jhofmann)
Attachment #8851890 - Flags: review?(jhofmann)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

29 days ago
mozreview-review
Comment on attachment 8851957 [details]
Bug 1346995 - Replace promiseErrorPageLoaded with BrowserTestUtils.waitForErrorPage in browser_ssl_error_reports.js

https://reviewboard.mozilla.org/r/124208/#review126806

Looks great, thank you!

Now this needs a run on our try server, to check if tests are still green (https://wiki.mozilla.org/ReleaseEngineering/TryServer).

Please also edit the commit message (see below) and do another hg push review

::: commit-message-72bc2:4
(Diff revision 1)
> +Bug 1346995 - Replace promiseErrorPageLoaded with BrowserTestUtils.waitForErrorPage in browser_ssl_error_reports.js
> +Removed the promiseErrorPageLoaded function from head.js. Replaced references to it with BrowserTestUtils.waitForErrorPage
> +function. References in browser_ssl_error_reports.js to promiseErrorPageLoaded removed and replaced with references to
> +BrowserTestUtils.waitForErrorPage instead.

You should amend the commit message to only say that sentence once and push again :)
Attachment #8851957 - Flags: review?(jhofmann) → review+
(Reporter)

Updated

29 days ago
Attachment #8851890 - Attachment is obsolete: true
Attachment #8851890 - Attachment is patch: false
Attachment #8851890 - Flags: review?(jhofmann)
(Reporter)

Updated

29 days ago
Attachment #8851886 - Attachment is obsolete: true
Attachment #8851886 - Attachment is patch: false
Attachment #8851886 - Flags: review?(jhofmann)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

29 days ago
mozreview-review
Comment on attachment 8851957 [details]
Bug 1346995 - Replace promiseErrorPageLoaded with BrowserTestUtils.waitForErrorPage in browser_ssl_error_reports.js

https://reviewboard.mozilla.org/r/124208/#review126836
(Reporter)

Comment 13

29 days ago
Thanks for amending the message. For future reference, you should usually end the first line of the commit message with r?reviewer or r=reviewer (if checking in), so in your case 

Bug 1346995 - Replace promiseErrorPageLoaded with BrowserTestUtils.waitForErrorPage in browser_ssl_error_reports.js. r=johannh

I'm setting the checkin-needed flag to get this landed!
Keywords: checkin-needed

Comment 14

28 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/29bf5683f2ee
Replace promiseErrorPageLoaded with BrowserTestUtils.waitForErrorPage in browser_ssl_error_reports.js r=johannh
Keywords: checkin-needed

Comment 15

28 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29bf5683f2ee
Status: ASSIGNED → RESOLVED
Last Resolved: 28 days ago
status-firefox55: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.