The waitForCertErrorLoad function is used in browser_captivePortal_certErrorUI.js to wait until cert errors are loaded. We should replace it with the more reliable BrowserTestUtils.waitForErrorPage function and make sure the affected tests still run correctly.  http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/browser/base/content/test/captivePortal/head.js#173  http://searchfox.org/mozilla-central/source/browser/base/content/test/captivePortal/browser_captivePortal_certErrorUI.js  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/captivePortal/browser_captivePortal_certErrorUI.js and learn more about Mochitest here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest
Created attachment 8846582 [details] [diff] [review] changes in browser_CaptivePortal_certErrorUI.js Replaced waitForCertErrorLoad with BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js. Tests failing after changes.
Attachment #8846582 - Flags: review?(jhofmann)
Assignee: nobody → meghanagupta3
Status: NEW → ASSIGNED
After chatting with Nihanth about this it turns out that captive portal pages don't emit the event that BrowserTestUtils.waitForErrorPage is listening to (AboutNetErrorLoad). So only the second waitForCertErrorLoad can be replaced by BrowserTestUtils.waitForErrorPage in that file. You should probably still change the first call in line 31 to BrowserTestUtils.waitForContentEvent(browser, "DOMContentLoaded") so that we can get rid of the shared function in head.js Let me know if anything is unclear :) Thanks!
Created attachment 8846629 [details] [diff] [review] Replaced function instances and deleted obsolete function from main.js Bug 1344726 - Replaced instances of waitForCertErrorLoad with BrowserTestUtils.waitForContentEvent and BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js. Removed waitForCertErrorLoad function in head.js
Comment on attachment 8846629 [details] [diff] [review] Replaced function instances and deleted obsolete function from main.js Review of attachment 8846629 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thank you! Now this needs a run on our try server, to check if all tests are still green. (https://wiki.mozilla.org/ReleaseEngineering/TryServer). I just did that for you, but in the future you might want to get your own try access. I've also set the checkin-needed flag. This flag signals that we want the patch merged. If the try looks good, someone will come around and check it in for us.
Attachment #8846629 - Flags: review?(jhofmann) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e35e5f2b9dfb5539598e67e3b08502e111be353 Bug 1344726 - Replace instances of waitForCertErrorLoad with BTU.waitForContentEvent and BTU.waitForErrorPage in browser_captivePortal_certErrorUI.js. r=johannh
Fixed the newline at the end of head.js and pushed to inbound.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.