The default bug view has changed. See this FAQ.

JS browser tests try to set prefs in the child process

RESOLVED FIXED in Firefox 10

Status

Fennec Graveyard
General
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: azakai, Assigned: cjones)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
Firefox 10
dev-doc-complete

Details

(Whiteboard: [inbound][QA-])

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Depends on: 669954
(Reporter)

Comment 1

6 years ago
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.
(Reporter)

Comment 2

6 years ago
Created attachment 545251 [details] [diff] [review]
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.
Attachment #545251 - Flags: review?(doug.turner)

Comment 3

6 years ago
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.
Attachment #545251 - Flags: review?(doug.turner) → review+

Comment 4

6 years ago
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.
Assignee: nobody → azakai
(Reporter)

Comment 5

6 years ago
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.
(Reporter)

Comment 7

6 years ago
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.

Comment 9

6 years ago
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.
Created attachment 545790 [details] [diff] [review]
part 1: Add Components.utils interfaces for playing with callee JSContext params
Assignee: azakai → jones.chris.g
Attachment #545790 - Flags: superreview?(jst)
Attachment #545790 - Flags: review?(mrbkap)
Created attachment 545791 [details] [diff] [review]
part 1.99: Excise prefs and broken code from browser.js. TO BE FOLDED

Just to make the next patch simpler.
Created attachment 545793 [details] [diff] [review]
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.
Attachment #545793 - Flags: review?
Attachment #545793 - Flags: review? → review?(bclary)

Comment 14

6 years ago
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?
Attachment #545793 - Flags: review?(dmandelin)
Attachment #545793 - Flags: review?(bclary)
Attachment #545793 - Flags: review+
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.
Blocks: 671557
Attachment #545793 - Flags: review?(dmandelin) → review+

Updated

6 years ago
Attachment #545790 - Flags: review?(mrbkap) → review+
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 :(
Attachment #545790 - Flags: superreview?(jst) → superreview+
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.
No longer depends on: 669954
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
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/694aacb1a14f
https://hg.mozilla.org/mozilla-central/rev/e31bbb9dbc92
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10

Comment 23

6 years ago
The new Components.utils properties should be mentioned on https://developer.mozilla.org/en/Components.utils
Keywords: dev-doc-needed

Updated

6 years ago
Whiteboard: [inbound] → [inbound][QA?]

Updated

6 years ago
Whiteboard: [inbound][QA?] → [inbound][QA-]
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.
Keywords: dev-doc-needed → dev-doc-complete

Updated

2 years ago
Duplicate of this bug: 592064
You need to log in before you can comment on or make changes to this bug.