500491 - Can't permanently set any options within the javascript shell using options()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11) Gecko/2009060214 Firefox/3.0.11
Build Identifier: mozilla-central Revision 42e818e6da7f

In the interactive shell, options are reset after each line.
So, typing "options("relimit");options()" will yield "relimit", but saying "options("relimit")" [newline] "options()" will yield whatever options the shell was run with.
Running the shell with -f and specifying a file that sets options seems to work fine.

Reproducible: Always

Steps to Reproduce:
1. Open the javascript shell.
2. Type "options("relimit")" (or any other option)
3. Hit return
4. Type "options()"
5. Hit return

Actual Results:  
The output will not include "relimit" (unless it was passed in as an option when opening the shell).

Expected Results:  
Expected output is "relimit" (and whatever other options were present).

Comment 1

[Brendan Eich [:brendan]]

Yikes! This is a regression. Can someone bisect and take the bug and fix it?

1.9.0 shell:

js> options('strict'); options()
strict
js> options()
strict
js> 

We have intentional support for sticky options, for the testsuite:

js> function enable() { options('strict') }
js> options()

js> enable()
js> options()
strict

This too is broken. Bob, didn't a test or three fail on this account?

David, thanks very much for reporting this.

/be

Comment 2

[Brendan Eich [:brendan]]

Sorry, my fault for not catching during review of patch for bug 497060.

Graydon, could you fix? Spot fix would not slam in oldOptions, rather just restore JSOPTION_JIT iff it was set on entry to js_Execute.

But in bug 497060 comment 32 I raised an issue not addressed before that bug was closed, about event handlers, which can enter the VM via JS_CallFunction* APIs, with deep native scope chains. We should fix that too, it seems like it could make for the same problem.

If there's a fix down in jstracer.cpp then it would beat this option-frobbing a layer or three up.

/be

Comment 3

[Graydon Hoare :graydon]

Yes of course, I had no idea script could set options *during* evaluation; I thought it was a passed-in sort of thing only. Sorry.

I also see now that you were saying something different about event handlers than I thought. I thought the old question was about whether event handlers that we *want* to wind up JIT'ed would accidentally fall under the influence of the former patch, and that inspecting JSOPTION_VAROBJFIX might help get them out from that influence. I see now you meant that JS_ExecuteFunction is an entirely independent path into the tracer that the old patch didn't cover! That's much worse. I'll file a folllowup and fix.

I did look a bit for somewhere in jstracer to stick this check, but it seems like the relevant initial-conditions of interpretation cannot be inferred at that level. It all just looks like a scope chain.

Comment 4

[Graydon Hoare :graydon]

Attachment: Fix the bug

Comment 5

[Bob Clary [:bc] (inactive)]

(In reply to comment #1)

> This too is broken. Bob, didn't a test or three fail on this account?

I have a *bunch* of browser regressions on tracemonkey and mozilla-central that I've been tracking since about 6/20. I don't see this on mozilla-1.9.1. I'm bisecting them now but haven't seen any in the shell like this. Looking at the lists of tests which use options() and the lists of regressions I've seen they do match up pretty closely. 

The reason I haven't reported them yet is the machine I was using for the testing and bisection couldn't reproduce the regressions. It was upgraded to Fedora 11 on Sunday/Monday and I just realized today that the new kernel values were throwing my test failure and regression logic off.

Comment 6

[Bob Clary [:bc] (inactive)]

The tests (browser only) affected by this are:

ecma/LexicalConventions/7.4.3-14-n.js
ecma/LexicalConventions/7.4.3-15-n.js
ecma/LexicalConventions/7.4.3-4-n.js
ecma/LexicalConventions/7.4.3-7-n.js
ecma/LexicalConventions/7.4.3-9-n.js
ecma_2/Exceptions/lexical-011.js
ecma_2/Exceptions/lexical-014.js
ecma_2/Exceptions/lexical-016.js
ecma_2/Exceptions/lexical-021.js
ecma_2/LexicalConventions/keywords-001.js
ecma_3/Object/8.6.1-01.js
js1_5/Exceptions/regress-315147.js
js1_5/Regress/regress-306633.js
js1_5/Regress/regress-317533.js
js1_5/Regress/regress-323314-1.js
js1_5/Regress/regress-352197.js
js1_5/extensions/regress-330569.js
js1_5/extensions/regress-351448.js
js1_5/extensions/regress-361964.js
js1_5/extensions/regress-365869.js
js1_5/extensions/regress-367923.js
js1_5/extensions/regress-382509.js
js1_7/block/regress-347559.js

Comment 7

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/a80b7a997401

Comment 8

[Bob Clary [:bc] (inactive)]

this is not fixed in tracemonkey. e.g. http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=ecma_3%2FObject%2F8.6.1-01.js;language=type;text/javascript still fails (as do the others).

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #3)
> Yes of course, I had no idea script could set options *during* evaluation; I
> thought it was a passed-in sort of thing only. Sorry.

Many tests require setting and resetting options during run time.

I think the reason the shell tests didn't show the failure is that the tests explicitly set the options to be used and do not call options() without an argument.

While http://hg.mozilla.org/tracemonkey/rev/a80b7a997401 did fix the problem in the shell, it is no longer possible in the browser to dynamically change the javascript.options at run time by manipulating preferences which breaks not only tests of strict or relimit but also the jit tests.

Comment 10

[Graydon Hoare :graydon]

(In reply to comment #9)

> the shell, it is no longer possible in the browser to dynamically change the
> javascript.options at run time by manipulating preferences which breaks not
> only tests of strict or relimit but also the jit tests.

You mean ... to dynamically change the JIT-ness of the options? The remainder should be settable from script now.

Comment 11

[Bob Clary [:bc] (inactive)]

No, any option. I looked at the patch and I don't see any reason it would cause the browser to fail. But http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=ecma_3%2FObject%2F8.6.1-01.js;language=type;text/javascript fails in today's tracemonkey on mac and all it cares about is strict mode.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a80b7a997401

Comment 13

[Graydon Hoare :graydon]

This didn't get "fully" fixed with the above commit, sadly. Reopening until I've figured out why bc is still seeing browser breakage in comment 8, comment 9 and comment 11.

Comment 14

[Brendan Eich [:brendan]]

Suggest a followup bug, esp. since this bug has been patched in tm and the patch is now in m-c. Voice of experience, I still have trouble listening to it, but it tends to pay off in spite of bug overhead.

/be

Comment 15

[Graydon Hoare :graydon]

Followup filed as bug 502675, closing this one as suggested.