445873 - Components.utils.Sandbox doesn't set JS version

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Atul Varma [:atul]]

It appears that the code which creates JS sandboxes [1] doesn't explicitly call JS_SetVersion (nor does the context callback for xpconnect [2]), and it further doesn't provide any mechanism for JS code to set the version.  As a result, it appears that all sandboxes created in Firefox don't accept JS 1.7 code (namely, use of 'let' and 'yield' statements raise syntax errors).

Note that sandboxes created using xpcshell *do* accept JS 1.7 code because xpcshell defines a context callback [3] that sets the JS version of all new contexts to the latest version.  This effectively means that xpcshell tests using sandboxes may perform significantly differently when run in xpcshell than they do when run in Firefox. 

To make the behavior consistent across both xpcshell and Firefox, I recommend that xpc_CreateSandboxObject() explicitly use JS_SetVersion() to set the JS version of new sandboxes to use whatever pre-1.7 JS version Firefox currently defaults to (since we don't want to break existing production code), and also allow for an optional version parameter to be passed in to Components.utils.Sandbox(), which overrides this default behavior.

If this sounds reasonable, I'd be happy to submit a patch that implements it.

[1] http://mxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpccomponents.cpp#3150

[2] http://mxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpcjsruntime.cpp#234

[3] http://mxr.mozilla.org/mozilla/source/js/src/xpconnect/shell/xpcshell.cpp#1305

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Sounds reasonable to me.

Comment 3

[Atul Varma [:atul]]

mrbkap, the motivation for this bug is for Ubiquity; I've created an XPCOM component for it for now [1].  The problem is, as you've told me, that the JS API provides source-level backwards compatibility but not binary-level compatibility, and as a result, we'd have to separately compile different versions of our XPCOM component for FF 3.0 and 3.1, which is an enormous hassle.

If at all possible, I'd like to throw another feature request into the Sandbox API so that Ubiquity won't need a binary component for FF 3.1 (assuming I can shepherd the patch quickly enough and you'd allow this, of course).  Ubiquity command feeds currently suffer from a major problem in that it's impossible to derive meaningful filename and line-number information from JS tracebacks because the command feeds are executed via Components.utils.evalInSandbox(), which ties the evaluated code to the filename and line number of the calling code.  So it'd be really nice if the patch (or perhaps a separate one, whichever's most convenient for you) could include two other optional parameters to specify the filename and line number, to make debugging easier for Ubiquity command feeds (and maybe even other code that's executed in sandboxes elsewhere in Firefox).

Do you think this would be possible?  Or can you think of a better solution?

[1] http://hg.toolness.com/ubiquity-firefox/file/173901c46db7/components/

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Atul, were you going to look into fixing this?

Comment 5

[Atul Varma [:atul]]

Yeah, sorry about the delay on this one... I'll be submitting a patch today or tomorrow if that's okay.

Comment 6

[Atul Varma [:atul]]

Attachment: First attempt at a patch.

Here's a first stab at a patch.  The new Components.utils.evalInSandbox() has the following signature, with the parameters in square brackets being optional:

  Components.utils.evalInSandbox(source, sandbox, [jsVersion], [filename], [lineNumber])

'jsVersion' is optional and should be a string corresponding to a JS version, e.g. "1.5", "1.7", etc.  Pass in "undefined" to leave this unspecified.

'filename' and 'lineNumber' are optional and correspond to the filename and line number of the beginning of the code being evaluated.

There's a few edge cases that I'd like advice on, so I don't expect it to pass review:

  * For the filename and JS version string, I'm using JS_GetStringBytes(), which could result in a strange string if the passed-in strings contain non-ASCII characters.  Is there some other function I should use to convert the JSString *'s into char *'s?

  * The line number parameter doesn't work if a string containing the line number is passed in (rather than an actual int).  Since JS is very loosely-typed and (afaik) doesn't have a formal integer-coercion function, perhaps I should do the conversion on the C side?

  * How should client code using evalInSandbox() determine whether the optional arguments are supported?  Should it just look at the version of the Gecko platform currently being used, or should they execute trivial code that uses an optional argument and see if an exception is thrown, or is there a cleaner way?

More limitations:

  * This new functionality has no unit tests; I'd like to add some.

  * I haven't yet added documentation to the IDL file, MDC, or anything else about the new calling convention.

Please let me know if there's anything else you'd like me to change.

Comment 7

[Atul Varma [:atul]]

Since this feature is integral to the upcoming release of Ubiquity, I nominate this for blocking on Firefox 3.1.

Comment 8

[Atul Varma [:atul]]

I just found out yesterday that my patch also appears to resolve bug 307984.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 350241 [details] [diff] [review]
First attempt at a patch.

>diff -r bb54a5700bca js/src/xpconnect/src/xpccomponents.cpp
>@@ -3517,7 +3535,8 @@
> nsresult
> xpc_EvalInSandbox(JSContext *cx, JSObject *sandbox, const nsAString& source,
>                   const char *filename, PRInt32 lineNo,
>-                  PRBool returnStringOnly, jsval *rval)
>+                  JSVersion jsVersion, PRBool returnStringOnly, jsval *rval
>+                  )

Nit: move the closing parenthesis back up to the previous line.

I'm asking brendan for sr, since he had some thoughts about this problem a while ago.

Comment 10

[Brendan Eich [:brendan]]

Use JS_DecodeBytes for UTF-8 future proofing.

For the line number argument, use JS_ValueToECMAUint32. Or better yet, use JS_ConvertArguments to do all the arg processing.

It might be better to add a new entry point you can object-detect, instead of adding trailing optional parameters to evalInSandbox.

Trailing optional parameters may work well in this case, but if order of param and utility are not correlated, an object parameter {version:1.7}, {version:1.8, filename:"...", lineno:...}, etc. could be better.

/be

Comment 11

[Brendan Eich [:brendan]]

Atul, are you gonna re-patch? I'm leaving the sr? in my queue but could minus. Leaving the request pending helps me find this bug.

/be

Comment 12

[Atul Varma [:atul]]

I will be re-patching, but I'm currently busy with some other priorities so I won't be able to get around to it today, and probably not tomorrow either--so you're welcome to minus it, and then I can re-request a new review when I re-patch.  Whatever's most convenient for you.

Comment 13

[(no longer active)]

Is this bug the reason behind these errors which I see in my error console (3.1b2)?

Error: missing ; before statement
Source File: file:///C:/Program%20Files/Mozilla%20Firefox%203.1/modules/PluralForm.jsm
Line: 70
Source Code:
let gFunctions = [ 

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIPrefBranch2.addObserver]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://mozapps/content/extensions/extensions.js :: Startup :: line 989"  data: no]

I see these when opening the Add-ons manager, and similar ones for Download manager, and any other piece of code which uses the PluralForm.jsm which uses |let| in its code.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #13)
> Is this bug the reason behind these errors which I see in my error console
> (3.1b2)?

No, that file doesn't use evalInSandbox. At a guess, you're seeing bug 467747.

Comment 15

[Atul Varma [:atul]]

Attachment: Latest version of patch, using JS_ConvertArguments()

Changed the patch to use JS_ConvertArguments().  I'm still unsure of how memory is being managed re: the string conversions that are going on here, however.

Comment 16

[Atul Varma [:atul]]

Attachment: xpcshell tests that ensure that the patch works properly.

Added a simple test suite for this patch.  They're not actually xpcshell framework tests; this is just an executable script that makes sure the patch works.

Comment 17

[Atul Varma [:atul]]

Attachment: Patch w/ mrbkap's nit fixed.

Oops, forgot to fix mrbkap's nit in my last patch, so this fixes it.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 356860 [details] [diff] [review]
Patch w/ mrbkap's nit fixed.

Drive-by nits:

>+    JSBool wasConvSuccess = JS_ConvertArguments(cx,
>+                                                argc,
>+                                                argv,
>+                                                "*o/ssi",
>+                                                &sandbox,
>+                                                &jsVersionStr,
>+                                                &filenameStr,
>+                                                &lineNo);

Canonical JSBool return value status variable name is "ok" -- shorter to boot, which allows less strung-out argument-per-line style (unless that was intended no matter how far over the first arg started?).

The stack at argv roots the object and strings, btw.

/be

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 356860 [details] [diff] [review]
Patch w/ mrbkap's nit fixed.

r=me with brendan's nits picked.

Comment 20

[Atul Varma [:atul]]

Attachment: Latest patch using "ok" as the JSBool

Just like the last patch, only with "ok" as the variable name for the return value of JS_ConvertArguments().

Comment 21

[Atul Varma [:atul]]

Brendan, I'm not sure what you meant by your last statement--that the stack at argv roots the object and strings.  Could you clarify?

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #21)
> Brendan, I'm not sure what you meant by your last statement--that the stack at
> argv roots the object and strings.  Could you clarify?

That was a response to your question about the ownership of the passed-back strings. argv is a rooted array of jsvals and the returned-string is owned by one of the entries in argv.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 357227 [details] [diff] [review]
Latest patch using "ok" as the JSBool

Looks good. r+sr=mrbkap, thanks!

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Landed on the trunk:

http://hg.mozilla.org/mozilla-central/rev/38c9c91e6ba0

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 357227 [details] [diff] [review]
Latest patch using "ok" as the JSBool

Bug is wanted.

/be

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 357227 [details] [diff] [review]
Latest patch using "ok" as the JSBool

a191=beltzner

Comment 27

[Atul Varma [:atul]]

I'm adding another checkin-needed keyword, just in case it's needed to push the patch to the 1.9.1 branch.

Comment 28

[Brendan Eich [:brendan]]

I think that's the right protocol. At least, I hope omeone will push this - the bug is marked wanted and the patch has approval.

Atul, you should start an hg.mozilla.org access request in due course!

/be

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0eb4b4bcc7c7