JS browser tests (|make jstestbrowser|, aka js reftests) try to set prefs in the child process. This fails, generating thousands of errors, for example Preferences_setIntPref: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIPrefBranch.setIntPref]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: file:///home/alon/Dev/mozilla-central/js/src/tests/browser.js :: Preferences_setIntPref :: line 386" data: no] Preferences_setCharPref: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIPrefBranch.setBoolPref]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: file:///home/alon/Dev/mozilla-central/js/src/tests/browser.js :: Preferences_setPref :: line 332" data: no] This happens in js/src/tests/browser.js, for example if a test does gczeal() We could perhaps add something to the test harness to remote this call (SpecialPowers does it for mochitests, smaug informs me). But perhaps there is a solution in the JS engine, to basically just apply the option, without going through the pref system.

Even if we did remote this, as SpecialPowers does, we would suffer from an unpredictable delay until the notification about the changed pref reaches the child process (bug 621363). The solution in that bug, spinning the event loop, is apparently too risky. Using generators is proposed instead. However I don't think this can work for jsreftests, we would need to rewrite thousands of them.

I realize this patch is controversial. Here is my rationale: 1. Using special powers, or copying the relevant code from special powers, isn't a good solution due to the previous comment (it forces us to spin an event loop as we wait for the information to get back to us, which is bad (cjones), and the alternative is to rewrite thousands of jsreftests). 2. And in any case changing the reftest harness in some way that enables preferences to be set is opposed (dbaron), since the goal is to keep reftests as similar as possible to normal web pages. 3. The problem we have here is very limited to jsreftests (although possibly it can help with certain other reftests as well (jmaher)). In that case, it will work to just apply the pref locally, so why not do just that. 4. The patch only allows setting the pref in the child if a special pref is set, so this will not affect other types of tests or the browser in general - it's only for jsreftests.

Comment on attachment 545251 [details] [diff] [review] patch Please comment above this: +user_pref("testing.allow_setting_local_prefs_in_child", true); We want to make sure that this is only set for testing, and we want to probably document why it is needed. if (GetContentChild()) { to: if (GetContentChild() && !AllowChildToSetPrefsLocally()) { Please add tests to ensure that this preference is not set for any unexpected tests.

cjones|bsmedberg: This is probably the best of all non-optimal solutions. I tend to agree with bsmedberg's no-pref-setting in child processes. Here we are carefully setting preferences for tests, not production code. We will also include additional tests to ensure that this preference is not set unexpectedly.

Sadly the discussion of this issue has ended up split between this bug and bug 621363. Do we want to dupe one into the other?

Are these the tests that are setting gczeal? Why don't we do something simpler and optimal, like fix gczeal? (Per discussion in bug 621363.) I'm fine leaving the discussion separate if the patterns in the jsreftests are different (gczeal f.e.) than "fix arbitrary setting of prefs in arbitrary tests" as in 621363.

It wouldn't be just gczeal, there are several prefs that are relevant, like javascript.options.jit.*. We can hack something into the JS engine to work around these things, like each one of them checking both a pref and some global variable, and add some interface to modifying those global variables from the jsreftest harness, but that seems more complicated and bug-prone to me than the prefs approach in this patch.

I reached the opposite conclusion! :) We just need to add some things to nsJSEnvironment. I'll write a patch. That these tests are playing with prefs looks like a giant hack to me. Removing the layer of pref indirection in favor of something more direct would I think make everyone's lives easier in the long run.

chris, do you know if applications that build on top of gecko depend on these preferences working? do the jsreftests not only test the functionality of the jsengine, but also that the named preferences work as expected?

You're asking two very different questions. It's become clear that jsreftest's browser.js is very broken. I'm fixing it. Any tests that depended on the JIT state being correct may or may not have been broken for N months. Also any tests that depended on gc() doing what it said have been broken too. This means that jsreftest tests may *fail* after the fixes here, because they've regressed in the meantime while browser.js was broken. Let's hope not. I have no idea whether applications that build on gecko (do you have something more specific in mind?) depend on these prefs working. Are you asking about those apps that also use content processes, in which these prefs will no longer work? To my knowledge, firefox is the only gecko app that uses content processes.

Just to make the next patch simpler.

Before this patch, 15 tests failed, mostly for strict mode not being on. There was one unexpected pass after this patch, because options('atline') works again. Thankfully everything looks OK. This patch tries to make sure tests, C.u, and browser.js all keep their sets of JSContext options in sync.

Comment on attachment 545793 [details] [diff] [review] part 2: Implement jsreftest browser.js JSContext-munging on top of Components.utils helpers Looks fine to me and passes jsreftests on Mac debug with all patches applied but I would want someone from the JS team to look at this as well. dmandelin? jmaher: this still uses enablePrivilege. Won't that need to be replaced with Special Powers anyway?

we need to get rid of all enablePrivilege calls. There is some hesitation about doing that with special powers in reftest, but it might be possible to do it just for jsreftests. Otherwise we would need to write some messageManager code to talk between content and messageManager to handle these things. Currently, there is code in the reftest harness to communicate to/from the content process, so it might be easier to just add some preference handling there.

Whoa whoa whoa, wait a second: this new code just needs to access simple attributes on Components.utils. That was the whole point of this approach; see comments 4 through 8. (There are other benefits too, like it being faster and less broken.) We don't have to deal with prefs here. We will in bug 621363. Let's figure out how to use these new C.u interfaces with SpecialPowers, if need be. I don't see much urgency.

jst: review ping.

Comment on attachment 545790 [details] [diff] [review] part 1: Add Components.utils interfaces for playing with callee JSContext params This totally fell off my list of things to review, very sorry for the delay here :(

Hmmmmmm ... I noticed that "jsreftests" J2 (I assume that's the buildbot name for jstestbrowser?) are running on tinderbox, and I see REFTEST INFO | [CONTENT] Using browser remote=true in the log. Are the pref-setting tests disabled or are we using some other trickery? I'm about to land these patches and would love to get the full test suite enabled.

I am not aware of any trickery, I do know we have 76 tests disabled for android (some due to memory).

https://hg.mozilla.org/integration/mozilla-inbound/rev/694aacb1a14f https://hg.mozilla.org/integration/mozilla-inbound/rev/e31bbb9dbc92

https://hg.mozilla.org/mozilla-central/rev/694aacb1a14f https://hg.mozilla.org/mozilla-central/rev/e31bbb9dbc92

The new Components.utils properties should be mentioned on https://developer.mozilla.org/en/Components.utils

Documentation updated: https://developer.mozilla.org/en/Components.utils.setGCZeal https://developer.mozilla.org/en/Components.utils https://developer.mozilla.org/en/Components_object Updated Firefox 10 for developers.