669949 - JS browser tests try to set prefs in the child process

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alon Zakai (:azakai)]

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.

Comment 1

[Alon Zakai (:azakai)]

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.

Comment 2

[Alon Zakai (:azakai)]

Attachment: patch

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 3

[Doug Turner (:dougt)]

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.

Comment 4

[Doug Turner (:dougt)]

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.

Comment 5

[Alon Zakai (:azakai)]

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?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 7

[Alon Zakai (:azakai)]

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.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 9

[Doug Turner (:dougt)]

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?

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add Components.utils interfaces for playing with callee JSContext params

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1.99: Excise prefs and broken code from browser.js. TO BE FOLDED

Just to make the next patch simpler.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Implement jsreftest browser.js JSContext-munging on top of Components.utils helpers

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 14

[Bob Clary [:bc] (inactive)]

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?

Comment 15

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

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.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

jst: review ping.

Comment 18

[Johnny Stenback (:jst)]

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 :(

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 20

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

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

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[Nickolay_Ponomarev]

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

Comment 24

[Eric Shepherd [:sheppy]]

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.