613383 - Setting runtime JS options can override the JSVersion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

See bug 596580 comment 32, and the testcase in bug 596580 comment 6 -- the let versioning fix from the browser is reported to not have held up on XULRunner.

Comment 1

[Daniel Kirsch]

I can still reproduce the original problem from bug 596580 using the testcase from attachment 475872 [details] (posted for bug 596580 as well).

The regression range is identical:
Works: 20100914 id=5588c9796f0b
Fails: 20100915 id=0caec4ddff74

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5588c9796f0b&tochange=0caec4ddff74

The testcase tries to load a file that doesn't exist. After that a second,
existing file will be loaded using a setTimeout call. Using this combination I
was able to reproduce the behaviour in my simplified testcase.
The testcase needs to be placed in a chrome://test/content/ directory.

When running the example, a message should be shown:
  Error in notfound.js
  Error creating URI (invalid URL scheme?)

Then two message should be shown
  "before let"
and
  "after let: let is ok"

Currently the first message is shown then a second message appears saying:

Error in chrome://test/content/let/let.js
SyntaxError: missing ; before statement

I've also updated the full example to download (Windows) with the latest XULRunner nightly 20101122:
http://birgin.de/test/bugzilla/613383_full_testcase.zip

Comment 2

[Daniel Kirsch]

It seems like in some cases the JS version information will be reset so the "let" keyword will not be handled correctly anymore. In the testcase this case is reproduced by trying to load a non existing file.

Comment 3

[Daniel Kirsch]

Chris, can you reproduce the error?
This bug is blocking our current development. If I can be of any help, please let me know.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

So, what I've found so far: we're failing to recognize |let| as a keyword because the context's JSVersion is set to JSVERSION_DEFAULT, and |let| requires >= 1.7. The correct answer here is 1.8 and not JSVERSION_DEFAULT, per the XUL script tag.

We seem to get that JSVERSION_DEFAULT (i.e. 0) from nsJSScriptTimeoutHandler::mVersion before we call into scx->EvaluateString in nsGlobalWindow. I'm looking into the reason why mVersion isn't initialized to the value one would expect.

Comment 5

[Daniel Kirsch]

BTW: Is there a pref or another way to set JSVERSION_DEFAULT? At least in my case this would be a workaround and it would make it possible to handle with keywords like let without explicitly defining the version in every script tag.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #5)
> BTW: Is there a pref or another way to set JSVERSION_DEFAULT?

I don't think so -- it seems pretty well baked in to XPConnect as a constant, so it's basically just a property of the libxul you're using.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

Okay, so, here's the mess:

- The thing that becomes the timeout (and calls |init()|) is compiled with JSVERSION_DEFAULT.
- The alert gets triggered under Interpret/CallJSNative -> XPC_WN_CallMethod -> CallMethodHelper::Invoke -> nsGlobalWindow::Alert -> do_GetService -> mozJSComponentLoaderFactory -> nsXPCWrappedJS::CallMethod -> adds "don't report uncaught option"
- The option-to-version sync realized that the options and currently-executing-version were out of sync (the script on the stack had XML enabled but the options had it disabled) so it overrode the version to JSVERSION_1_8:NOXML:ANONFUNFIX.
- After this, all the calls to JS_SetVersion are overrides -- when we reach init() the version is overridden to JSVERSION_DEFAULT which causes the mozJSSubScriptLoader's findVersion to get the answer that we don't want.

The (fortunately fairly forward-facing) immediate solution is to provide a "scoped option change" C++ API and a "we hope you will actually scope these" C API that guarantee not to leave the version overridden. I think I'll see how hard it is to axe JS_ToggleOptions while I'm in there.

The crux of the problem is that we conflate runtime options with compile options within cx->options -- as a result, when you go to update something irrelevant, like DONT_REPORT_UNCAUGHT, you end up syncing options to the version of the script on the stack and unintentionally force an override.

I'll update the "kill JS_SetVersion" bug (bug 595691) with more information as to the ultimate solution.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #7)
> I'll update the "kill JS_SetVersion" bug (bug 595691) with more information as
> to the ultimate solution.

See bug 595691 comment 5.

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Auto runtime options in xpcwrappedjs.

Fixes the XULRunner test case, will write a test and throw it up on try.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

Passed on try: http://hg.mozilla.org/try/rev/f1ef8a50372f

Will whip up a quick JSAPI test and put up for review.

Comment 11

[Daniel Kirsch]

How can I test this or is it not yet testable? (I currently cannot create custom builds)

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 496333 [details] [diff] [review]
Auto runtime options in xpcwrappedjs.

On reflection, there's not a good (non-obvious) test to write. The AutoRuntimeOptions should not set a version override, but it's clear that won't happen by construction.

The only interesting corner case is that the options can be further changed once AutoRuntimeOptions is in scope, but will be clobbered with oldOptions (from the point of the AutoRuntimeOptions scope) indiscriminately. This conforms to the old behavior we're replacing, so it's no more hazardous than it used to be.

There are more JS_SetOptions that I'm removing as we speak as part of my bug 595691 queue, so this is a stopgap fix for the xpcwrappedjs/mozJSComponentLoader alone. Other uses to fix include pref changes and worker object wrappers.

Comment 13

[Daniel Kirsch]

This all sounds very complicated. Why not just set the version with the latest script that defines a version and make this behaviour part of the standard?

eg:
<script type="application/x-javascript;"/>
// JSVERSION_DEFAULT will be used

<script type="application/x-javascript; version=1.8;"/>
// Now 1.8 is used

<script type="application/x-javascript;"/>
// still use 1.8

<script type="application/x-javascript; version=1.7;"/>
// Hmmm... why not still use 1.8?

IMHO the highest defined version should be used and that's the author's responsibillity.

Hmmm... thinking about that it could be a problem in a chrome code because my code uses 1.5 while another extension or the chrome://global code uses 1.8. 
But what's the actual problem? Why do I need to define the version anyway and why isn't it always set to the highest available version? Only because of "let" and "yield"? If so, this could be part of a new release (at least in a chrome environment). A lot of things changed within the last years. I had to adjust my application for every new release, so it would be no big problem to fix this as well.

Just some thoughts...

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #13)

It is complicated, but JS_SetVersion removal will simplify things.

We use the same JS engine for content and chrome code, so we need to support script tags with different versioning anyway -- no good reason to break backwards compatibility there.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 496333 [details] [diff] [review]
Auto runtime options in xpcwrappedjs.

I'd have some nits if this wasn't a stopgap solution, but since this will be wholly subsumed by bug 595691, this should be fine.

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

I'm closing this as a duplicate of bug 595691 because that solves a superset of the issues and is blocking beta 9 (so it'll be in soon, anyway) -- Daniel, I can submit to try server to create a custom build for you if you can't make one yourself.

Comment 17

[Daniel Kirsch]

Chris, a testable version would be cool just to check, if the issue is solved in our complex environment too, but this isn't really urgent right now as nothing will be changed here before the end of the year.

Comment 18

[Georg Maaß]

With XulRunner 2.0b10pre on MacOS X BuildID=20110121030329 I also have this problem. As all libraries are loaded on first use via subscript loader application fails when loading the first library, because all libraries make heavy use of let statements. As result of this application already failes during startup so can not fetch the build identifier from about page, but took it from platform.ini inside XulRunner.

This bug makes XulRunner 2.0 currently unusable for my Application too.

Comment 19

[Georg Maaß]

The problem has to do with the setTimeout, which is used in onload handler of the window to give the GUI time to draw disabled static GUI elements before loading the code. It is not necessary to load a not existing file to get this trouble. Just doing the subscript loading triggered by a setTimeout is enough for this issue.

Comment 20

[Chris Leary [:cdleary] (not checking bugmail)]

Not sure what dmandelin's reasoning was for reopening after duping (probably just slips my mind), but a reduced test case for bug 506138 is included in bug 595691 attachment 506138 [details] [diff] [review].

Georg, if you can provide another reduced testcase we can add that in as well. Otherwise, I think this bug is just waiting for bug 595691 to resolve/fixed.

Comment 21

[Chris Leary [:cdleary] (not checking bugmail)]

Fixed with test case by landing for parent bug 595691. Please file a new bug for any outstanding issues.