296374 - move evalInSandbox to Components.utils.evalInSandbox

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

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

The evalInSandbox code may well be useful to chrome code as well, and the
global-namespace pollution of the current name never really appealed to me.  And
having Components.util will make it easier to add other handy utilities like
Factory or Module, whee.

Comment 1

[Giorgio Maone [:ma1]]

I've been looking for a JS accessible way to limit priviliges of evaluated JS
code a long time. Please do it ASAP :-)

Comment 2

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

dbradley: what do you think of my naming choice?

Comment 3

[David Bradley]

I think the name is fine and it's a good idea. We're already getting odds and
ends stuck in nsIXPComponents, such as lookupMethod and reportError. It's
probably not such a good idea to keep tacking things onto that interface and
providing util would help that stabalize that interface.

Comment 4

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

I want to move reportError and lookupMethod into Components.util as well, and I
have a patch to do this.  It leaves the rE and lM entry points in Components for
now, with an NS_WARNING and a forwarding to the util object.

lookupMethod isn't a piece of API we really ever wanted anyone else to use, as
per the comment in the IDL, so I don't expect that moving it will be
controversial.  (And in our new post-jst-XPCNativeWrapper world, we may not
really need it much at all.)  reportError was just added for 1.8b2, so I think
moving it with a warning in 1.8b3 and removing the Components.reportError entry
point for 1.8final should be fine.

Cc:ing bsmedberg and caillon, in case they want to disagree.

Comment 5

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

Attachment: Create Components.util, move evalInSandbox, reportError, lookupMethod to it

This also changes the evalInSandbox API to be more self-contained.  The patch
to nsProxyAutoConfig.js shows the new usage model.

I need to add some evalInSandbox doc to the IDL (not that there's any now!),
which I'll do in a follow-up patch with any review comments addressed.

Comment 6

[Darin Fisher]

Comment on attachment 186021 [details] [diff] [review]
Create Components.util, move evalInSandbox, reportError, lookupMethod to it

>Index: netwerk/base/src/nsProxyAutoConfig.js

>         // allocate a fresh Sandbox to clear global scope for new PAC script
>-        this._sandBox = new Sandbox();
>+        this._sandBox = Components.util.evalInSandbox(pacUtils, pacURI);
> 
>         // add predefined functions to pac
>-        var mypac = pacUtils + pacText;
>         this._sandBox.myIpAddress = myIpAddress;
>         this._sandBox.dnsResolve = dnsResolve;
>         this._sandBox.alert = proxyAlert;
> 
>         // evaluate loaded js file
>-        evalInSandbox(mypac, this._sandBox, pacURI);
>+        Components.util.evalInSandbox(mypac, pacURI, this._sandBox);

It looks like you've removed the definition of mypac, but it is
still being used.  Also, this new API seems really awkward to me.
The first call to evalInSandbox doesn't actually eval anything is
that right?  Only the second one that takes the third sandbox 
parameter actually evals something?

Comment 7

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

Yeah, I didn't rediff after fixing that mypac ref (should be pacText); biesi
caught it too.

Every call to evalInSandbox evaluates something.  If a sandbox is provided
through the 3rd parameter, it's reused, otherwise a new one is created.  So the
first call evaluates the pacUtils in a new sandbox and returns it.  After that
we want to use the same sandbox to eval pacText, so we pass it as the third
parameter.

I could expose something so that
   this._sandBox = new Components.util.Sandbox;
worked, but it'd be a fair bit of xpconnect glue-whacking, and it didn't really
seem worth it.

(I'm not quite sure, tbh, why we eval pacUtils instead of using functions like
with myIpAddress and alert, but I haven't looked closely at them.)

Comment 8

[Darin Fisher]

Ok, that makes sense now.  Thanks for the explanation.

Comment 9

[David Bradley]

Comment on attachment 186021 [details] [diff] [review]
Create Components.util, move evalInSandbox, reportError, lookupMethod to it

I'm not a super reviewer. I'm fine with the general idea of what's being done,
though.

Comment 10

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

Attachment: Add doc for evalInSandbox, fix mypac/pacText error, remove unused patch chunk

This is what I'd like to check in.  (The last one had a chunk of
exception-reporting fix in it which, while perhaps exciting, is not ready for
prime time and not appropriate for this bug.)

Moving benjamin's r+ ahead, and asking caillon to finish the job.  Tales of woe
or victory from people with "exciting" PAC configs are very welcome as well,
though my simple testing makes me think it's all good.

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 186324 [details] [diff] [review]
Add doc for evalInSandbox, fix mypac/pacText error, remove unused patch chunk

unable to test currently, but i'll do so at my earliest convenience.  patch
looks fine, though so r=caillon on that accord.  For bonus points, wanna get
rid of (or match them, your call) the tabs in the makefile?  I'm sure it looks
proper in an editor, but doesn't look as 31337 in diff view!

>Index: js/src/xpconnect/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/xpconnect/src/Makefile.in,v
>retrieving revision 1.80
>diff -u -p -r1.80 Makefile.in
>--- js/src/xpconnect/src/Makefile.in	20 May 2005 03:12:21 -0000	1.80
>+++ js/src/xpconnect/src/Makefile.in	15 Jun 2005 14:19:51 -0000
>@@ -63,6 +63,7 @@ REQUIRES	= xpcom \
> 		  string \
> 		  js \
> 		  caps \
>+                  necko \
> 		  dom \
> 		  $(NULL)
>

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

didn't there use to be a standalone xpconnect? (but maybe that's already broken,
I have no idea :-) )

Comment 13

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

Does this not build with XPCONNECT_STANDALONE?  I tried to keep the ifdefs
working correctly, though I didn't actually go so far as to test it.

I don't think it does anything that the old code didn't do in loader/, so I
think it's fine.  If there are bugs they should be easy to fix once someone
decides they need to care about it.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

sorry. I didn't notice the ifdefs. I need to wake up.

Comment 15

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

Comment on attachment 186324 [details] [diff] [review]
Add doc for evalInSandbox, fix mypac/pacText error, remove unused patch chunk

When testing this patch, please pretend that I closed the deprecation comment
for Components.lookupMethod.

Comment 16

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

I'm gonna land this in a bit (maybe tomorrow), it's working well for me here.

Comment 17

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

Comment on attachment 186324 [details] [diff] [review]
Add doc for evalInSandbox, fix mypac/pacText error, remove unused patch chunk

Requesting approval; we want this API shift in the alpha.

Comment 18

[Benjamin Smedberg]

Comment on attachment 186324 [details] [diff] [review]
Add doc for evalInSandbox, fix mypac/pacText error, remove unused patch chunk

js/src/xpconnect/src/Makefile.in needs tab-tab-space-space instead of all
spaces

Comment 19

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

Committed, with whitespace fix in xpconnect/src/Makefile.in.  Thanks all.

Comment 20

[David G King]

This checkin broke my Win32/MinGW/cygwin build

e:/mozilla/source/mozilla/js/src/xpconnect/src/xpccomponents.cpp: In member func
tion `virtual nsresult nsXPCComponents_Util::EvalInSandbox(const nsAString_inter
nal&, const nsACString_internal&)':
e:/mozilla/source/mozilla/js/src/xpconnect/src/xpccomponents.cpp:2200: error: in
valid conversion from `const PRUnichar*' to `const jschar*'
make[4]: *** [xpccomponents.o] Error 1
make[4]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/obj/mozilla/js/sr
c/xpconnect/src'

Comment 21

[jag (Peter Annema)]

Looks like you might be able to NS_REINTERPRET_CAST your way out of this, per lxr.

Comment 22

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

Bleh, crap, I always forget about that mingwism.  Can you see if
NS_REINTERPRET_CAST leaeds to victory, and I'll try to check in the fix tomorrow
if so?  I'm far too tired to be watching the tree ATM.

Comment 23

[David G King]

I tried to patch it, but my c is really rusty, and I'm not familiar with that
area of code. Got nowhere. Sorry.

Comment 24

[jag (Peter Annema)]

Attachment: (untested) This should fix it

Comment 25

[Piotr Komoda]

(In reply to comment #24)
> Created an attachment (id=187757) [edit]
> (untested) This should fix it

Checkin from this bug also broke Visual Studio 2005 build (in the same place)
and patch from this attachment seems to work for me. Thanks!

Comment 26

[David G King]

(In reply to comment #24)
> Created an attachment (id=187757) [edit]
> (untested) This should fix it
> 

Yup, fixes it for me as well. I almost had the patch right, but local knowledge
got it past my silly mistakes.

Comment 27

[David Bradley]

Comment on attachment 186021 [details] [diff] [review]
Create Components.util, move evalInSandbox, reportError, lookupMethod to it

Just removing request to get it off my radar

Comment 28

[John Bandhauer]

This fix should have included updating the "Components Object Reference" at: http://www.mozilla.org/scriptable/components_object.html

It's not too late now :)