1347482 - Having a System Add-on as a WebExtension with a content script causes mochitest failures in dom/ to do with enabling prefs in tests

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Mark Banner (:standard8)]

We are currently looking at enabling pageshot as a system add-on in the Firefox tree. Our initial experimentations have led us to find a number of test failures on try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b6533ee47272f4c8712a081af342cdeef12ae1e&selectedJob=83038766&filter-searchStr=tc-M

The failures are for the content based Mochitests. Generally the failures are items like:

dom/workers/test/test_navigator.html | Navigator has no 'connection' property!
dom/workers/test/test_navigator_secureContext.html | Navigator has no 'connection' property!
mock_gamepad.js, line 9: TypeError: navigator.requestGamepadServiceTest is not a function
test_geolocation_is_undefined_when_pref_is_off.html undefined assertion name - got [object Geolocation], expected undefined
test_bug957899.html, line 12: TypeError: navigator.requestWakeLock is not a function
test_presentation_availability.html | navigator.presentation should be available

These tests all have one thing in common: they are setting a preference to enable or disable a feature that's exposed via WebIDL on window.navigator.

The line of code that appears to be causing this due to enabling a WebExtension with a content script is:

https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/toolkit/components/extensions/ExtensionContent.jsm#151

This appears to initialise window.navigator, such that preference changes don't take effect (until the page is reloaded, something I don't think the mochitests can't do).

Comment 1

[Mark Banner (:standard8)]

At the moment, this will block PageShot as a system add-on, for which it is aiming for 54. So we need to find a solution.

For most of the tests, we could either:

1) Skip the test if the preference isn't enabled by default.
2) Set the preference to "on" for tests globally, rather than in the individual tests.

They would work for most of the tests, however `test_geolocation_is_undefined_when_pref_is_off.html` is specifically checking geolocation isn't available when the pref is off, so we'd still have an issue there.

Hence, I'm not sure of what the best way of getting this to work is.

Comment 2

[Andrew Overholt [:overholt]]

Boris, do you know what could be going wrong here?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment 0 explains what's going wrong.  The tests are relying on nothing touching navigator before they do their pref-setting, but ExtensionContent.jsm pokes at navigator and gets to run before the test code does.

The right fix is to change the tests to set the pref, wait for the callback that it's set, then create a new iframe and examine the navigator object inside that iframe.  That will be created after the pref is set, so will properly take it into account.

Comment 4

[Andrew Swan [:aswan]]

If it helps, we could pretty easily fix ExtensionContent.jsm to avoid touching navigator.  The line referenced in comment 0 was originally written that way to avoid having to expose the permission check that enables mozAddonManager to js but we ended up exposing that permission check anyway so I don't think it would be awful to use it instead of looking for navigator.mozAddonManager.

Comment 5

[Boris Zbarsky [:bzbarsky]]

It might help Mark's specific issue, but the tests are just kinda fragile as things stand, and making them less fragile is a good idea no matter what.

Comment 6

[Mark Banner (:standard8)]

I'm going to start looking at this later this week/start of next.

Comment 7

[Mark Banner (:standard8)]

Attachment: Bug 1347482 - Change various DOM tests to run the navigator checks in sub-iframes.

We have to run the navigator checks in sub-iframes as we need to set preferences which affect navigator's API, and hence need a clean instantiation.

Review commit: https://reviewboard.mozilla.org/r/125448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125448/

Comment 8

[Mark Banner (:standard8)]

The WIP is as far as I've got so far - it appears the test harness doesn't let you run tests from sub-iframes, so you have to proxy results back to the parent.

So far I've fixed the simplest test, still working out how to do the others.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8853348 [details]
Bug 1347482 - Change various DOM tests to run the navigator checks in sub-iframes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125448/diff/1-2/

Comment 10

[Boris Zbarsky [:bzbarsky]]

Mark, please watch out: due to bug 1286740 you will NOT get the right emails when that patch is reviewed.  :(

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8853348 [details]
Bug 1347482 - Change various DOM tests to run the navigator checks in sub-iframes.

https://reviewboard.mozilla.org/r/125448/#review129672

LGTM - it would be nice if there was an easy way to do this without the annoying boilerplate, but I can't think of one off of the top of my head, so this will have to do.

Comment 12

[Mark Banner (:standard8)]

Thank you for the extra notification Boris.

Unfortunately it turns out that there's still some failures I missed (or more were introduced), this patch is good though, so I'll get it landed, and then work on those other issues today.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c68d659c2b7
Change various DOM tests to run the navigator checks in sub-iframes. r=mystor

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c68d659c2b7