Last Comment Bug 445873 - Components.utils.Sandbox doesn't set JS version
: Components.utils.Sandbox doesn't set JS version
Status: RESOLVED FIXED
: fixed1.9.1
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla1.9.1b3
Assigned To: Atul Varma [:atul]
:
:
Mentors:
: 453036 494528 (view as bug list)
Depends on: 484459
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-17 15:33 PDT by Atul Varma [:atul]
Modified: 2009-06-03 12:05 PDT (History)
10 users (show)
benjamin: blocking1.9.1-
benjamin: wanted1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First attempt at a patch. (3.45 KB, patch)
2008-11-26 16:26 PST, Atul Varma [:atul]
mrbkap: review+
Details | Diff | Splinter Review
Latest version of patch, using JS_ConvertArguments() (4.28 KB, patch)
2009-01-13 17:12 PST, Atul Varma [:atul]
no flags Details | Diff | Splinter Review
xpcshell tests that ensure that the patch works properly. (1.84 KB, text/plain)
2009-01-13 17:15 PST, Atul Varma [:atul]
no flags Details
Patch w/ mrbkap's nit fixed. (4.26 KB, patch)
2009-01-13 17:17 PST, Atul Varma [:atul]
mrbkap: review+
Details | Diff | Splinter Review
Latest patch using "ok" as the JSBool (3.97 KB, patch)
2009-01-15 13:48 PST, Atul Varma [:atul]
mrbkap: review+
mrbkap: superreview+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review

Description Atul Varma [:atul] 2008-07-17 15:33:33 PDT
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) 2008-07-18 03:08:14 PDT
Sounds reasonable to me.
Comment 2 Simon Bünzli 2008-08-31 10:12:10 PDT
*** Bug 453036 has been marked as a duplicate of this bug. ***
Comment 3 Atul Varma [:atul] 2008-11-14 11:51:43 PST
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) 2008-11-20 19:29:43 PST
Atul, were you going to look into fixing this?
Comment 5 Atul Varma [:atul] 2008-11-26 10:42:39 PST
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] 2008-11-26 16:26:57 PST
Created attachment 350241 [details] [diff] [review]
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] 2008-12-07 09:24:02 PST
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] 2008-12-10 09:50:24 PST
I just found out yesterday that my patch also appears to resolve bug 307984.
Comment 9 Blake Kaplan (:mrbkap) 2008-12-10 13:46:36 PST
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] 2008-12-10 14:10:06 PST
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] 2008-12-15 17:43:08 PST
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] 2008-12-16 11:17:43 PST
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 :Ehsan Akhgari 2008-12-17 01:06:04 PST
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) 2008-12-17 11:32:21 PST
(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] 2009-01-13 17:12:00 PST
Created attachment 356857 [details] [diff] [review]
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] 2009-01-13 17:15:39 PST
Created attachment 356859 [details]
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] 2009-01-13 17:17:56 PST
Created attachment 356860 [details] [diff] [review]
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] 2009-01-13 18:30:57 PST
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) 2009-01-14 11:16:50 PST
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] 2009-01-15 13:48:34 PST
Created attachment 357227 [details] [diff] [review]
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] 2009-01-15 13:50:35 PST
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) 2009-01-15 16:16:36 PST
(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) 2009-01-15 16:17:23 PST
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] 2009-01-30 13:22:43 PST
Landed on the trunk:

http://hg.mozilla.org/mozilla-central/rev/38c9c91e6ba0
Comment 25 Brendan Eich [:brendan] 2009-01-30 13:34:24 PST
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] 2009-02-05 21:55:30 PST
Comment on attachment 357227 [details] [diff] [review]
Latest patch using "ok" as the JSBool

a191=beltzner
Comment 27 Atul Varma [:atul] 2009-02-09 14:30:39 PST
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] 2009-02-09 15:07:42 PST
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) 2009-02-09 15:16:20 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0eb4b4bcc7c7
Comment 30 Atul Varma [:atul] 2009-06-03 12:05:22 PDT
*** Bug 494528 has been marked as a duplicate of this bug. ***

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