448676 - Enable UniversalXPConnect in tinderbox crashtest and/or reftest profiles

Status: RESOLVED WONTFIX

(Testing :: Reftest, defect)

Description

[Graeme McCutcheon [:graememcc]]

There are several layout reftests (bug 263683, bug 407959, bug 401361) currently disabled that need to request UniversalXPConnect in order to work. 

When creating a profile for the test run, the unit test boxes should have the capability preferences set to automatically accept these requests. Without this, any time a reftest is written asking for the privileges, the security manager dialog is displayed, causing multiple non-obvious reftest failures, and potentially long cycle times.

Comment 1

[Graeme McCutcheon [:graememcc]]

Attachment: Patch

Unsure who would review this.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm... which tests would this profile affect?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The tests need to be runnable by developers; changing tinderbox alone is both insufficient and harmful.

Comment 4

[(no longer active)]

Ping?  Any progress being made here?

Comment 5

[(no longer active)]

Can this be handled in ReadManifest (layout/tools/reftest/reftest.js) so that UniversalXPConnect can be enabled for reftests marked with a special tag in the manifest?

Comment 6

[Mats Palmgren (inactive)]

Attachment: Like so?

This enables "UniversalXPConnect" for all tests.
I have a half-baked patch that adds syntax to the manifest to choose
any privilege for each individual test too... if this is the right
approach at all...

Comment 7

[Boris Zbarsky [:bzbarsky]]

That patch:

1)  Nukes existing capability prefs if there are any
2)  Doesn't work unless the profile already has the "don't do the new file://
    security stuff" pref set, unless I'm missing something.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not even sure we should do this for reftests; it encourages people to write tests that are usable only by Mozilla.  It would be nice if reftests were generally usable by other browsers.

(Note that we do now have the ability to do reftest-equivalent inside mochitest.)

Comment 9

[Mats Palmgren (inactive)]

(In reply to comment #7)
> 1)  Nukes existing capability prefs if there are any

The 'prefHasUserValue' was intended to avoid existing prefs and I actually
tested that existing "capability.principal.codebase.pN..." in prefs.js
survived untouched.  Maybe I'm missing something, please elaborate.

> 2)  Doesn't work unless the profile already has the "don't do the new file://
>     security stuff" pref set, unless I'm missing something.

Sorry, I don't know what pref set you're referring to.  Would it work if
I also add that pref set unless it already exists?

Comment 10

[Mats Palmgren (inactive)]

(In reply to comment #8)
I agree, but I think the primary reason for needing privileges is for
crash tests.  In my experience, some crash tests does not crash
reliably under mochitest.  (I just saw that today actually, in bug 456273
the test crashes reliably when loading from file: but only occasionally
when run as a mochitest.)

> (Note that we do now have the ability to do reftest-equivalent inside
> mochitest.)

Ok, I didn't know that. Then we should use that for the reftests that need
privileges. (BTW, is it documented how to use it somewhere?)

So what do you suggest for crash tests that need privileges?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ah, gCapabilityPrefBranch doesn't include other capability prefs.  I misread that part, sorry.

> Sorry, I don't know what pref set you're referring to.

"security.fileuri.strict_origin_policy".  Setting that to false would probably make this patch work, yes...

> So what do you suggest for crash tests that need privileges?

Making file:// mochitests work might be the answer, since we need those anyway.

Comment 12

[(not currently active) Ted Mielczarek]

This might be better served by fixing bug 468913.

Comment 13

[(not currently active) Ted Mielczarek]

We could easily set whatever prefs we want in runreftest.py now, although I don't know how dbaron feels about that. Another option would be to add a third reftest pass, like "reftest-privileged", which would create a profile with the right prefs, and run tests from a different reftest.list so that these tests could be sequestered from the other tests.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm ok with enabling more privileges in the crashtests than for the reftests.  I think privileged reftests should be done using the reftest-in-mochitest, in the rare cases that we really need them.

Comment 15

[Graeme McCutcheon [:graememcc]]

I should have returned here to comment long ago. 

Many bugs later, but before it was explicitly mentioned in the comments here, I learned of mochitest's window snapshot and comparison capabilities. These proved sufficient for the test I was trying to write at the time of filing this bug.

I'm happy for this to either be closed, or morphed to cover crashtests.

Comment 16

[Jesse Ruderman]

enablePrivilege is going away.  See bug 546848 and its dependency tree.

Comment 17

[(not currently active) Ted Mielczarek]

When we get the work in bug 462483 done, it should be trivial to make it available to reftests as well, if we want to go that route.