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)
Firefox
Security
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
Assignee | ||
Comment 1•7 years ago
|
||
Hello. Would like to be assigned this bug. Thanks.
Reporter | ||
Comment 2•7 years ago
|
||
Thanks! Have fun!
Assignee: nobody → lenikmutungi
Status: NEW → ASSIGNED
Priority: -- → P5
Assignee | ||
Comment 3•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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•7 years ago
|
Attachment #8851890 -
Attachment is obsolete: true
Attachment #8851890 -
Attachment is patch: false
Attachment #8851890 -
Flags: review?(jhofmann)
Reporter | ||
Updated•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
bugherder |
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.
Description
•