1042996 - Remove custom Sandbox JSContext

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I found this patch in an old branch for bug 981218. I have yet to finish that bug, but we might as well get this into the tree for bug 951991.

Right now we create a custom JSContext for every call to EvalInSandbox. We can just get rid of it.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove the special JSContext for Sandboxes. v1

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=3ec01f780aa0

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Always set an explicit version in evalInSandbox. v1

If setVersion() is not invoked on compileOptions, it ends up with
JSVERSION_UNKNOWN, which invokes findVersion() on the JSContext, which does a
bunch of crazy hunting of previous scripted stack frames that we most certainly
don't want for sandboxes, which are supposed to be controlled environments.
Using a separate JSContext in evalInSandbox isolates us from these effects, so
once we stop doing that we need to be more explicit here.

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=1a6e77921e72

Comment 5

[Bob Owen (:bobowen)]

Comment on attachment 8461163 [details] [diff] [review]
Part 2 - Remove the special JSContext for Sandboxes. v1

Review of attachment 8461163 [details] [diff] [review]:
-----------------------------------------------------------------

So, as the SandboxPrivate isn't an nsIScriptGlobalObject, we just default to the SafeJSContext.
I was wondering if we could do something like that here, thanks.

Most of the remaining context pushing will need an AutoEntryScript I think.
So, I'm waiting for jimb to land his patch as he seems pretty close, otherwise I'd have to revisit everything again.

Looks good to me, apart from my comment and musings below.

::: js/xpconnect/src/Sandbox.cpp
@@ +1479,5 @@
>      RootedValue v(cx, UndefinedValue());
>      RootedValue exn(cx, UndefinedValue());
>      bool ok = true;
>      {
> +        mozilla::dom::AutoEntryScript entry(priv);

Although it's obvious, comment saying why we need this and, more importantly, that it's Gecko-specific (which I hope I'm right in saying :-) ).

I wonder if it would be worth adding another constructor to AutoEntryScript that just takes nsIGlobalObject*.
It might simplify things a bit, as the way the current constructor works we can only do this on main thread anyway.
I'll think about it when I add the Init function.

I also wonder if it would be worth choosing a standard variable name, like what tends to happen for JSAutoCompartment.
It's already got a couple of different ones.
If you have a preference, I could do the change along with the Init, as I'd have to hit all the code anyway.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #5)
> Most of the remaining context pushing will need an AutoEntryScript I think.
> So, I'm waiting for jimb to land his patch as he seems pretty close,
> otherwise I'd have to revisit everything again.

Ok. Jim, do you have a rough ETA? Given that this is a Q3 goal I don't want to let it sit for too long. ;-)

> Although it's obvious, comment saying why we need this and, more
> importantly, that it's Gecko-specific (which I hope I'm right in saying :-)
> ).

OK, I'll add the comment.

> I also wonder if it would be worth choosing a standard variable name, like
> what tends to happen for JSAutoCompartment.
> It's already got a couple of different ones.
> If you have a preference, I could do the change along with the Init, as I'd
> have to hit all the code anyway.

I would prefer aes. For some reason I didn't do that here - I'll fix that.

Comment 7

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #6)
> (In reply to Bob Owen (:bobowen) from comment #5)
> > Most of the remaining context pushing will need an AutoEntryScript I think.
> > So, I'm waiting for jimb to land his patch as he seems pretty close,
> > otherwise I'd have to revisit everything again.
> 
> Ok. Jim, do you have a rough ETA? Given that this is a Q3 goal I don't want
> to let it sit for too long. ;-)

Actually, I was thinking about this last night, I don't want to put extra pressure on Jim.
It looks like he has quite a few complicated things to juggle for that bug and we don't need it for functional reasons for bug 951991.

I'll start working on the patches with the current set-up.
If I get a chance to do the AutoEntryScript changes before, I will and switch the patches round.

If not, the follow-up changes will be pretty quick.

Comment 8

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ed20a6b9938d

Comment 9

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/60d9d4dffca4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fa252d7f676e

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60d9d4dffca4
https://hg.mozilla.org/mozilla-central/rev/fa252d7f676e