1769791 - allowXULXBL shouldn't be the default when running mochitests

Status: RESOLVED FIXED

(Core :: XUL, task)

Description

[Andrew McCreight [:mccr8]]

jmaher is experimenting with running tests with a more realistic profile, and one of the things this uncovered is that we have allowXULXBL enabled by default in testing profiles. A testing server can disable XUL explicitly, but right now only "noxul.example.com" has this set.

I'm sure this made sense when it was added, but XUL and XBL have been really pared down since then. At a minimum, we should default to not allowing XUL, and maybe add some kind of "yesxul.example.com" for existing tests, but maybe we should just delete the tests that try to create XUL elements on content pages.

Comment 1

[Andrew McCreight [:mccr8]]

It looks like reftests also set allowXULXBL, but maybe that should be investigated in a separate bug.

Comment 2

[Andrew McCreight [:mccr8]]

There's the similar dom.allow_XUL_XBL_for_file pref, which is set to true in reftests.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #0)

but maybe we should just delete the tests that try to create XUL elements on content pages.

I'd actually be OK with this. In-content XUL is pretty broken at this point anyway, and in the past I've WONTFIXed bugs people have filed against it.

Comment 4

[Andrew McCreight [:mccr8]]

I've fixed a few of the places that rely on this, either by explicitly setting allowxulxbl for the iframes we then load code into or by deleting the part of the test that does XUL stuff, but two remain:

• test_bug564863.xhtml: sets a bunch of attributes, including some XUL stuff.
• test_bug547996-2.xhtml: events on XUL elements.

Unlike the other tests I fixed, the XUL here is at the top level of the page.

Some potential fixes (mostly proposed by kmag):

• Change the tests to load their XUL stuff in an iframe.
• Delete the test. We'd want to make sure we have non-mochitest-plain that test the behavior, because it may still be used by chrome JS.
• Port the XUL part of the tests to run as chrome. By making them a mochitest-browser-chrome or the like. I poked a bit at turning one of these into a mochitest-chrome but we probably want to avoid writing more of those.

It is possible that more things depend on this, but those are the tests that seem to be failing because of it in the work in bug 1769257.

Comment 5

[Andrew McCreight [:mccr8]]

I did a try push, and unfortunately it appears like there are a lot more tests that fail when XUL is disabled. Maybe it will be worth the time to add a way to add allowxulxbl in the .ini file after all.

Comment 6

[Andrew McCreight [:mccr8]]

Here are some of the failures in my try run. It isn't as bad as I thought, as each kind of test is run many different times in the try push.

Crash test:
js/xpconnect/tests/chrome/test_bug932906.xhtml | Threw when passing non-DOM XPCWN to content

Mochitest plain:
dom/html/test/test_bug458037.xhtml
image/test/mochitest/test_xultree_animation.xhtml
layout/xul/test/test_bug386386.html, test_bug394800.xhtml (and presumably everything else in the mochitest.ini file in that directory)
toolkit/content/tests/mochitest/test_mousecapture.xhtml

Mochitest browser-chrome:
toolkit/components/tooltiptext/tests/browser_bug331772_xul_tooltiptext_in_html.js
browser/base/content/test/general/browser_save_video_frame.js (not obviously XUL related, so maybe just a regular failure?)

Mochitest devtools:
devtools/client/inspector/markup/test/browser_markup_remove_xul_attributes.js
devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-xul.js
devtools/client/inspector/rules/test/browser_rules_inactive_css_xul.js
devtools/server/tests/browser/browser_inspector-anonymous.js

Comment 7

[Andrew McCreight [:mccr8]]

I have some patches to dredge out the permission stuff in testing, but the simplest way to disable XUL is to change this line to permissions = {}.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Bug 1769791, part 1 - Remove support for setting permissions in mozprofile.

The only permission that is set is allowXULXBL.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

is there anything I can do to help out in this bug? I could make a manifest flag allowxulxbl and we can then decide to delete/fix those tests later. That won't work for reftest- happy to hack there as well.

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Joel Maher ( :jmaher ) (UTC -0800) from comment #9)

is there anything I can do to help out in this bug? I could make a manifest flag allowxulxbl and we can then decide to delete/fix those tests later. That won't work for reftest- happy to hack there as well.

Yeah, that would be useful. Converting tests individually isn't too hard, but it does take a bit of time.

Also, I actually got confused about what the C test was. test_bug932906.xhtml is a mochitest-chrome, not a crash test.

browser_save_video_frame.js looks to pass locally with allowxulxbl, so I think whatever failure I was seeing was unrelated.

Comment 11

[Andrew McCreight [:mccr8]]

As you might expect, the BC test and devtools tests look easy to fix with pushpermissions, so it is really just the mochitest-plain and the mochitest-chrome tests where it'll likely help the most.

Comment 12

[Andrew McCreight [:mccr8]]

I've written a patch for the BC test and devtools tests and posted it for review in bug 1772851.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

if we go the route of a manifest entry, we will need to run the tests in a separate job. This isn't too hard, but I wanted to call this out. The reason for this is we create a fresh profile, then run a manifest of tests with that profile. If it is as simple as seeing a tag and adding:

  await SpecialPowers.pushPermissions([
    { type: "allowXULXBL", allow: true, context: URL_ROOT },
  ]);

then I see that as possible in the harness- we would treat it like we do https_first_disabled:
https://searchfox.org/mozilla-central/search?q=https_first_disabled&path=&case=false&regexp=false

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: Bug 1769791 - allow_xul_xbl support in manifests. r=mccr8!,gbrown!

Comment 15

[Phabricator Automation]

Comment on attachment 9280315 [details]
Bug 1769791 - allow_xul_xbl support in manifests. r=mccr8!,gbrown!

Revision D148681 was moved to bug 1773523. Setting attachment 9280315 [details] to obsolete.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

the WIP patch to remove xul from mozprofile looks pretty good (appears to touch all the testing related sqlite code (some talos stuff to consider) and the tests are appropriately modified). I have tested this on a larger set of tests (so many tests use mozprofile) and they seem to work. Curious why reftest works given that there is mention of requiring XUL. When 1773523 is merged in, we could land the noxul patch here.

:mccr8, do you want to put it up for review?

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Joel Maher ( :jmaher ) (UTC -0800) from comment #16)

the WIP patch to remove xul from mozprofile looks pretty good (appears to touch all the testing related sqlite code (some talos stuff to consider) and the tests are appropriately modified). I have tested this on a larger set of tests (so many tests use mozprofile) and they seem to work.

Great!

Curious why reftest works given that there is mention of requiring XUL. >

From comment 1, it looks like it uses a different mechanism than the mozprofile stuff to set the preference.

:mccr8, do you want to put it up for review?

Yeah, I'll rebase it today or tomorrow and then put it up for review.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/670004b987ac
Remove support for setting permissions in mozprofile. r=jmaher

Comment 19

[Cristina Cozmuta (:CrissCozmuta)]

Backed our for causing multiple failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/87cf6fcf7ac96522339d330b82c557dcffa861d9

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=670004b987ac7e78a204d942c7dfa3c450ab8ac3&selectedTaskRun=G0XcyKZMQXaXwavPShcbyg.0

Link to failure log:
https://treeherder.mozilla.org/logviewer?job_id=380936118&repo=autoland&lineNumber=1928
https://treeherder.mozilla.org/logviewer?job_id=380926800&repo=autoland&lineNumber=2377
https://treeherder.mozilla.org/logviewer?job_id=380935129&repo=autoland&lineNumber=6663

Failure line :
OSError: [WinError 6] The handle is invalid
REFTEST TEST-UNEXPECTED-FAIL | dom/base/crashtests/344434-1.xhtml | load failed: timed out waiting for reftest-wait to be removed
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/dom/multipleappendwithxul.xhtml == layout/reftests/dom/multipleappendwithxul-ref.xhtml | load failed: timed out waiting for reftest-wait to be removed

Comment 20

[Andrew McCreight [:mccr8]]

I don't see how the Win7 xpcshell-test failures could be related to my patch, but I'll trigger those tests on my try run. xpcshell tests didn't get run at all in my try run so maybe there's something there.

The reftest and crashtest failures are interesting. They are only failing on Android, and the tests involved are clearly using XUL. Maybe on desktop they get the XUL flag enabled via the code I found in comment 1, but that isn't used on Android?

Comment 21

[Andrew McCreight [:mccr8]]

If it is really the case that the XUL permission is given on desktop via that code but not Android, I should eliminate that permission at the same time, or we'll get a weird divergence in behavior between Android and desktop.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

reftests on android run via a remote script:
https://searchfox.org/mozilla-central/source/layout/tools/reftest/remotereftest.py#333

I didn't see anything in there that would indicate a difference.

Comment 24

[Andrew McCreight [:mccr8]]

With some testing, it seems to actually be the setting of the dom.allow_XUL_XBL_for_file pref that is the issue. On desktop, deleting the permission code from manifest.jsm didn't have any effect, but deleting the pref from user.js resulted in the same failures as seen on Android.

Comment 25

[Andrew McCreight [:mccr8]]

The Win7 XPCShell failure shows up in my try run, so that's definitely my fault. Maybe I didn't quite clean something up related to mozprofile permissions writing stuff.

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mccr8, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

I suspect :mccr8 got side tracked; ping me if you have questions or need help.

Comment 28

[Andrew McCreight [:mccr8]]

Yeah, sorry, I haven't had a chance to work on this again. The Android issue is probably something I can figure out, but the Win7 xpcshell failure is just weird.

Comment 29

[Andrew McCreight [:mccr8]]

I think I figured out why the Android reftests were failing. ServeTestBase in manifest.jsm sets the allowXULXBL permission for http://localhost, but only if runHttp is true. Elsewhere in that file, I see the comment "We can't yet run the local HTTP server for non-local reftests." which I think means that runHttp is false for Android remote reftests. To address this, I refactored the code a bit so that CreateUrls always sets the allowXULXBL permission for testbase, which I think then ensures that it is set for file:// in the remote case. That at least seems to be good enough to make the Android reftests and crashtests pass in a try run. I also removed the setting of dom.allow_XUL_XBL_for_file in the reftest user.js, as it doesn't seem to be doing anything. I don't know why that is. I'll have to check that that doesn't break other platforms.

Next I have to figure out the Windows XPCShell test problem.

Comment 30

[Andrew McCreight [:mccr8]]

Attachment: Bug 1769791, part 2 - Add allowXULXBL for file URIs for reftests even if not http.

This ensures we have the permission to load XUL files from file URIs when we are
not using HTTP, which we seem to need for remote reftests, like Android.

This also removes the setting of dom.allow_XUL_XBL_for_file in the reftest pref
file, which doesn't seem to be enough to make Android remote reftests work for
some reason, so I don't think it is doing anything.

Comment 31

[Andrew McCreight [:mccr8]]

I've investigated the Win7 opt XPCShell failures a bit. First off, these are happening during the --self-test phase, not while running the XPCShell tests per se. Every XPCShell chunk seems to run it, and they are all failing in the same way.

They seem to be hitting this error OSError: [WinError 6] The handle is invalid inside Python's subprocess.py. Doing some Googling, this seems to be a Windows-specific issue inside subprocess.py that was fixed in the middle of 2019. The XPCShell log contains the string CPython3.7.3.final.0-32, which came out around March 2019, so I guess that's consistent with my theory.

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

I filed a jira ticket to upgrade win7 python: https://mozilla-hub.atlassian.net/browse/RELOPS-251. If this is straightforward we could have the win7 pool upgraded by the end of the month, if this is not straightforward, we can revisit solutions then.

Comment 33

[Andrew McCreight [:mccr8]]

Thanks. I have two ideas for workarounds that won't require upgrading Python.

One would be to add a wait to runxpcshelltests, because I found somebody who was able to do that to work around this issue in unpatched versions of Python.

I also noticed that at least two of the subtests that are failing are ones that use the same file name for their manifest as an earlier, passing test. Maybe I can work around this issue by renaming those files.

Finally, I'm going to see if the XPCShell unit tests fail with my patch (and without either of these fixes) on Windows 10. It looks like Windows 10 is using a later version of Python that contains the fix for the subprocess issue. If that still fails, then my theory about the cause is probably wrong.

Comment 34

[Andrew McCreight [:mccr8]]

Unfortunately, neither of my attempts at a fix helped. Adding the wait does seem to have shifted which tests failed a bit, which is weird, but didn't actually help over all. I did confirm that the XPCShell unit tests pass on Windows 10, so that at least supports my theory that it is related to the old version of Python used on Windows 7.

Comment 35

[Andrew McCreight [:mccr8]]

It looks like there's already some code in place to work around the subprocess._cleanup issue in runxpcshelltests.py. It looks like the issue is that not all of the popen calls have had the work around applied, or maybe the guard for the current workaround isn't quite right, because when I bashed it into place everywhere in that file I finally got a green Win7 run. I'll probably do that in a separate bug blocking this one.

Comment 36

[Andrew McCreight [:mccr8]]

I found another reference to these self-tests failing with verify mode due to the popen issue. It is possible that my patch will fix that, but I'll leave it to somebody else to investigate if they want.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54312b95b3a3
part 1 - Remove support for setting permissions in mozprofile. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/cfe4296e5710
part 2 - Add allowXULXBL for file URIs for reftests even if not http. r=jmaher

Comment 38

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/54312b95b3a3
https://hg.mozilla.org/mozilla-central/rev/cfe4296e5710