Closed Bug 1344726 Opened 5 years ago Closed 5 years ago

Replace waitForCertErrorLoad with BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js

Categories

(Firefox :: Security, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: johannh, Assigned: meghanagupta3, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

The waitForCertErrorLoad[0] function is used in browser_captivePortal_certErrorUI.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/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/browser/base/content/test/captivePortal/head.js#173
[1] http://searchfox.org/mozilla-central/source/browser/base/content/test/captivePortal/browser_captivePortal_certErrorUI.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/captivePortal/browser_captivePortal_certErrorUI.js

and learn more about Mochitest here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest
Replaced waitForCertErrorLoad with BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js. Tests failing after changes.
Flags: needinfo?(jhofmann)
Attachment #8846582 - Flags: review?(jhofmann)
Assignee: nobody → meghanagupta3
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)
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!
Bug 1344726 - Replaced instances of waitForCertErrorLoad with BrowserTestUtils.waitForContentEvent and BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js. Removed waitForCertErrorLoad function in head.js
Attachment #8846582 - Attachment is obsolete: true
Attachment #8846582 - Flags: review?(jhofmann)
Attachment #8846629 - Flags: review?(jhofmann)
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+
Keywords: checkin-needed
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e35e5f2b9df
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1347602
You need to log in before you can comment on or make changes to this bug.