Closed Bug 445873 Opened 16 years ago Closed 16 years ago

Components.utils.Sandbox doesn't set JS version


(Core :: XPConnect, defect)

Not set





(Reporter: avarma, Assigned: avarma)



(Keywords: fixed1.9.1)


(2 files, 3 obsolete files)

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.



Sounds reasonable to me.
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?

Atul, were you going to look into fixing this?
Yeah, sorry about the delay on this one... I'll be submitting a patch today or tomorrow if that's okay.
Attached patch First attempt at a patch. (obsolete) — Splinter Review
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.
Assignee: nobody → avarma
Attachment #350241 - Flags: review?(mrbkap)
Since this feature is integral to the upcoming release of Ubiquity, I nominate this for blocking on Firefox 3.1.
Flags: blocking1.9.1?
I just found out yesterday that my patch also appears to resolve bug 307984.
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.
Attachment #350241 - Flags: superreview?(brendan)
Attachment #350241 - Flags: review?(mrbkap)
Attachment #350241 - Flags: review+
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.

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.

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.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
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.
(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.
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.
Attachment #350241 - Attachment is obsolete: true
Attachment #356857 - Flags: review?(mrbkap)
Attachment #350241 - Flags: superreview?(brendan)
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.
Attached patch Patch w/ mrbkap's nit fixed. (obsolete) — Splinter Review
Oops, forgot to fix mrbkap's nit in my last patch, so this fixes it.
Attachment #356857 - Attachment is obsolete: true
Attachment #356860 - Flags: review?(mrbkap)
Attachment #356857 - Flags: review?(mrbkap)
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.

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

r=me with brendan's nits picked.
Attachment #356860 - Flags: review?(mrbkap) → review+
Just like the last patch, only with "ok" as the variable name for the return value of JS_ConvertArguments().
Attachment #356860 - Attachment is obsolete: true
Attachment #357227 - Flags: review?(mrbkap)
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?
(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.
Attachment #357227 - Flags: superreview+
Attachment #357227 - Flags: review?(mrbkap)
Attachment #357227 - Flags: review+
Comment on attachment 357227 [details] [diff] [review]
Latest patch using "ok" as the JSBool

Looks good. r+sr=mrbkap, thanks!
Keywords: checkin-needed
Landed on the trunk:
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.1b3
Comment on attachment 357227 [details] [diff] [review]
Latest patch using "ok" as the JSBool

Bug is wanted.

Attachment #357227 - Flags: approval1.9.1?
Attachment #357227 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 357227 [details] [diff] [review]
Latest patch using "ok" as the JSBool

I'm adding another checkin-needed keyword, just in case it's needed to push the patch to the 1.9.1 branch.
Keywords: checkin-needed
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 access request in due course!

Depends on: 484459
You need to log in before you can comment on or make changes to this bug.