1344726 - Replace waitForCertErrorLoad with BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js

Status: RESOLVED FIXED

(Firefox :: Security, enhancement)

Description

[Johann Hofmann [:johannh]]

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

Comment 1

[Meghana Gupta]

Attachment: changes in browser_CaptivePortal_certErrorUI.js

Replaced waitForCertErrorLoad with BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js. Tests failing after changes.

Comment 2

[Johann Hofmann [:johannh]]

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!

Comment 3

[Meghana Gupta]

Attachment: 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 4

[Johann Hofmann [:johannh]]

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.

Comment 5

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9f31f66b4a92a8b21309642536aeaba461e6622

Comment 6

[Johann Hofmann [:johannh]]

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

Comment 7

[Johann Hofmann [:johannh]]

Fixed the newline at the end of head.js and pushed to inbound.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e35e5f2b9df