302834 - Components.utils.evalInSandbox should return result, throw exception

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Axel Hecht]

We chatted about this a bit, and evalInSandbox should return the result of the
expression like eval does.
To make room for the return value, the sandbox creation should be in an extra
method on Components.utils.

I also found that evalInSandbox throws an NS_ERROR_FAILURE for errors in the
string to be evaluated instead of the original exception. (Not sure if this
should be a separate bug.)

Comment 1

[Axel Hecht]

If I read the code right, the exception stuff is due to 
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpccomponents.cpp#2223,
the call to cc->SetReturnValueWasSet(PR_TRUE); only happens on success.

The other thing I'm wondering, why do we give a new principal for each eval?
That sounds like an invitation to tunnel data from one principal to the other
via the sandbox. I wonder if the URI should only be given for the sandbox 
creation.

Comment 2

[Axel Hecht]

I filed bug 303108 on the exception issue.

Comment 3

[Brendan Eich [:brendan]]

We need to get this right for 1.8, it's a new API.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Here is the proposed interface:

var sandbox = new Components.utils.sandbox(principalURI);
var result = Components.utils.evalInSandbox("foo", sandbox);
// ...
result = Components.utils.evalInSandbox("bar", sandbox);

None of the parameters are optional, since it's hard to make this a real
"sandbox". Comments are welcome.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Brendan pointed out that it should be "Components.utils.Sandbox", not
"Components.utils.sandbox". I'll start hacking on a patch for this tomorrow.

Comment 6

[Axel Hecht]

Regarding the optionality, we could default to about:blank for the principalURI
in the sandbox cration, and in the evalInSandbox, default to a blank sandbox for
a about:blank principal.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: attempt to implement 1

Now that I understand the XPConnect stuff, this patch took longer to generate
than it probably should have. This is an attempt to implement the interface
described in comment 4.

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 193625 [details] [diff] [review]
attempt to implement 1 

> /**
>+* interface of object returned by Components.utils.Sandbox.

...of object constructed by Components.utils.Sandbox

>+     * evalInSandbox evaluates the provided source string in the given sandbox.
>+     * It returns the result of the evaluation to the caller.
>      *
>-     * var s = C.u.evalInSandbox("var five = 5", "http://www.mozilla.org");
>+     * var s = new C.u.Sandbox("http://www.mozilla.org");
>+     * var res = C.u.evalInSandbox("var f = 5; 4 + five", s);
>      * var outerFive = s.five;
>      * s.seven = 7;

You changed to use "f" instead of "five" in the example, but then the rest of
the
example doesn't make sense.  Also, you never show that res is 9, which is an
important
element of the interface -- one of the main reasons for this patch!

>-class nsXPCComponents_Constructor : public nsIXPCComponents_Constructor, public nsIXPCScriptable
>+class nsXPCComponents_Constructor : public nsIXPCComponents_Constructor,
>+                                    public nsIXPCScriptable

Don't need that change, and it bloats the diff and blame.

> /***************************************************************************/
>+// Javscript constructor for the sandbox object

sp: "JavaScript"

>+class nsXPCComponents_utils_Sandbox : public nsIXPCComponents_utils_Sandbox,
>+				      public nsIXPCScriptable

Underhanging in the second line.

>+NS_IMETHODIMP
>+nsXPCComponents_Utils::GetSandbox(nsIXPCComponents_utils_Sandbox **aSandbox)
>+{
>+    NS_ENSURE_ARG_POINTER(aSandbox);
>+    if (!mSandbox) {
>+        if (!(mSandbox = new nsXPCComponents_utils_Sandbox())) {
>+            *aSandbox = nsnull;
>+            return NS_ERROR_OUT_OF_MEMORY;
>+        }
>+    }

Use && here instead of a nested if?

>+JS_STATIC_DLL_CALLBACK(void)
>+sandbox_finalize(JSContext *cx, JSObject *obj)
>+{
>+    jschar *data = (jschar *)JS_GetPrivate(cx, obj);
>+    if (data) {
>+        delete[] data;
>+    }
>+}

Is there a reason we don't just use a JSString for the principal URI, and let
the GC do
its magic?  Define it as a property on the sandbox, or just hide it in a
private slot.

>+NS_IMETHODIMP
>+nsXPCComponents_utils_Sandbox::CallOrConstruct(
>+                                             nsIXPConnectWrappedNative *wrapper,

Better to overextend the 80-char boundary by 2 characters -- especially in this
file --
than to do this, IMO.

>+    if (!JSVAL_IS_STRING(argv[0]))
>+        return ThrowAndFail(NS_ERROR_XPC_BAD_ID_STRING, cx, _retval);

Should we convert to string here instead?  eval(4) works, seems like
C.u.evalInSandbox(4)
should as well.

>+    size_t length = JS_GetStringLength(principalURI);
>+    jschar *data = new jschar[length + 1];
>+    if (!data)
>+        return ThrowAndFail(NS_ERROR_OUT_OF_MEMORY, cx, _retval);
>+
>+    jschar *uri = JS_GetStringChars(principalURI);
>+    for (size_t i = 0; i < length; ++i)
>+        data[i] = uri[i];
>+
>+    data[length] = '\0';
>+    if (!JS_SetPrivate(cx, sandbox, data)) {
>+        delete[] data;
>+        return ThrowAndFail(NS_ERROR_XPC_UNEXPECTED, cx, _retval);
>+    }

We'd be able to lose all of this code as well, in exchange for a
JS_DefineProperty("__principalURI__").

>+    if (argc < 2) {
>+        JS_ReportError(cx, "Not enough arguments to evalInSandbox");
>+        return NS_ERROR_XPC_NOT_ENOUGH_ARGS;
>     }

Does this not double-report?  I think returning NS_ERROR_XPC_NOT_ENOUGH_ARGS
suffices without the JS_ReportError as well.  But why not ThrowAndFail here?

>+    // The second argument is the sandbox object. It is required.
>+    jsval *argv;
>+    rv = cc->GetArgvPtr(&argv);
>+    if (NS_FAILED(rv))
>+        return rv;
>+    if (JSVAL_IS_PRIMITIVE(argv[1]))
>+        return NS_ERROR_INVALID_ARG;
>+    JSObject *sandbox = JSVAL_TO_OBJECT(argv[1]);
>+    if (JS_GetClass(cx, sandbox) != &SandboxClass)
>+        return NS_ERROR_INVALID_ARG;

(And here, though I realize that this code was there before you got to it.)

>+    if (NS_SUCCEEDED(rv))
>         cc->SetReturnValueWasSet(PR_TRUE);
>-    }

Pike had some patches around this too, to make sure it threw exceptions
correctly.
I trust you've tested that codepath?

r=shaver with nits picked.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)

> We'd be able to lose all of this code as well, in exchange for a
> JS_DefineProperty("__principalURI__").

I'd prefer a reserved slot, or OBJ_SET_SLOT and OBJ_SET_SLOT on JSSLOT_PRIVATE
(can't store a JSString * private without a mark hook to mark it, since private
data is an int-tagged 0-mod-2-or-wider pointer hidden by that tagging from the
GC's mark phase).

New patch with shaver's fixes would be welcome!

/be

Comment 10

[Axel Hecht]

Looking at my patch in attachement 191355, this doesn't fix the exception stuff.

Another thing I saw, we still do the dance to get the principal from the string
for each and every evaluation, but now, the string is bound to the sandbox.
Couldn't we just bind the principal directly to the sandbox?

This might be good if we ever support the "no particular" special principal, too.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Ok, my next iteration of the patch will store the principal instead of the
principal URI. I didn't do it in this patch because I got scared that I would
end up holding onto a lock or something like that, but it looks like those fears
were unfounded.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated to comments

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 193716 [details] [diff] [review]
updated to comments

r=shaver, booyah.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193716 [details] [diff] [review]
updated to comments

Brendan, want to give this a second stamp (being in xpconnect and so close to
beta and all that)?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 193716 [details] [diff] [review]
updated to comments

>+interface nsIXPCComponents_utils_Sandbox : nsISupports

Underscores in typenames, what is the world coming to? ;-)

I'll live.

>+// We use the nsIXPScriptable macros to generate lots of stuff for us.
>+#define XPC_MAP_CLASSNAME           nsXPCComponents_utils_Sandbox
>+#define XPC_MAP_QUOTED_CLASSNAME   "nsXPCComponents_utils_Sandbox"

I guess this reads better (this => lining up nsXPC... and not lining up the
start of macro definitions).

>+NS_IMETHODIMP
>+nsXPCComponents_utils_Sandbox::CallOrConstruct(nsIXPConnectWrappedNative *wrapper,
>+                                               JSContext * cx, JSObject * obj,
>+                                               PRUint32 argc, jsval * argv,
>+                                               jsval * vp, PRBool *_retval)

Spaces after *s above bug me -- how 'bout you?

>+{
>+#ifdef XPCONNECT_STANDALONE
>+    return NS_ERROR_UNAVAILABLE;

NS_ERROR_NOT_AVAILABLE

Anyone testing XPCONNECT_STANDALONE?

>+    if(!stdUrl ||
>+       NS_FAILED((rv = stdUrl->Init(nsIStandardURL::URLTYPE_STANDARD, 80,
>+                                    codebase, nsnull, nsnull))) ||

The NS_FAILED actual argument is over-parenthesized once.

>+       !(iURL = do_QueryInterface(stdUrl, &rv))) {
>+        if (NS_SUCCEEDED(rv))
>+            rv = NS_ERROR_FAILURE;
>+        return ThrowAndFail(rv, cx, _retval);
>+    }
>+    
>+    nsIPrincipal *principal;
>+    nsCOMPtr<nsIScriptSecurityManager> secman =
>+        do_GetService(kScriptSecurityManagerContractID);
>+    if(!secman ||
>+       NS_FAILED(secman->GetCodebasePrincipal(iURL, &principal)) ||

Seems kind of arbitrary to capture rv from stdUrl->Init above, but replace the
rv of secman->GetCodebasePrincipal here with NS_ERROR_FAILURE.

>+       !principal) {
>+       return ThrowAndFail(NS_ERROR_FAILURE, cx, _retval);
>+    }
>+
>+    if (!JS_SetPrivate(cx, sandbox, principal)) {
>+        NS_RELEASE(principal);
>+        return ThrowAndFail(NS_ERROR_XPC_UNEXPECTED, cx, _retval);
>+    }
>+
>+    if (vp)
>+        *vp = OBJECT_TO_JSVAL(sandbox);
>+
>+    *_retval = JS_TRUE;
>+    return NS_OK;
>+#endif
>+}

r=me with some nits picked.

Who will document these APIs on devmo?	We must do that.  Need a bug to track?

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bring PAC up to date

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 193774 [details] [diff] [review]
Bring PAC up to date

r+a=shaver, thanks for finishing this up.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in on MOZILLA_1_8_BRANCH and trunk.

Comment 19

[Darin Fisher]

There are reports of PAC being broken.  Did this patch cause that regression? 
Have you confirmed that PAC still works with these changes?  Please see bug
306467.  Thanks.