1344267 - Marionette test files for chrome scope should not be shipped with Firefox

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect, P2)

Description

[Florian Quèze [:florian]]

The files packaged at http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/testing/marionette/jar.mn#36 are shipped in Firefox. I don't think that's intended.

The 3 following files are reported by my test at bug 1316187 as unreferenced:
chrome://marionette/content/test_anonymous_content.xul
chrome://marionette/content/test_dialog.properties
chrome://marionette/content/test_dialog.xul

test_dialog.properties has been recently added in bug 1331037.

Are these files chrome mochitests that could be packaged like the examples in http://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/chrome.ini ?

Comment 1

[Andreas Tolfsen ❲:ato❳]

I’m surprised that ENABLE_TESTS is turned on for release builds.  Does
this mean we ship test-only binaries such as dist/bin/certutil with
officially branded builds?

> Are these files chrome mochitests that could be packaged like the
> examples in
> http://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/chrome.ini?

It’s not totally clear to me what the unique characteristics about
chrome resources are and where they need to be to be allowed to run, so
my immediate answer is “maybe”.

Marionette uses these files to test its chrome interaction abilities.
It’s unclear to me if we can load in a file such as test_dialog.xul
through navigating to it and expecting the existing tests to work.
whimboo may have greater insight in this area.

Comment 2

[Florian Quèze [:florian]]

(In reply to Andreas Tolfsen ‹:ato› from comment #1)
> I’m surprised that ENABLE_TESTS is turned on for release builds.

ENABLE_TESTS means you want to be able to run tests after building. It's absolutely required for release builds to pass tests before we consider shipping them, so this will be true for most builds you'll encounter.

> Does
> this mean we ship test-only binaries such as dist/bin/certutil with
> officially branded builds?

No, random files have to be listed in browser/installer/package-manifest.in to be shipped. (by 'random files' here I mean files that aren't packaged as chrome files).

Comment 3

[Andreas Tolfsen ❲:ato❳]

We should very likely work towards a solution that removes these files
from testing/marionette/jar.mn.  Unsetting ENABLE_TESTS also has other
consequences, such as that `./mach marionette-test` will fail when there
really is no reason for them to.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> The 3 following files are reported by my test at bug 1316187 as unreferenced:
> chrome://marionette/content/test_anonymous_content.xul
> chrome://marionette/content/test_dialog.properties
> chrome://marionette/content/test_dialog.xul
> 
> test_dialog.properties has been recently added in bug 1331037.
> 
> Are these files chrome mochitests that could be packaged like the examples
> in
> http://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/
> chrome.ini ?

Those test files are indeed used to test the chrome handling of Marionette.  But there are also others which you are not mentioning here:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/chrome

When I eg. search for test_nested_iframe.xul I do not find any reference of that file beside the entry in jar.mn under testing/marionette. So why does your script not list those?

Something else we could do for getting chrome tested, is to package all those chrome test files into an add-on which the marionette unit tests will install. This solution would also work if ENABLE_TESTS is set to false.

Comment 5

[Florian Quèze [:florian]]

(In reply to Henrik Skupin (:whimboo) from comment #4)

> But there are also others which you are not mentioning here:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/chrome
> 
> When I eg. search for test_nested_iframe.xul I do not find any reference of
> that file beside the entry in jar.mn under testing/marionette. So why does
> your script not list those?

There's a reference in test.xul (which is unreferenced) : http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/testing/marionette/chrome/test.xul#21

My script doesn't use a reference graph yet, so files referenced only from unreferenced flags aren't reported.

Comment 6

[Andreas Tolfsen ❲:ato❳]

> Something else we could do for getting chrome tested, is to package
> all those chrome test files into an add-on which the marionette unit
> tests will install. This solution would also work if ENABLE_TESTS is
> set to false.

I like that idea.  That is in line with how we bootstrap addon
dependencies for other harnesses such as mochitest using Marionette.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> My script doesn't use a reference graph yet, so files referenced only from
> unreferenced flags aren't reported.

So I'm unclear which immediate action we should take here. What are you proposing? Just adding the .xul files under `support-files` in our unit-test manifest?

(In reply to Andreas Tolfsen ‹:ato› from comment #6)
> > Something else we could do for getting chrome tested, is to package
> > all those chrome test files into an add-on which the marionette unit
> > tests will install. This solution would also work if ENABLE_TESTS is
> > set to false.
> 
> I like that idea.  That is in line with how we bootstrap addon
> dependencies for other harnesses such as mochitest using Marionette.

While this might sound good, I'm unclear in how long we will still be able to have support for old-style add-ons with XUL pages contained. I think the cut-off date is close. Somewhat in the next versions of Firefox.

Comment 8

[Florian Quèze [:florian]]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #5)
> > My script doesn't use a reference graph yet, so files referenced only from
> > unreferenced flags aren't reported.
> 
> So I'm unclear which immediate action we should take here.

Any solution that doesn't cause test-only files to be shipped to our whole user base is fine with me.
I don't have a specific proposal, you know this code better than I do.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Any solution that doesn't cause test-only files to be shipped to our whole
> user base is fine with me.
> I don't have a specific proposal, you know this code better than I do.

How do you know that? I might know better our tests, but I have no idea how to load XUL files in chrome scope if we aren't shipping them with Firefox. As far as I know we do not support remote XUL pages anymore, and legacy extensions are no longer supported in Firefox 57. So how can I ship XUL files that they are accessible via the chrome:// scheme? Or are there other options in opening new chrome windows with a XUL file not shipped via chrome://?

Comment 10

[Florian Quèze [:florian]]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> What are you
> proposing? Just adding the .xul files under `support-files` in our unit-test
> manifest?

You can try this. If it works that's nicely simple. I don't know anything about how the test_*.py tests work, so I don't know if it's likely to work or not.

If you end up needing something more complicated, I would look at how mochitest's test harness files are packaged.
Eg. stuff at http://searchfox.org/mozilla-central/source/testing/mochitest/jar.mn#34 that seems to be packaged in an add-on by http://searchfox.org/mozilla-central/source/testing/mochitest/moz.build

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This bug came up again when talking to Simon Friedberger because those pages show CSP issues when evaluating JavaScript via Marionette in chrome scope. So whether we get the CSP issues fixed or as stated above remove those files from the official Firefox build.

I have taken a look at Mochitest and how it does the registration for the addon. As it looks like there is still a way to register chrome URLs dynamically even it was stated the feature is gone. But it needs to be done through an experimental API as it looks like:

https://searchfox.org/mozilla-central/rev/8867630d67bfe76f6065df08d60381d45d73c439/testing/mochitest/api.js#133-144

This feature will probably stay given that it is required by Mochitests to work. But what I'm not sure about is if this would also require the preference extensions.experiments.enabled to be set to true. If that's true we may have to extract chrome scope tests into their own sub folder and maybe have to run a separate unit test job where we can then install the addon with the affected xhtml files registered correctly. Defining the preference should be done in the defaults section of the toml file so bug 1959688 would have to be fixed first.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Lets see if we can work on this bug for M16. Also because we would need such test cases as well for our WebDriver BiDi implementation.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I’ve extracted the relevant files and bundled them into an add-on that registers them for access via chrome URIs. However, this approach only works with unbranded builds of Firefox, but not with Beta or Release versions.

Registering a chrome handler requires meeting certain prerequisites:

• extensions.experiments.enabled must be set to true to use the registration API.
• In Beta/Release builds, the Cu.isInAutomation flag also needs to be true, which is a flag that is never true when running tests via Marionette.

Since we can’t satisfy the second condition, we’ll need to create a privileged add-on similar to quitter. This kind of add-on can be signed and checked into the tree as an XPI, just like tools/quitter/.

Proposed Workflow for Updating the Extension:

1. When test files need updating, use the upstream repository. You can generate an unsigned XPI locally using the webext tool to test the changes right away.
2. Request signing for the updated add-on. This process is mostly automated, though some steps may require manual intervention.
3. Once signed, stash the updated XPI in mozilla-central under /remote/test/extensions/chrome-assets/.
4. Submit a new Phabricator review to land the changes.

With the add-on-based approach in place, we’ll also need to implement the following:

• Add a helper method to the Marionette test framework (either the test case or harness) that installs the add-on into the active profile when tests require access to chrome:// URIs. This add-on should be automatically removed during test teardown. Note that the installation path may differ depending on whether the test is run via mach or mozharness; see this logic in mochitests for reference.

• Update all relevant tests to call this helper method when they rely on the chrome:// resources exposed by the add-on.

• Update test_archive.py to include the add-on in the common test package. The final location within the archive should be chosen to allow sharing between Marionette and WDSpec tests.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1344267 - [remote] Don't ship Marionette test files for chrome context with Firefox.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Another option for tests that use those chrome pages would be to run them isolated in a specific test job with the disable_all_security preference set to true - this is not a preference that we want to set in general for WebDriver and is required to be set before Firefox starts!

But when setting the preference, it would allow us to as well get the extension installed in a branded build because the check for Cu.isInAutomation will then return true.

However, this approach would bypass certain security checks, which risks masking failures or regressions that real Firefox users might encounter when automating with WebDriver. That said, I'm not necessarily opposed to taking that route.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There is actually another way to even simply it more. We discussed that yesterday in the Add-ons meeting and it indeed removes a lot of overhead and I think that this approach would work for us.

Instead of explicitly providing an add-on for those chrome files we could directly call the chrome registration handler when executing code in the chrome context. An example Marionette tests demonstrates it:

    def test_chrome_window(self):
        with self.marionette.using_context("chrome"):
            self.marionette.execute_script("""
                const { FileUtils } = ChromeUtils.importESModule(
                    "resource://gre/modules/FileUtils.sys.mjs"
                );
                XPCOMUtils.defineLazyServiceGetter(
                    this,
                    "aomStartup",
                    "@mozilla.org/addons/addon-manager-startup;1",
                    "amIAddonManagerStartup"
                );
                const registry = Cc["@mozilla.org/chrome/chrome-registry;1"].getService(
                    Ci.nsIChromeRegistry
                );

                const path = "/Users/henrik/code/gecko/remote/test/chrome-assets/";
                const rootURI = Services.io.newFileURI(new FileUtils.File(path));
                const manifestURI = Services.io.newURI(
                    "manifest.json",
                    null,
                    rootURI
                );
                this.chromeHandle = aomStartup.registerChrome(
                    manifestURI,
                    [["content", "remote-chrome", "content/"]]
                );
            """)
            new_window = self.open_chrome_window(
                "chrome://remote-chrome/content/test.xhtml"
            )

This works perfectly well in beta and release builds because there is no longer the need for the Experiments API and also the restriction for Cu.isInAutomation is cleared.

I'll check if we can reduce the number of XHTML files so we maybe only need 3 of them for our tests. Given that we will have to share them with upcoming wdspec tests, it's not clear yet where we want to package them. As well we need a solution for the reftest.xhtml and reftest-content.js files which are needed by reftests under /layout as well.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Moving out the chrome files for reftests is a bit more complicated given that it spans across Marionette unit tests and web-platform-tests. Especially that the latter will have to work upstream as well it's not that clear to me yet where to put the relevant xhtml and js files. As such I'll file a new bug to handle the reftest files after we have a fix landed here.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With the patch applied the CSP is failing and causing a crash because of inline styles and embedded iframes. While I could outsource the styles to a separate stylesheet, I'm not able to work around the iframe issue. Our test files need to check that.

Simon, what needs to be done to prevent the crash from happening? It looks like that <?csp default-src 'none'; ?> is the only thing that I can add. Any other addition causes a crash as well. May we have to set security.csp.testing.allow_internal_csp_violation to true for those specific tests? I hope that we can circumvent that.

Comment 19

[Simon Friedberger [:simonf]]

What exactly is the file you're having trouble with?

If you want to allow something you may need to add an exception in one of the lists here-ish: https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityUtils.cpp#1316

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The most obvious thing are the inline styles that are used and which I was able to solve by moving it out into a separate stylesheet. But what's not working is that I can allow frames to be used. Those are always denied:

[70674] Hit MOZ_CRASH(Document (chrome://marionette-chrome/content/test.xhtml) must not contain a CSP with the unchecked directive frame-src) at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityUtils.cpp:1968
#01: nsContentSecurityUtils::AssertChromePageHasCSP(mozilla::dom::Document*)[/Users/henrik/code/gecko-obj/debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5065a80]

Also for the test_dialog.xhtml file I get:

 Mozilla crash reason: Unexpected CSP violation on page chrome://marionette-chrome/content/test_dialog.xhtml caused by script-src-attr (URL: inline, Source: ) violating the directive: "default-src 'none'" (file: chrome://marionette-chrome/content/test_dialog.xhtml line: 0).

But none of the test files have actually an inline script defined. So not sure where this is actually coming from.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Discussed with Simon on Slack and we can actually use <?csp default-src chrome:; ?> for those files that embed iframes. The other script related issue came from the inline oncommand="" attribute for the button element in test_dialog.xhtml`. Removing it fixed it. I'll update the patch.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0de2658f47f8
https://hg.mozilla.org/integration/autoland/rev/d6975a9d7fc7
[remote] Don't ship Marionette test files for chrome context with Firefox. r=webdriver-reviewers,simonf,jdescottes

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6ae2179c107d
https://hg.mozilla.org/integration/autoland/rev/19f854ec1c99
Revert "Bug 1344267 - [remote] Don't ship Marionette test files for chrome context with Firefox. r=webdriver-reviewers,simonf,jdescottes" for causing multiple perma failures

Comment 24

[Sandor Molnar[:smolnar]]

Backed out for causing multiple perma failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/19f854ec1c995224cf63dca1a4c5c080bce26918

Push with failures

Failure log -> bc

Failure log -> xpc

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As it looks like the .dtd test files as used by Marionette were the last ones in the tree that are actually considered to check:
https://searchfox.org/mozilla-central/search?q=&path=.dtd&case=false&regexp=false

With moving them out of the product code and the usage of Fluent these days we most likely can now get rid of this particular test.

The other xpcshell failures happen on Android where I didn't actually run tests for. I'll check.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1344267 - [browser] Remove the checkAllTheDTDs test.

This test is no longer needed given that the Marionette
DTD test file was the only one remaining in the product.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7a069af9b7ca
https://hg.mozilla.org/integration/autoland/rev/42c3fb1f104b
[browser] Remove the checkAllTheDTDs test. r=mossop
https://github.com/mozilla-firefox/firefox/commit/1ec2485224f4
https://hg.mozilla.org/integration/autoland/rev/49a944031a5b
[remote] Don't ship Marionette test files for chrome context with Firefox. r=webdriver-reviewers,simonf,jdescottes

Comment 28

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/42c3fb1f104b
https://hg.mozilla.org/mozilla-central/rev/49a944031a5b