Closed Bug 1346995 Opened 7 years ago Closed 7 years ago

Replace promiseErrorPageLoaded with BrowserTestUtils.waitForErrorPage in browser_ssl_error_reports.js

Categories

(Firefox :: Security, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: johannh, Assigned: leni1, Mentored)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Hello. Would like to be assigned this bug. 
Thanks.
Thanks! Have fun!
Assignee: nobody → lenikmutungi
Status: NEW → ASSIGNED
Priority: -- → P5
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). 
|
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.
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)
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!
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)
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 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+
Attachment #8851890 - Attachment is obsolete: true
Attachment #8851890 - Attachment is patch: false
Attachment #8851890 - Flags: review?(jhofmann)
Attachment #8851886 - Attachment is obsolete: true
Attachment #8851886 - Attachment is patch: false
Attachment #8851886 - Flags: review?(jhofmann)
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
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
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
https://hg.mozilla.org/mozilla-central/rev/29bf5683f2ee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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.