877673 - MakeObjectPropsNormal like API that matches jetpack requirements better

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

We have talked a lot about this. Here is a version that I think was the final conclusion. The other version would be to use the source compartments global as the |this| argument, which might be better, I'm not sure, but would require manual re-wrapping of the arguments. (which again might not be terrible, because that would be a place to do structural cloning for non-native object arguments...)

Anyway, this is a start, let me know what you think.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Attachment: export helpers

Comment 2

[Gabor Krizsanits (INACTIVE)]

Sorry about the windows line endings in the test file :(

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 755960 [details] [diff] [review]
export helpers

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

So, currently we have the unfortunate situation that there are two functions called "WrapCallable" in XPCComponents.cpp, differentiated by overloads. Their consumers and use cases are totally disjoint, and we should rename one of them to make things less confusing (especially since this patch relies on that stuff more now). Can you do that, either in this patch or a separate one?

Comments inline.

::: js/xpconnect/src/XPCComponents.cpp
@@ +2982,5 @@
> +WrapCallable(JSContext *cx, HandleObject scope,
> +             HandleObject callable, MutableHandleValue vp);
> +
> +// Instead of simply wrapping a function into another compartment,
> +// this helper function creates a native function on the target,

'in the', and nix the comma.

@@ +2984,5 @@
> +
> +// Instead of simply wrapping a function into another compartment,
> +// this helper function creates a native function on the target,
> +// compartment and propogates the call to the original function.
> +// Principal of the caller must subsume the target's.

The principal of the caller must subsume that of the callee.

@@ +2985,5 @@
> +// Instead of simply wrapping a function into another compartment,
> +// this helper function creates a native function on the target,
> +// compartment and propogates the call to the original function.
> +// Principal of the caller must subsume the target's.
> +static JSBool

This needs some inline documentation about the arguments that this function expects, given that the JSNative call signature doesn't really tell us anything about it.

@@ +3003,5 @@
> +
> +    RootedObject funObj(cx, &args[0].toObject());
> +    RootedObject targetScope(cx, &args[1].toObject());
> +
> +    // If there is an unsafe to unwrap security wrapper around the

The new terminology for 'unsafe to unwrap security wrapper' is just 'security wrapper'.

@@ +3004,5 @@
> +    RootedObject funObj(cx, &args[0].toObject());
> +    RootedObject targetScope(cx, &args[1].toObject());
> +
> +    // If there is an unsafe to unwrap security wrapper around the
> +    // targetScope the best if we just fail.

doesn't parse.

@@ +3007,5 @@
> +    // If there is an unsafe to unwrap security wrapper around the
> +    // targetScope the best if we just fail.
> +    targetScope = CheckedUnwrap(targetScope);
> +    if (!targetScope) {
> +        XPCThrower::Throw(NS_ERROR_FAILURE, cx);

Ugh, let's not add any more dependencies on XPCThrower. Just JS_ReportError, here and below please.

@@ +3011,5 @@
> +        XPCThrower::Throw(NS_ERROR_FAILURE, cx);
> +        return false;
> +    }
> +
> +    if (!JS_ObjectIsCallable(cx, funObj) || js::IsWrapper(funObj)) {

Hm, why the IsWrapper distinction? This likely to be confusing to any API consumer that can have functions in multiple same-origin scopes.

@@ +3044,5 @@
> +        XPCThrower::Throw(NS_ERROR_UNEXPECTED, cx);
> +        return false;
> +    }
> +
> +    JS_SET_RVAL(cx, vp, rval);

Use CallArgs for this.

@@ +3049,5 @@
> +    return true;
> +}
> +
> +// Helper function to get filename and line number from the current stack frame.
> +nsresult GetSourceInfoFromStack(nsXPIDLCString &filename, int32_t &lineNo)

Can we just use JS_DescribeScriptedCaller() for this? I don't really want to add new consumers to the custom XPC stack munging stuff.

@@ +3067,5 @@
> +
> +// Similar to evalInSandbox except this one is used to eval a script in the
> +// scope of a window. Principal of the caller must subsume the target's.
> +static JSBool
> +EvalInWindow(JSContext *cx, unsigned argc, jsval *vp)

Same two comments for ExportFunction apply here.

@@ +3076,5 @@
> +        return false;
> +    }
> +
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (!args[0].isString() || args[1].isPrimitive()) {

Just check !args[1].isObject()

@@ +3086,5 @@
> +    RootedObject targetScope(cx, &args[1].toObject());
> +    size_t srcLen;
> +    const jschar *src = JS_GetStringCharsAndLength(cx, srcString, &srcLen);
> +
> +    // If we cannot unwrap we mustn't eval in it.

"mustn't" is kind of archaic - just say "must not".

@@ +3093,5 @@
> +        XPCThrower::Throw(NS_ERROR_FAILURE, cx);
> +        return false;
> +    }
> +
> +    // Make sure that we have a window object. And ref. the

addref, without the two period

@@ +3100,5 @@
> +    nsCOMPtr<nsIGlobalObject> global;
> +    nsCOMPtr<nsPIDOMWindow> window;
> +    if (!JS_IsGlobalObject(inner) ||
> +        !(global = GetNativeForGlobal(inner)) ||
> +        !(window = do_QueryInterface(global))) {

Brace goes on the new line.

@@ +3128,5 @@
> +        if (!JS::Evaluate(cx, targetScope, options,
> +                          src, srcLen, rval.address())) {
> +            return false;
> +        }
> +    }

I think this is probably going to have to go through nsIScriptContext::EvaluateString, if only because we probably still need the CanExecuteScripts check. :-(

@@ +4461,5 @@
>      return true;
>  }
>  
> +JSBool
> +WrapCallable(JSContext *cx, HandleObject scope, HandleObject callable, MutableHandleValue vp)

Hm, so this is different from the existing WrapCallable because it doesn't take an id, right? Why doesn't it?

@@ +5194,5 @@
>    }
>    *parentObj = mScope->GetGlobalJSObject();
>    return NS_OK;
>  }
> +

At the end of the file here - is this whitespace removal or addition?

::: js/xpconnect/tests/chrome/test_evalInWindow.xul
@@ +2,5 @@
> +<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
> +<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css"
> +                 type="text/css"?>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=XXXXX

Need to fix the XXXXXXes.

@@ +12,5 @@
> +
> +  <!-- test results are displayed in the html:body -->
> +  <body xmlns="http://www.w3.org/1999/xhtml">
> +
> +  <iframe src="http://example.org/tests/js/xpconnect/tests/mochitest/file_evalInSandbox.html"

I don't want to depend on file_foo in tests other than test_foo. Can you use file_empty here?

@@ +20,5 @@
> +          onload="crossOrig(this)">
> +  </iframe>
> +  <iframe src="chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/file_evalInSandbox.html"
> +          onload="crossOrig(this)">
> +  </iframe>

These iframes need to go after the <script> tag, otherwise the onload handlers might fail to compile.

@@ +29,5 @@
> +      SimpleTest.waitForExplicitFinish();
> +      const Cu = Components.utils;
> +      var sb = new Cu.Sandbox("http://example.org", {wantExportHelpers: true});
> +
> +      function executeIn(frame) {    

trailing whitespace, here an elsewhere in the file.

@@ +33,5 @@
> +      function executeIn(frame) {    
> +        return function(script) {sb.frame = frame; return Cu.evalInSandbox("evalInWindow('" + script + "',frame)", sb)};
> +      }
> +
> +      function sameOrig(frame)

Make these function names more descriptive. testSameOrigin and testCrossOrigin?

@@ +37,5 @@
> +      function sameOrig(frame)
> +      {
> +        is(frame.contentWindow.document.foo, undefined, 'content expando should not be visible through xray');
> +      
> +        var executeInSameOrig = executeIn(frame.contentWindow);

This setup seems awkard to me. Can we just do executeIn(frame, script) instead of returning an intermediate closure?

::: js/xpconnect/tests/unit/test_exportFunction.js
@@ +1,3 @@
> +function run_test() {
> +  var Cu = Components.utils;
> +  var epsb = new Cu.Sandbox(["http://blah.com", "http://fu.com"], {wantExportHelpers: true});

try to use example.foo domains, for consistency and per RFC 2606.

@@ +9,5 @@
> +  epsb.do_check_true = do_check_true;
> +  epsb.do_check_eq = do_check_eq;
> +
> +// exporting should work if prinicipal of the source sandbox
> +// subsumes the principal of the target sandbox

Indendation.

@@ +12,5 @@
> +// exporting should work if prinicipal of the source sandbox
> +// subsumes the principal of the target sandbox
> +  Cu.evalInSandbox("(" + function(){
> +    var wasCalled = false;
> +    var funToExport = function(a, thisarg){

) {

@@ +21,5 @@
> +    this.checkIfCalled = function(){
> +      do_check_true(wasCalled);
> +      wasCalled = false;
> +    }
> +    subsb.imported = exportFunction(funToExport, this.subsb);

inconsistent |this|-qualification for subsp.

@@ +25,5 @@
> +    subsb.imported = exportFunction(funToExport, this.subsb);
> +  }.toSource() + ")()", epsb);
> +
> +// exported function should be able to be call from the
> +// target sandbox

"be called". Also, make comments complete sentences if possible (capitalize and period).

@@ +33,5 @@
> +
> +// apply should work but the |this| argument should not be
> +// possible to be changed
> +  Cu.evalInSandbox("(" + function(){
> +    imported.apply("something", [42, "something"]);

Hm, I don't grok this. This test seems to be checking that the caller of the imported function controls the |this|-binding. But this seems questionably safe, and contrary to what the comment suggests.

@@ +41,5 @@
> +    checkIfCalled();
> +  }.toSource() + ")()", epsb);
> +
> +// exporting should throw if princpal of the source sandbox does
> +// not subsume the principal of the target

Indendation.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Uhh... Thanks a lot for the through review this patch meant to be only a base for a high level discussion, but this is some great help.

I start with this since this is the main question for me:

> > +
> > +// apply should work but the |this| argument should not be
> > +// possible to be changed
> > +  Cu.evalInSandbox("(" + function(){
> > +    imported.apply("something", [42, "something"]);
> 
> Hm, I don't grok this. This test seems to be checking that the caller of the
> imported function controls the |this|-binding. But this seems questionably
> safe, and contrary to what the comment suggests.

In the first version I always used the source global in the call as |this| (and then left the comment unchanged here so it's now totally confusing and wrong...). The second (current) version I simply let content alter the |this|. This is a more natural version (no tricks in the native layer), but in a way less secure. But in the end I think the best thing is just to avoid using the |this| in an exported function explicitly... So I ended up with this version. In general, are you OK with the current approach?


(In reply to Bobby Holley (:bholley) from comment #3)
> So, currently we have the unfortunate situation that there are two functions
> called "WrapCallable" in XPCComponents.cpp

Yeah, that's quite bad I will change it.

> 
> Comments inline.
> 

Hmm... Where can I learn about all these things? The let's check how other people do stuff in the same file approach clearly failing me all the time...

> > +// Principal of the caller must subsume the target's.
> 
> The principal of the caller must subsume that of the callee.

Here both the caller and the callee are from the same compartment mostly, and the target scope that we pass in as an argument is the only reference from the other compartment. So it's now "... that of the target."

> The new terminology for 'unsafe to unwrap security wrapper' is just
> 'security wrapper'.

Just wanted to make clear the connection between the CheckedUnwrap and security wrappers, but it's probably unnecessary indeed...

> > +
> > +    // If there is an unsafe to unwrap security wrapper around the
> > +    // targetScope the best if we just fail.
> 
> doesn't parse.

how about?
// We can only export functions to scopes those are transparent for us,
// hence if there is a security wrapper around targetScope we must throw.

> Ugh, let's not add any more dependencies on XPCThrower. Just JS_ReportError,
> here and below please.

Thanks, I was hoping that we have something that can throw meaningful errors.

> Hm, why the IsWrapper distinction? This likely to be confusing to any API
> consumer that can have functions in multiple same-origin scopes.

Yeah, in fact, it does not even have to be same-origin. I see no reason not to let to export any function we have a reference to in our sandbox. Do you? I mean... shall I do a subsume check here or I can just go ahead and do an unsafe unwrap? We cannot get a hold of a function that we cannot call right? Maybe I should check if I'm allowed to call the wrapped function object...
 
> I think this is probably going to have to go through
> nsIScriptContext::EvaluateString, if only because we probably still need the
> CanExecuteScripts check. :-(
>

I was kind of expecting something like this... had no idea which is the proper eval to call... alright, I'll do that
 
> 
> Hm, so this is different from the existing WrapCallable because it doesn't
> take an id, right? Why doesn't it?
> 

In the old setup it made sense to use an id. But what id would you use for an anonymous function for example? Also I'm not sure I want to specify any id in the target scope, don't know if it has any side effect... But this is a very good question, I'm not very good with JS API in general, I think this is the right thing to do but if you find my arguments invalid, I'm all ears.

> 
> I don't want to depend on file_foo in tests other than test_foo. Can you use
> file_empty here?
> 

Yeah, I guess I can define that property on it via .wrappedJSObject.

> RFC 2606.

Thanks, never heard of it.

Again, sorry for causing you this much work I was not expecting you to do a full review on it. In the future, I will file a bit more polished patches if I want to get some feedback from you for sure :)

Comment 5

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> Uhh... Thanks a lot for the through review this patch meant to be only a
> base for a high level discussion, but this is some great help.

Ah, sorry about that. I should have assumed so, given the f? - I guess just mention explicitly what kind of feedback you're looking for, but either way it's no big deal. :-)

> In the first version I always used the source global in the call as |this|
> (and then left the comment unchanged here so it's now totally confusing and
> wrong...). The second (current) version I simply let content alter the
> |this|. This is a more natural version (no tricks in the native layer), but
> in a way less secure. But in the end I think the best thing is just to avoid
> using the |this| in an exported function explicitly... So I ended up with
> this version. In general, are you OK with the current approach?

I don't think we should allow content to munge the |this| object. These things are designed to be functions, not methods.

In fact, I don't think we should allow content to pass objects at all. In an ideal world, this would be enforced by opaque wrappers in chrome to unwaived content objects. But since we're pretty far from that at the moment, it seems like we should actually iterate over the arguments and check !isObject. This will give us more flexibility to do opaque wrappers later on.


> Hmm... Where can I learn about all these things? The let's check how other
> people do stuff in the same file approach clearly failing me all the time...

Which things, exactly? A lot of it is just grammar/idiom correction, which is totally understandable.

> 
> > > +// Principal of the caller must subsume the target's.
> > 
> > The principal of the caller must subsume that of the callee.
> 
> Here both the caller and the callee are from the same compartment mostly,
> and the target scope that we pass in as an argument is the only reference
> from the other compartment. So it's now "... that of the target."

Ag, I see.


> > The new terminology for 'unsafe to unwrap security wrapper' is just
> > 'security wrapper'.
> 
> Just wanted to make clear the connection between the CheckedUnwrap and
> security wrappers, but it's probably unnecessary indeed...

If so, I'd say something like "security wrapper (i.e. a wrapper that is not safe to unwrap)" or something.

> 
> > > +
> > > +    // If there is an unsafe to unwrap security wrapper around the
> > > +    // targetScope the best if we just fail.
> > 
> > doesn't parse.
> 
> how about?
> // We can only export functions to scopes those are transparent for us,

"scopes that are transparent to us"

> // hence if there is a security wrapper around targetScope we must throw.

I'd just say 'so' instead of 'hence'.

> > Hm, why the IsWrapper distinction? This likely to be confusing to any API
> > consumer that can have functions in multiple same-origin scopes.
> 
> Yeah, in fact, it does not even have to be same-origin. I see no reason not
> to let to export any function we have a reference to in our sandbox. Do you?
> I mean... shall I do a subsume check here or I can just go ahead and do an
> unsafe unwrap? We cannot get a hold of a function that we cannot call right?
> Maybe I should check if I'm allowed to call the wrapped function object...

We should still probably CheckedUnwrap, if only because we want to keep the instances of UncheckedUnwrap low in our codebase. Anyone who wants to export a security-wrapped function can just wrap it in their own function. This also gives us more flexibility if we ever want to make certain security-wrapped functions non-callable.

> > Hm, so this is different from the existing WrapCallable because it doesn't
> > take an id, right? Why doesn't it?
> > 
> 
> In the old setup it made sense to use an id. But what id would you use for
> an anonymous function for example? Also I'm not sure I want to specify any
> id in the target scope, don't know if it has any side effect... But this is
> a very good question, I'm not very good with JS API in general, I think this
> is the right thing to do but if you find my arguments invalid, I'm all ears.

The main use of id is to make fn.name work correctly, I think. We can just pass the name of the wrapped function. And add tests to make sure.

> Again, sorry for causing you this much work I was not expecting you to do a
> full review on it. In the future, I will file a bit more polished patches if
> I want to get some feedback from you for sure :)

No problem! I owe you some reviews at this point anyway. :-)

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #5)
> mention explicitly what kind of feedback you're looking for, but either way
> it's no big deal. :-)

Indeed, I read back the bug description and I could have been a lot more explicit.

> 
> I don't think we should allow content to munge the |this| object. These
> things are designed to be functions, not methods.

That's what I thought as well first. Since munging with the |this| object has only effect on the explicit usage of the |this| keyword, this is currently my least concern. But let's assume we do that for now.

> 
> In fact, I don't think we should allow content to pass objects at all. In an
> ideal world, this would be enforced by opaque wrappers in chrome to unwaived
> content objects. But since we're pretty far from that at the moment, it
> seems like we should actually iterate over the arguments and check
> !isObject. This will give us more flexibility to do opaque wrappers later on.
> 

OK so this is the real question. Let's play with this thought for a second. So first of all I would let through xrayable object. And then there are function objects that would be great to be able to call at least... Also, a function that can do a structure clone from an opaque object were important as well in the future.

Problem is that the other part of the API, the evalInWindow can return an object from the other scope. The real job of that function actually to make it possible to define a property on the content window (or on the other scope in general). So maybe I could just swap that function with one that can only 'safely' define a property on an object form another scope (most importantly on a window) and nothing more. But don't forget, that this whole exportFunction in the end returns a function object of the target scope to the source scope even if we block the arguments and the |this| pointer. So there is no point blocking object arguments temporarily until we have these opaque wrappers, since that would only guarantee the illusion of safety.

By the way, the reason why I'm pushing this is not only because I want to use nsEP for content-script always, but recently Irakli is pushing very hard some structural changes in the SDK module system. Where the main.js instead of being run in a chrome sandbox, would be running in a frameless window with nsEP (so it would be index.html). That addon window scope would have require, and could load in chrome functionality through modules, which would mean, this whole API would be a lot more important. (content-exposing something to content is a rare case, and also very simple cases, addons using modules though...). This change stresses me out to be honest, but so far I failed to convince him that this is a bad idea... And indeed there are a lot of advantages of this design, but I'm not too optimistic about this part.

> 
> Which things, exactly? A lot of it is just grammar/idiom correction, which
> is totally understandable.
> 

Usually coding style, like the inline commenting for functions... and which API should I use from the many for a certain thing :) but I guess it's more of a rhetorical question...

Comment 7

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> OK so this is the real question. Let's play with this thought for a second.
> So first of all I would let through xrayable object. And then there are
> function objects that would be great to be able to call at least... Also, a
> function that can do a structure clone from an opaque object were important
> as well in the future.
> 
> Problem is that the other part of the API, the evalInWindow can return an
> object from the other scope. The real job of that function actually to make
> it possible to define a property on the content window (or on the other
> scope in general). So maybe I could just swap that function with one that
> can only 'safely' define a property on an object form another scope (most
> importantly on a window) and nothing more. But don't forget, that this whole
> exportFunction in the end returns a function object of the target scope to
> the source scope even if we block the arguments and the |this| pointer. So
> there is no point blocking object arguments temporarily until we have these
> opaque wrappers, since that would only guarantee the illusion of safety.

Well, I'm more concerned with content being able to pass objects to chrome that chrome, which is why I'm suggesting that we dynamically filter the arguments that get passed. But yeah, in general this is a problem and is only going to get worse if jetpack starts depending on it. I'm going to do some investigation and see how much stuff if we start making unwaived content objects opaque (or some approximation thereof).


> 
> By the way, the reason why I'm pushing this is not only because I want to
> use nsEP for content-script always, but recently Irakli is pushing very hard
> some structural changes in the SDK module system. Where the main.js instead
> of being run in a chrome sandbox, would be running in a frameless window
> with nsEP (so it would be index.html).

Why a window and not a sandbox?

> That addon window scope would have
> require, and could load in chrome functionality through modules, which would
> mean, this whole API would be a lot more important. (content-exposing
> something to content is a rare case, and also very simple cases, addons
> using modules though...).

So the nsEP scope would have unrestricted access to lots of chrome functionality? That seems bad. But I might not fully understand the situation. Let's talk about this on IRC sometime?


> 
> > 
> > Which things, exactly? A lot of it is just grammar/idiom correction, which
> > is totally understandable.
> > 
> 
> Usually coding style, like the inline commenting for functions... and which
> API should I use from the many for a certain thing :) but I guess it's more
> of a rhetorical question...

Oh, sorry. When I said "Comments inline" I just meant "my review comments are below". It wasn't a review comment itself. :-)

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #7)
> 
> Well, I'm more concerned with content being able to pass objects to chrome
> that chrome, which is why I'm suggesting that we dynamically filter the
> arguments that get passed. But yeah, in general this is a problem and is
> only going to get worse if jetpack starts depending on it. I'm going to do
> some investigation and see how much stuff if we start making unwaived
> content objects opaque (or some approximation thereof).

Awesome, thanks. You can count on me on the getting it done part.

> Why a window and not a sandbox?

So the basic idea is to expose DOM API in a natural way, and also make addon development as natural for web developers as possible. But then it gets dirty because this addon window should be a XUL window as well, because we rely on SwapFrameLoaders for panels. So I think it's better to ask Irakli because honestly I have too much concerns and questions about this whole plan right now
to explain it the why part. But for sure the exposing DOM API should be the main reason for it, without windowless workarounds like I did for indexeddb and xhr.

> So the nsEP scope would have unrestricted access to lots of chrome
> functionality? That seems bad. But I might not fully understand the
> situation. Let's talk about this on IRC sometime?

Well, not really unrestricted, since it needs to explicitly require modules, that exports only a very well defined set of functionality, and these modules are part of the SDK and the add-on is reviewed as well. But still... yes, it does sound quite scary to me. This is exactly what I was trying to avoid the whole time, and I was so far pretty happy with the SDK structure ( that is: chrome modules, nsEP content-scripts, only message based communication between the two, and rarely exporting some stuff to content...)

I will have a talk with Irakli about this at 10am your time today, feel free to join. But I will be also available after the meeting, hopefully with a better understanding about his plans, and we can talk on IRC too. And he is working from the SF office, so at some point it might be a good idea for you to grab him for a talk if you are there too.

> Oh, sorry. When I said "Comments inline" I just meant "my review comments
> are below". It wasn't a review comment itself. :-)

Hah! Alright then :) I misinterpreted it completely wrong then :)

Comment 9

[Gabor Krizsanits (INACTIVE)]

Yay. So The good thing is that this whole idea of moving the main add-on script into a window with nsEP is more like a very long term future dream, and based on what I told them about the difficulties they might have to face, kind of turned them off... So let's not care about this part now. For now the DOM API stuff will be handled differently, but that part seems to be an off-topic in this bug we'll discuss that on IRC or in some other bug. (add-on will still live in system sandboxes, and it will have access to some windows from lower privileged frame less windows, or something in that direction...)

Comment 10

[Bobby Holley (:bholley)]

Actually, I think we should instead just do a structured clone for all object arguments. What do you think?

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #10)
> Actually, I think we should instead just do a structured clone for all
> object arguments. What do you think?

Pure JS object arguments you mean? Yes, I agree. I would leave xrayables. There is still one thing I don't know how to achieve, setting safely a property on the window object with this exported function as the value. I think exportFunction should do that optionally in c++ and use the evalInWindow only to let chrome/nsEP access content APIs (like the gmail API), as a replacement for the unsafeWindow/wrappedJSObject that is used now for this.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)

> Pure JS object arguments you mean? Yes, I agree. I would leave xrayables.

Definitely.

> There is still one thing I don't know how to achieve, setting safely a
> property on the window object with this exported function as the value. I
> think exportFunction should do that optionally in c++ and use the
> evalInWindow only to let chrome/nsEP access content APIs (like the gmail
> API), as a replacement for the unsafeWindow/wrappedJSObject that is used now
> for this.

Yeah, that sounds good. We still need to think about what the return values for evalInWindow look like.

Comment 13

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #12)
> Yeah, that sounds good. We still need to think about what the return values
> for evalInWindow look like.

I think the same as for the arguments (structured clone/xray). That's the reason I need that extra functionality (set prop.) for exportFunction. It will take a bit of a juggling to do everything with this limited API, but it should be an exceptional case to need all this anyway...

What seems to be a bigger problem than I expected is that we need a modified version of the structured cloning algorithm, that leaves xrayables alone... Do you know who knows more about structured cloning?

Comment 14

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> What seems to be a bigger problem than I expected is that we need a modified
> version of the structured cloning algorithm, that leaves xrayables alone...

I have a feeling that this would take way too much of work, and probably the js team would not be too thrilled by the idea... So I might just make the exception case for xrayable if the argument itself is an xrayable, and will not care about the case of one of the properties of an argument is native for now, just let the structured cloning algorithm we have do what it does.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> > What seems to be a bigger problem than I expected is that we need a modified
> > version of the structured cloning algorithm, that leaves xrayables alone...
> 
> I have a feeling that this would take way too much of work, and probably the
> js team would not be too thrilled by the idea... So I might just make the
> exception case for xrayable if the argument itself is an xrayable, and will
> not care about the case of one of the properties of an argument is native
> for now, just let the structured cloning algorithm we have do what it does.

It should actually be very straightforward - the structured clone algorithm punts to a callback if it hits a JSObject with a JSClass it doesn't recognize. So we can just set up the callbacks to just call JS_Wrap for DOM objects.

Comment 16

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #15)
> It should actually be very straightforward - the structured clone algorithm
> punts to a callback if it hits a JSObject with a JSClass it doesn't
> recognize. So we can just set up the callbacks to just call JS_Wrap for DOM
> objects.

Awesome, thanks :)

Comment 17

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #15)
> It should actually be very straightforward - the structured clone algorithm
> punts to a callback if it hits a JSObject with a JSClass it doesn't
> recognize. So we can just set up the callbacks to just call JS_Wrap for DOM
> objects.

Problem is that from what I see here, is that there are two callbacks, read and write. And in the write callback I must serialize an object into a buffer and give it a tag, and in the read callback I must create a new object from this data stream. For easily serializable and then re-creatable objects this works, but for wrapped natives? I could probably use this buffer to transfer some raw pointers, but that seems to be incredibly whacky. Or I could add some hacks to the algorithm, that maintains an array of objects and then the serialized data would be the index in that array. But this hack would totally go against the base assumption of this whole cloning algorithm implementation, that everything that goes through is serialized, and I'm afraid Jason Orendorff would take my head for it. Is there some obvious solution here that I just fail to see?

Comment 18

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17)
> (In reply to Bobby Holley (:bholley) from comment #15)
> > It should actually be very straightforward - the structured clone algorithm
> > punts to a callback if it hits a JSObject with a JSClass it doesn't
> > recognize. So we can just set up the callbacks to just call JS_Wrap for DOM
> > objects.
> 
> Problem is that from what I see here, is that there are two callbacks, read
> and write. And in the write callback I must serialize an object into a
> buffer and give it a tag, and in the read callback I must create a new
> object from this data stream. For easily serializable and then re-creatable
> objects this works, but for wrapped natives? I could probably use this
> buffer to transfer some raw pointers, but that seems to be incredibly
> whacky. Or I could add some hacks to the algorithm, that maintains an array
> of objects and then the serialized data would be the index in that array.
> But this hack would totally go against the base assumption of this whole
> cloning algorithm implementation, that everything that goes through is
> serialized, and I'm afraid Jason Orendorff would take my head for it. Is
> there some obvious solution here that I just fail to see?

Have a look at the way we do structured clones of Blobs, Files, etc when postMessage-ing to workers. I'm pretty sure we just serialize the pointer, which should be fine here too.

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #18)
> Have a look at the way we do structured clones of Blobs, Files, etc when
> postMessage-ing to workers. I'm pretty sure we just serialize the pointer,
> which should be fine here too.

Indeed we only serialize the pointers there. Alright, I'll just do the same then.

Comment 20

[Gabor Krizsanits (INACTIVE)]

> +        XPCThrower::Throw(NS_ERROR_FAILURE, cx);

Last thing I'm struggling with is JS_ReportError. Simply put it does not work. I return false and call JS_ReportError instead of XPCThrower::Throw, and it even sets the pending exception on the context, but the exception is not detectable at js level. Any other API to throw exception? By the way, while I was struggling with this, I came across this https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp#3945 Is there a reason that in xpc_EvalInSandbox after we set the exception on the caller cx we return NS_OK? This seems like a bug to me.

Comment 21

[Gabor Krizsanits (INACTIVE)]

In the xpcshell test it works, so this is only an issue in a setup where I call exportFunction from a sandbox that is in a xul window (chrome test)... weird.

Comment 22

[Gabor Krizsanits (INACTIVE)]

Ok, so the problem seems to be in my code, and I think I know what is going on, but I still wonder if that NS_OK is a bug in xpc_EvalInSandbox if there were an exception in the script.

Comment 23

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22)
> Ok, so the problem seems to be in my code, and I think I know what is going
> on, but I still wonder if that NS_OK is a bug in xpc_EvalInSandbox if there
> were an exception in the script.

Yeah, that seems like a bug. Want to file and see if anything breaks on try?

Comment 24

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #23)
> Yeah, that seems like a bug. Want to file and see if anything breaks on try?

Sure.

Comment 25

[Gabor Krizsanits (INACTIVE)]

Attachment: export helpers v2

A few things about this version. I'm not very confident with the naming of the transmitter functions... if it hurts the native English speaker ears, feel free to suggest something better. Also NewMethodCallTransmitter and NewFunctionCallTransmitter does not differ much in the end, so I might want to merge them together and add a function pointer argument to it. In the EvaInWindow function, after the eval string call, transferring the exception from the window context to the caller context bits does not do much, since in the test for some reason, exception thrown in the script, does not end up anywhere... Probably, if we stick to this version I will have to think about some more testing too. Finally, I flagged this patch as feedback, but in my mind this version might be actually close to the final version. It changed quite a lot since the last version, so an interdiff might not be very useful, but if you think it would help, or if you'd prefer to be split up in more patches, let me know.

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 765334 [details] [diff] [review]
export helpers v2

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +2973,1 @@
>  

XPCComponents is getting pretty big. I think at some point we should split out the sandbox stuff. Doesn't have to be now, though.

@@ +2979,5 @@
>  }
>  
> +JSBool
> +NewFunctionCallTransmitter(JSContext *cx, HandleObject scope, HandleId id, HandleObject callable,
> +                           MutableHandleValue vp);

Let's call these FunctionForwarders

@@ +2991,5 @@
> +// the exported function, and all the non-native JS object arguments
> +// will be structure cloned.
> +// The wrapped new transmitter function created will be the return
> +// value. The 3rd argument is the name of the property that will
> +// be set on the target scope, with the transmitter function as 

Whitespace

@@ +2995,5 @@
> +// be set on the target scope, with the transmitter function as 
> +// the value.
> +// Principal of the caller must subsume that of the target.
> +//
> +/* jsval ExportFunction( in jsval funToExport, in jsval targetScope, in string name) */

Given that this isn't exposed through XPIDL, I think describing with XPIDL is a bit confusing.

@@ +3036,5 @@
> +
> +    size_t nameLength;
> +    const jschar* nameChars = JS_GetStringCharsAndLength(cx, funName, &nameLength);
> +    if (!nameLength) {
> +        JS_ReportError(cx, "3rd argument");

Some of these strings passed to JS_ReportError could use some improvement.

@@ +3095,5 @@
> +}
> +
> +enum XPCCloneTags {
> +    SCTAG_BASE = JS_SCTAG_USER_MIN,
> +    SCTAG_XPC_WRAPPEDNATIVE

Well, this also applies to DOM objects, which are not WNs. How about just SCTAG_XRAYABLE? I don't think there's a need to prefix it, given that this enum isn't exported in a header or anything.

@@ +3101,5 @@
> +
> +static void
> +CloneOrXrayError(JSContext *cx, uint32_t /* aErrorId */)
> +{
> +    JS_ReportError(cx, "XPC clone error.");

This functions seems dumb. Am I correct in my reading of the code that, without this callback, the structured clone functions will return false without setting an exception?

If so, that's bad JSAPI design. But if that's what we have to work with, then I'd prefer to just JS_ReportError directly when the call fails, rather than using this callback. And I'd prefer to inline it, with custom strings, rather than just invoking it at the end of CloneOrXrayRead.

@@ +3119,5 @@
> +            MOZ_ASSERT(WrapperFactory::IsXrayable(xrayable), "Object pointer must be a native!");
> +            // Just in case: another rooted for the wrapper object:
> +            RootedObject xrayed(cx, xrayable);
> +            JS_WrapObject(cx, xrayed.address());
> +            return xrayed;

Can we assert here that we actually end up with a reflector or XrayWrapper?

@@ +3139,5 @@
> +    // pointers, and serialize those pointers instead. Given the complexity of creating
> +    // a JSObject in another compartment for a given XPCWrappedNative, we serialize the
> +    // JSObject pointers instead and simply call JS_WrapObject in the reader.
> +    AutoObjectVector *xrayables = static_cast<AutoObjectVector *>(closure);
> +    if (WrapperFactory::IsXrayable(obj)) {

As the code in WrapperFactory stands, this will bork for properties that are _themselves_ XrayWrappers (i.e. XOWs). Please add some tests for that.

@@ +3145,5 @@
> +        xrayables->append(obj);
> +        // Now that the object is rooted its pointer can be serialized
> +        JSObject *rawptr = obj;
> +        if (JS_WriteUint32Pair(writer, SCTAG_XPC_WRAPPEDNATIVE, 0) &&
> +            JS_WriteBytes(writer, &rawptr, sizeof(rawptr))) {

This is totally unsafe with a moving GC. How about storing indices into the vector, instead?

@@ +3165,5 @@
> +// This is a special structured cloning, that only wraps xrayable objects
> +// and clones everything else.
> +//
> +JSBool
> +CloneOrXray(JSContext *cx, MutableHandleValue val)

I'm not really comfortable with framing this around Xrays, because there are cases in which we have a reflector but no Xrays (like XPCOM objects for which we just make one reflector per scope). In general, such things shouldn't be showing up in content, but that's not a strong enough invariant to depend on these things coming up as Xray.

So let's call this "CloneNonReflectors" and replace references to "Xrayables" with "Reflectors".

@@ +3203,5 @@
> +    for (unsigned i = 0; i < args.length(); i++) {
> +        if (!CloneOrXray(cx, args.handleAt(i))) {
> +            return false;
> +        }
> +    }

Hm, what about |this|?

@@ +3289,5 @@
> +    // on the calling context.
> +    RootedValue exn(cx, UndefinedValue());
> +    if (JS_GetPendingException(wndCx, exn.address())) {
> +        JS_ClearPendingException(wndCx);
> +        JS_SetPendingException(cx, exn);

Hm, are these two cxes guaranteed to be same-compartment here?

@@ +4174,5 @@
>  }
>  
>  /***************************************************************************/
>  
> +/* void 

Whitespace.

@@ +4175,5 @@
>  
>  /***************************************************************************/
>  
> +/* void 
> +andbox(in AString source, in nativeobj sandbox); */

Er, what?

@@ +4603,5 @@
>      return NS_OK;
>  }
>  
>  JSBool
> +MethodCallTransmitter(JSContext *cx, unsigned argc, Value *vp)

I don't think function vs method is really a meaningful distinction here. As far as I can tell, the main distinction is whether or not we do the clone, right? Can probably just templatize that on DoClone or something.

@@ +4630,5 @@
> +    RootedValue v(cx, js::GetFunctionNativeReserved(&args.callee(), 0));
> +    NS_ASSERTION(v.isObject(), "weird function");
> +    RootedObject origFun(cx, &v.toObject());
> +    {
> +        JSAutoCompartment ac(cx, origFun);

This is a GC hazard. the FunctionNativeReserved _must_ be wrapped into the compartment of its function object, otherwise you have cross-compartment references living on the heap.

::: js/xpconnect/wrappers/WrapperFactory.h
@@ +82,5 @@
>      static bool XrayWrapperNotShadowing(JSObject *wrapper, jsid id);
> +
> +    // Returns true if the js objects is a holder for a native object
> +    // (can be wrapped into an Xray wrapper).
> +    static bool IsXrayable(JSObject *obj);

Let's just move GetXrayType from XrayWrapper.h to here and expose that.

Comment 27

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #26)
> XPCComponents is getting pretty big. I think at some point we should split
> out the sandbox stuff. Doesn't have to be now, though.

Agree. Bug 886237

> 
> Given that this isn't exposed through XPIDL, I think describing with XPIDL
> is a bit confusing.
> 

Yeah... I wasn't sure what format to be used here, I couldn't find any function with this signature with an argument list in the comments. Now I can get creative and come up with something, but I would prefer to follow some existing pattern if there are any we're using in cases like this.

> @@ +3036,5 @@
> > +
> > +    size_t nameLength;
> > +    const jschar* nameChars = JS_GetStringCharsAndLength(cx, funName, &nameLength);
> > +    if (!nameLength) {
> > +        JS_ReportError(cx, "3rd argument");
> 
> Some of these strings passed to JS_ReportError could use some improvement.

Whooops... I don't know what happened there...

> > +static void
> > +CloneOrXrayError(JSContext *cx, uint32_t /* aErrorId */)
> > +{
> > +    JS_ReportError(cx, "XPC clone error.");
> 
> This functions seems dumb.

Yes. I'll look into it.

> > +    AutoObjectVector *xrayables = static_cast<AutoObjectVector *>(closure);
> > +    if (WrapperFactory::IsXrayable(obj)) {
> 
> As the code in WrapperFactory stands, this will bork for properties that are
> _themselves_ XrayWrappers (i.e. XOWs). Please add some tests for that.
> 

I think those at this point are either unwrapped by the cloning algorithm, or it threw because it could not checked unwrap it. Anyway, test is coming, and we'll see.

> @@ +3145,5 @@
> > +        xrayables->append(obj);
> > +        // Now that the object is rooted its pointer can be serialized
> > +        JSObject *rawptr = obj;
> > +        if (JS_WriteUint32Pair(writer, SCTAG_XPC_WRAPPEDNATIVE, 0) &&
> > +            JS_WriteBytes(writer, &rawptr, sizeof(rawptr))) {
> 
> This is totally unsafe with a moving GC. How about storing indices into the
> vector, instead?

I would love to read up on all these rooted stuff and moving GC. Is there any doc hidden somewhere for it? I kind of thought that rooting means telling the GC not to move the object because it's pointer is being used... But then it's more like, if the GC moves some object around it updates all the rooted that belongs to its pointer? I'm perfectly fine with the array id version, I just want to understand this.

> I'm not really comfortable with framing this around Xrays, because there are
> cases in which we have a reflector but no Xrays (like XPCOM objects for
> which we just make one reflector per scope). In general, such things
> shouldn't be showing up in content, but that's not a strong enough invariant
> to depend on these things coming up as Xray.

Well, it can be from chrome to nsEP too, not just to content... So yeah, this sounds good, do you have a simple case in mind by any chance that is easy to be tested? How can I detect these cases (I mean to extend the current IsXrayable)?


> @@ +3203,5 @@
> > +    for (unsigned i = 0; i < args.length(); i++) {
> > +        if (!CloneOrXray(cx, args.handleAt(i))) {
> > +            return false;
> > +        }
> > +    }
> 
> Hm, what about |this|?

Yeah... |this| is completely ignored in the function call forwarder, but this function shouldn't ignore it probably. Or at very least I should add some warning there. Actually I might want to use a cloned |this| in the forwarder too.

> 
> @@ +3289,5 @@
> > +    // on the calling context.
> > +    RootedValue exn(cx, UndefinedValue());
> > +    if (JS_GetPendingException(wndCx, exn.address())) {
> > +        JS_ClearPendingException(wndCx);
> > +        JS_SetPendingException(cx, exn);
> 
> Hm, are these two cxes guaranteed to be same-compartment here?

Seems like a bug. I cannot not cover this code path with any test.

> >  
> > +/* void 
> > +andbox(in AString source, in nativeobj sandbox); */
> 
> Er, what?

Looks like an 'EvalIn' search where I replaced that part with an enter instead of jumping to find next...

> 
> @@ +4603,5 @@
> >      return NS_OK;
> >  }
> >  
> >  JSBool
> > +MethodCallTransmitter(JSContext *cx, unsigned argc, Value *vp)
> 
> I don't think function vs method is really a meaningful distinction here. As
> far as I can tell, the main distinction is whether or not we do the clone,
> right? Can probably just templatize that on DoClone or something.

The differences are: they function call happens in different compartment, the |this| argument, and the cloning. Function vs method is silly, in an earlier version it kind of made sense. Cloning is the most descriptive probably. Also, maybe I should use a cloned |this| in the call forwarder...  

> 
> @@ +4630,5 @@
> > +    RootedValue v(cx, js::GetFunctionNativeReserved(&args.callee(), 0));
> > +    NS_ASSERTION(v.isObject(), "weird function");
> > +    RootedObject origFun(cx, &v.toObject());
> > +    {
> > +        JSAutoCompartment ac(cx, origFun);
> 
> This is a GC hazard. the FunctionNativeReserved _must_ be wrapped into the
> compartment of its function object, otherwise you have cross-compartment
> references living on the heap.

Right. So what about moving GC here? How will storing object pointers like this ever work with moving GC?

> > +    static bool IsXrayable(JSObject *obj);
> 
> Let's just move GetXrayType from XrayWrapper.h to here and expose that.

Will that catch those reflectors you mentioned earlier?

Comment 28

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] (sick) from comment #27)
> > This is totally unsafe with a moving GC. How about storing indices into the
> > vector, instead?
> 
> I would love to read up on all these rooted stuff and moving GC. Is there
> any doc hidden somewhere for it? I kind of thought that rooting means
> telling the GC not to move the object because it's pointer is being used...
> But then it's more like, if the GC moves some object around it updates all
> the rooted that belongs to its pointer? I'm perfectly fine with the array id
> version, I just want to understand this.

Yeah. Wikipedia has same basic information on it conceptually:

http://en.wikipedia.org/wiki/Garbage_collection_%28computer_science%29#Moving_vs._non-moving

And here are some docs that sfink wrote:
https://wiki.mozilla.org/Sfink/Draft_-_GC_Pointer_Handling

The basic idea is that a GC can move objects around, so all references to objects need to be using Handle<T> or Rooted<T>, so that the fact that the GCthing has moved can be abstracted away. This is the motivation behind all the exact rooting work.

> > I'm not really comfortable with framing this around Xrays, because there are
> > cases in which we have a reflector but no Xrays (like XPCOM objects for
> > which we just make one reflector per scope). In general, such things
> > shouldn't be showing up in content, but that's not a strong enough invariant
> > to depend on these things coming up as Xray.
> 
> Well, it can be from chrome to nsEP too, not just to content... So yeah,
> this sounds good, do you have a simple case in mind by any chance that is
> easy to be tested? How can I detect these cases (I mean to extend the
> current IsXrayable)?

Well, long-term, we don't want any per-compartment XPCWNs to be exposed to content at all. So I don't think we really need to build tests around them. I think a simple IsReflector() function somewhere in xpcpublic.h that checks IS_WN_REFLECTOR || IsDOMObject should be sufficient. We could probably use it in a few other places as well.

> > Hm, what about |this|?
> 
> Yeah... |this| is completely ignored in the function call forwarder, but
> this function shouldn't ignore it probably. Or at very least I should add
> some warning there. Actually I might want to use a cloned |this| in the
> forwarder too.

I think we should probably just ban |this| entirely (that is to say, just set it to undefined or whatever). |this| tends to be used for storing state, so cloning might not be useful (multiple calls would get a different object each time).

> > This is a GC hazard. the FunctionNativeReserved _must_ be wrapped into the
> > compartment of its function object, otherwise you have cross-compartment
> > references living on the heap.
> 
> Right. So what about moving GC here? How will storing object pointers like
> this ever work with moving GC?

The GC knows about reserved slots, and updates them appropriately.


> 
> > > +    static bool IsXrayable(JSObject *obj);
> > 
> > Let's just move GetXrayType from XrayWrapper.h to here and expose that.
> 
> Will that catch those reflectors you mentioned earlier?

This review comment is more or less obsolete I suppose - I wrote it before I decided that we shouldn't be thinking about this in terms of Xray at all.

Comment 29

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #28)
> I think we should probably just ban |this| entirely (that is to say, just
> set it to undefined or whatever). 

Last bits to fix here... Problem is if I pass in a non-object value as the |this| for JS_CallFunctionValue, the global of the function object will be used. Which is not too bad, since that's the global where we export from, which cannot be manipulated from content side. And in a way even makes sense... I think I would prefer undefined too, but I don't know much about the JS internals here, so cannot really find a workaround, nor am I sure that I should just hack my way around here.

Comment 30

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #29)
> (In reply to Bobby Holley (:bholley) from comment #28)
> > I think we should probably just ban |this| entirely (that is to say, just
> > set it to undefined or whatever). 
> 
> Last bits to fix here... Problem is if I pass in a non-object value as the
> |this| for JS_CallFunctionValue, the global of the function object will be
> used.

It actually depends whether "use strict" is set for the callee or not. But I don't think we really care of |this| gets unboxed to the callee global or not. It's really just a detail of the callee. If the callee wanted, it could equivalently do:

var thisObj = this;
function calle() {
  // |thisObj === this| for non-strict
}

Comment 31

[Gabor Krizsanits (INACTIVE)]

Attachment: export helpers v3

(In reply to Bobby Holley (:bholley) from comment #30)
> It actually depends whether "use strict" is set for the callee or not. But I
> don't think we really care of |this| gets unboxed to the callee global or
> not. 

Exactly! Alright then. This should be it more or less. Also, sorry for the lag on this one, just there were a few other stuff that kept me busy recently.

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 772180 [details] [diff] [review]
export helpers v3

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

Still some stuff to be worked out.

::: js/xpconnect/src/XPCComponents.cpp
@@ +2944,5 @@
> + * this helper function creates a native function in the target
> + * compartment and transmits the call to the original function.
> + * That call will be different than a regular JS function call in
> + * that, the |this| argument will be always the original scope of
> + * the exported function, and all the non-native JS object arguments

Not quiet true, since it depends on strictness. Just say "|this| is left unbound".

@@ +2945,5 @@
> + * compartment and transmits the call to the original function.
> + * That call will be different than a regular JS function call in
> + * that, the |this| argument will be always the original scope of
> + * the exported function, and all the non-native JS object arguments
> + * will be structure cloned.

"will be cloned using the structured clone algorithm".

@@ +2946,5 @@
> + * That call will be different than a regular JS function call in
> + * that, the |this| argument will be always the original scope of
> + * the exported function, and all the non-native JS object arguments
> + * will be structure cloned.
> + * The wrapped new transmitter function created will be the return

no more transmitters

@@ +2948,5 @@
> + * the exported function, and all the non-native JS object arguments
> + * will be structure cloned.
> + * The wrapped new transmitter function created will be the return
> + * value. The 3rd argument is the name of the property that will
> + * be set on the target scope, with the transmitter function as

same here.

@@ +2950,5 @@
> + * The wrapped new transmitter function created will be the return
> + * value. The 3rd argument is the name of the property that will
> + * be set on the target scope, with the transmitter function as
> + * the value.
> + * Principal of the caller must subsume that of the target.

'The principal'

@@ +2955,5 @@
> + *
> + * Expected type of the arguments and the return value jsvals:
> + * JSFunction ExportFunction(JSFunction funToExport,
> + *                           JSObject targetScope,
> + *                           JSString name)

I'd just say 'function', 'object', and 'string', since this is from the perspective of script (and also make the first letter lowercase?). JSFunction isn't something that a script can really access, for example.

@@ +2960,5 @@
> + */
> +static JSBool
> +ExportFunction(JSContext *cx, unsigned argc, jsval *vp)
> +{
> +

Nit: Remove these extra newlines.

@@ +2963,5 @@
> +{
> +
> +    MOZ_ASSERT(cx);
> +    if (argc < 3) {
> +        JS_ReportError(cx, "Function requires at least 3 arguments.");

JS_ReportError messages shouldn't have a period. Fix this in all of the calls?

@@ +2981,5 @@
> +    // It's OK to use UncheckedUnwrap here, since if we have a reference to
> +    // this function object, it means we can call it at least and exporting
> +    // needs only a permission for calling it.
> +    funObj = UncheckedUnwrap(funObj);
> +    if (!JS_ObjectIsCallable(cx, funObj) || js::IsWrapper(funObj)) {

Why do we need to check js::IsWrapper here? UncheckedUnwrap should either strip all wrappers (aside from the outer window, which isn't callable) or return null.

@@ +2984,5 @@
> +    funObj = UncheckedUnwrap(funObj);
> +    if (!JS_ObjectIsCallable(cx, funObj) || js::IsWrapper(funObj)) {
> +        JS_ReportError(cx, "First argument must be a function.");
> +        return false;
> +    }

Ok, so why do we unwrap funObj here? It seems like we do this, and then we enter the compartment of targetScope, and then we create the FunctionForwarder, which defines funObj as the native reserved slot on the forwarder object.

These two objects aren't necessarily same-compartment, which is a GC hazard. I complained about this in comment 26. The fact that the test coverage doesn't hit a compartment mismatch here means we either need better tests or better compartment assertions.

@@ +2990,5 @@
> +    // We can only export functions to scopes those are transparent for us,
> +    // so if there is a security wrapper around targetScope we must throw.
> +    targetScope = CheckedUnwrap(targetScope);
> +    if (!targetScope) {
> +        JS_ReportError(cx, "Permission failed to export into the target scope.");

"Permission denied to export function into scope. Bonus points for figuring out the varargs syntax so that you can include funName in the error message.

@@ +2997,5 @@
> +
> +    size_t nameLength;
> +    const jschar* nameChars = JS_GetStringCharsAndLength(cx, funName, &nameLength);
> +    if (!nameLength) {
> +        JS_ReportError(cx, "3rd argument should be a not empty string.");

"non-empty string"

@@ +3001,5 @@
> +        JS_ReportError(cx, "3rd argument should be a not empty string.");
> +        return false;
> +    }
> +
> +    RootedValue rval(cx);

Why do we need this? Can't we just do args.rval() whenever we need it?

@@ +3008,5 @@
> +        // its compartment.
> +        JSAutoCompartment ac(cx, targetScope);
> +
> +        // And now, let's create a helper function in the target compartment
> +        // around it, so the function will look like a regular function object.

around what?

@@ +3009,5 @@
> +        JSAutoCompartment ac(cx, targetScope);
> +
> +        // And now, let's create a helper function in the target compartment
> +        // around it, so the function will look like a regular function object.
> +        // (Note: there are differences when it is called see above.)

I found this line confusing to read - I'd just nix it.

@@ +3011,5 @@
> +        // And now, let's create a helper function in the target compartment
> +        // around it, so the function will look like a regular function object.
> +        // (Note: there are differences when it is called see above.)
> +        RootedId id(cx);
> +        if (!JS_ValueToId(cx, args[2], id.address())) {

The handling of 'name' in this function feels sloppy. First we convert it to a JSString, then we get its chars, then we convert the original value to an id. Can we just do it all with an id?

@@ +3015,5 @@
> +        if (!JS_ValueToId(cx, args[2], id.address())) {
> +            JS_ReportError(cx, "Converting name to an id failed.");
> +            return false;
> +        }
> +        if (!NewFunctionForwarder(cx, targetScope, id, funObj, /*doclone=*/ true, &rval)) {

/* doclone = */

@@ +3020,5 @@
> +            JS_ReportError(cx, "Exporting function failed.");
> +            return false;
> +        }
> +
> +        // We have the transmitter function in the target compartment, now

no more transmitters. grep for this?

@@ +3024,5 @@
> +        // We have the transmitter function in the target compartment, now
> +        // we have to add it to the target scope as a property.
> +        if (!JS_DefineUCProperty(cx, targetScope, nameChars, nameLength, rval,
> +                                 JS_PropertyStub, JS_StrictPropertyStub,
> +                                 JSPROP_READONLY | JSPROP_ENUMERATE))

I think we should be using JS_DefinePropertyById unless we really need to do otherwise.

@@ +3026,5 @@
> +        if (!JS_DefineUCProperty(cx, targetScope, nameChars, nameLength, rval,
> +                                 JS_PropertyStub, JS_StrictPropertyStub,
> +                                 JSPROP_READONLY | JSPROP_ENUMERATE))
> +        {
> +            JS_ReportError(cx, "Define property on target scope failed.");

You don't need to JS_ReportError when a JSAPI call fails. The underlying code should set an exception if it returns false, unless it OOMs (which is what it means to return false without setting an exception).

@@ +3033,5 @@
> +    }
> +
> +    // Finally we have to re-wrap the exported function back to the caller compartment.
> +    if (!JS_WrapValue(cx, rval.address())) {
> +        JS_ReportError(cx, "Wrapping result failed.");

Same here - no need to JS_ReportError.

@@ +3042,5 @@
> +    return true;
> +}
> +
> +JSBool
> +GetFilenameAndLineNumber(JSContext *cx, nsACString &filename, unsigned &lineno)

This should be static, right? Also, we never want to use JSBool except for in JSAPI functions. Just use bool.

@@ +3046,5 @@
> +GetFilenameAndLineNumber(JSContext *cx, nsACString &filename, unsigned &lineno)
> +{
> +    JSScript *script;
> +    if (JS_DescribeScriptedCaller(cx, &script, &lineno)) {
> +        if (const char *cfilename = ::JS_GetScriptFilename(cx, script)) {

Don't ::-prefix, please.

@@ +3047,5 @@
> +{
> +    JSScript *script;
> +    if (JS_DescribeScriptedCaller(cx, &script, &lineno)) {
> +        if (const char *cfilename = ::JS_GetScriptFilename(cx, script)) {
> +            filename.Assign(nsDependentCString(cfilename));

nsACString.Assign() always copies, and doesn't do any tricks if you pass it an nsDependentCString, right? Please confirm this with someone who knows about strings, since otherwise we'd have a GC hazard.

@@ +3061,5 @@
> +}
> +
> +enum XPCCloneTags {
> +    SCTAG_BASE = JS_SCTAG_USER_MIN,
> +    SCTAG_XPC_REFLECTOR

as noted in comment 26, this can just be ForwarderCloneTags and SCTAG_REFLECTOR, since this enum isn't in a header.

@@ +3067,5 @@
> +
> +static void
> +CloneNonReflectorsError(JSContext *cx, uint32_t /* aErrorId */)
> +{
> +}

Why is this function empty? Can we not supply a null callback if we don't want to do anything?

@@ +3106,5 @@
> +    // We need to maintain a list of reflectors to make sure all these objects
> +    // are properly rooter. Only their indices will be serialized.
> +    AutoObjectVector *reflectors = static_cast<AutoObjectVector *>(closure);
> +    if (IsReflector(obj)) {
> +        reflectors->append(obj);

isn't this fallible?

@@ +3107,5 @@
> +    // are properly rooter. Only their indices will be serialized.
> +    AutoObjectVector *reflectors = static_cast<AutoObjectVector *>(closure);
> +    if (IsReflector(obj)) {
> +        reflectors->append(obj);
> +        JSObject *rawptr = obj;

What's this about?

@@ +3119,5 @@
> +    JS_ReportError(cx, "CloneNonReflectorsWrite error.");
> +    return false;
> +}
> +
> +JSStructuredCloneCallbacks gXPCStructuredCloneCallbacks = {

I'd prefer not to call these 'xpc callbacks', since they're really just for function forwarder. gForwarderStructuredCloneCallbacks?

@@ +3149,5 @@
> +            return false;
> +        }
> +    }
> +
> +    // Now let's create the clones in the target compartment.

Now, recreate the object in the target compartment.

@@ +3162,5 @@
> +    return true;
> +}
> +
> +JSBool
> +CloneNonReflectors(JSContext *cx, CallArgs &args)

This overload is just used in one place, right? I think I'd prefer to inline it.

@@ +3181,5 @@
> + * scope of a window. Principal of the caller must subsume the target's.
> + *
> + * Expected type of the arguments jsvals:
> + * jsval EvalInWindow(JSString script,
> + *                    JSObject window)

Same as exportFunction - let's use the js types rather than the C++ types, and lowercase the first letter.

@@ +3220,5 @@
> +        JS_ReportError(cx, "Second argument must be a window.");
> +        return false;
> +    }
> +
> +    nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(window);

Once we have an nsPIDOMWindow, there's no need to QI further. We can just static_cast it to nsGlobalWindow and call the methods directly.

@@ +3235,5 @@
> +        JS_ReportError(cx, "Cannot determine file name and line number.");
> +        return false;
> +    }
> +
> +    RootedValue rval(cx);

Again, not sure we need rval.

@@ +3266,5 @@
> +        } else {
> +            JS_ReportError(cx, "Exception cannot be wrapped.");
> +        }
> +        return false;
> +    }

Hm, do we really want to wrap the exception here? It seems like we should be similarly careful with that like we are with rval below.

@@ +4616,5 @@
> +}
> +
> +JSBool
> +NewFunctionForwarder(JSContext *cx, HandleObject scope, HandleId id, HandleObject callable,
> +                     bool doclone, MutableHandleValue vp)

Why does this function need to take |scope|? Can't it just assume cx is in the right compartment and do JS_GetGlobalForScopeChain(cx)?

@@ +4619,5 @@
> +NewFunctionForwarder(JSContext *cx, HandleObject scope, HandleId id, HandleObject callable,
> +                     bool doclone, MutableHandleValue vp)
> +{
> +    JSFunction *fun = js::NewFunctionByIdWithReserved(cx, doclone ? FunctionForwarderDoClone :
> +                                                                    FunctionForwarderNoClone,

Let's call these CloningFunctionForwarder and NonCloningFunctionForwarder.

::: js/xpconnect/src/xpcpublic.h
@@ +94,5 @@
>  {
>      return IS_WN_CLASS(js::GetObjectClass(obj));
>  }
>  
> +static bool IsReflector(JSObject *obj);

Erm, static linkage doesn't make sense for things that are declared in header files. Drop the static and put it in namespace xpc?

Comment 33

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) (on vacation 7/11-7/14) from comment #32)

> @@ +2984,5 @@
> > +    funObj = UncheckedUnwrap(funObj);
> > +    if (!JS_ObjectIsCallable(cx, funObj) || js::IsWrapper(funObj)) {
> > +        JS_ReportError(cx, "First argument must be a function.");
> > +        return false;
> > +    }
> 
> Ok, so why do we unwrap funObj here? It seems like we do this, and then we
> enter the compartment of targetScope, and then we create the
> FunctionForwarder, which defines funObj as the native reserved slot on the
> forwarder object.
> 
> These two objects aren't necessarily same-compartment, which is a GC hazard.
> I complained about this in comment 26. The fact that the test coverage
> doesn't hit a compartment mismatch here means we either need better tests or
> better compartment assertions.

Because I was dumb, and forgot to hit hg qref... sigh... Anyway, most of the nits you spotted exists in the current version as well. Thanks for the review before the holiday!

Comment 34

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) (on vacation 7/11-7/14) from comment #32)
> @@ +3011,5 @@
> > +        // And now, let's create a helper function in the target compartment
> > +        // around it, so the function will look like a regular function object.
> > +        // (Note: there are differences when it is called see above.)
> > +        RootedId id(cx);
> > +        if (!JS_ValueToId(cx, args[2], id.address())) {
> 
> The handling of 'name' in this function feels sloppy. First we convert it to
> a JSString, then we get its chars, then we convert the original value to an
> id. Can we just do it all with an id?

I just wanted to avoid all the complication that comes with an implicit ToString call and value to id conversion for things like numeric primitives. So I wanted to prevent everything else than non-empty strings. Shall I just skip the whole thing and do a ValueToId and forget the rest you mean?

> 
> 
> @@ +3047,5 @@
> > +{
> > +    JSScript *script;
> > +    if (JS_DescribeScriptedCaller(cx, &script, &lineno)) {
> > +        if (const char *cfilename = ::JS_GetScriptFilename(cx, script)) {
> > +            filename.Assign(nsDependentCString(cfilename));
> 
> nsACString.Assign() always copies, and doesn't do any tricks if you pass it
> an nsDependentCString, right? Please confirm this with someone who knows
> about strings, since otherwise we'd have a GC hazard.
> 

Yes, assign is safe. Ms2ger helped me validating this.

Comment 35

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #34) 
> Yes, assign is safe. Ms2ger helped me validating this.

CompileOptions concerns me a bit though, it does not keep the string alive. It should be a stack class really, but it's not. And there are some cases where it gets even copied... Bug 892643

Probably if there is an issue with it that should not be fixed here though...

Comment 36

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #34)
> > The handling of 'name' in this function feels sloppy. First we convert it to
> > a JSString, then we get its chars, then we convert the original value to an
> > id. Can we just do it all with an id?
> 
> I just wanted to avoid all the complication that comes with an implicit
> ToString call and value to id conversion for things like numeric primitives.
> So I wanted to prevent everything else than non-empty strings. Shall I just
> skip the whole thing and do a ValueToId and forget the rest you mean?

Talking to Waldo, it sounds like we're generally moving away from jsid and more towards JSString. So ideally given jsval v, we'd do:

if (!v.isString()) {
  // handle it.
}
RootedString str(cx, v.toString());
if (JS_GetStringLength(str) == 0) {
  // handle it.
}

// Apparently this is the only way to get a jsid here. :-( 
RootedId id(cx).
JS_ValueToId(cx, v, &id);



This mostly looks like what you're doing, but I think we can eliminate the char* stuff.


(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #35)
> CompileOptions concerns me a bit though, it does not keep the string alive.
> It should be a stack class really, but it's not. And there are some cases
> where it gets even copied... Bug 892643

Does the string get copied? Can you follow up and verify with someone that what your patch does is safe?

Comment 39

[Gabor Krizsanits (INACTIVE)]

Attachment: export helpers v4

I think SetFunctionNativeReserved should do a compartment check...

Other than that I hope I fixed all the mistakes/nits. The #include "nsGlobalWindow.h" is for the static_cast. Let me know if you see something else to be fixed! And thanks for the help with this CompileOptions bug.

Comment 40

[Bobby Holley (:bholley)]

Comment on attachment 777717 [details] [diff] [review]
export helpers v4

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

Still some work needed, but I think I can r+ this, assuming you're very careful about addressing all the comments.

::: js/xpconnect/src/XPCComponents.cpp
@@ +2948,5 @@
> + * That call will be different than a regular JS function call in
> + * that, the |this| is left unbound, and all the non-native JS
> + * object arguments will be cloned using the structured clone
> + * algorithm.
> + * The wrapped new forwarder function created will be the return

The return value is the new forwarder function, wrapped into the caller's compartment.

@@ +3004,5 @@
> +            return false;
> +        }
> +
> +        // The function forwarder will live in the target compartment. Since
> +        // this function will be referenced from its private slot, to avoid

'to avoid a'

@@ +3024,5 @@
> +        // We have the forwarder function in the target compartment, now
> +        // we have to add it to the target scope as a property.
> +        if (!JS_DefinePropertyById(cx, targetScope, id, args.rval(),
> +                                   JS_PropertyStub, JS_StrictPropertyStub,
> +                                   JSPROP_READONLY | JSPROP_ENUMERATE))

JSPROP_READONLY isn't very meaningful without JSPROP_PERMANENT here, since code can just delete/recreate the property (or reconfigure it).

More to the point though, do we really want the prevent the callee from modifying the property? In general, properties exposed by DOM APIs are configurable. I think we probably want this to be, too.

@@ +3078,5 @@
> +            MOZ_ASSERT(IsReflector(reflector), "Object pointer must be a reflector!");
> +
> +            JS_WrapObject(cx, reflector.address());
> +            JS_ASSERT(WrapperFactory::IsXrayWrapper(reflector) ||
> +                      IS_WN_REFLECTOR(reflector));

I don't think this is a valid assertion. Suppose the content code gets a XOW reference to a new-DOM object in the chrome scope that exported the function. If it passes that to the clone, that clone will re-wrap the DOM object into its own compartment, giving a new DOM reflector. I think we need to assert IsXrayWrapper || IsReflector, here.

@@ +3120,5 @@
> +};
> +
> +/*
> + * This is a special structured cloning, that clones only non-reflectors.
> + */

Please ad some comments explaining the compartment invariants here (which compartment cx is in, which compartment val is in, etc).

@@ +3167,5 @@
> +EvalInWindow(JSContext *cx, unsigned argc, jsval *vp)
> +{
> +    MOZ_ASSERT(cx);
> +    if (argc < 2) {
> +        JS_ReportError(cx, "Function requires at least two arguments");

'at least two'? Seems like it takes exactly two.

@@ +3188,5 @@
> +        return false;
> +    }
> +
> +    // Make sure that we have a window object.
> +    RootedObject inner(cx, CheckedUnwrap(targetScope, false));

Please add /* stopAtOuter = */ to the |false| here.

@@ +3209,5 @@
> +
> +    nsCString filename;
> +    unsigned lineNo;
> +    if (!GetFilenameAndLineNumber(cx, filename, lineNo)) {
> +        JS_ReportError(cx, "Cannot determine file name and line number");

This effectively means that we can't ever call this function from a non-scripted caller, which isn't great. Can we default to something instead?

@@ +3216,5 @@
> +
> +    nsDependentJSString srcDepString;
> +    srcDepString.init(cx, srcString);
> +
> +    // Compile option must be created from the context

CompileOptions

@@ +3672,5 @@
>              return NS_ERROR_XPC_UNEXPECTED;
> +
> +        if (options.wantExportHelpers &&
> +            (!JS_DefineFunction(cx, sandbox, "exportFunction", ExportFunction, 0, 0) ||
> +             !JS_DefineFunction(cx, sandbox, "evalInWindow", EvalInWindow, 0, 0)))

Don't we want to pass a non-zero nargs here?

@@ +4574,5 @@
> +    NS_ASSERTION(v.isObject(), "weird function");
> +    RootedObject origFunObj(cx, UncheckedUnwrap(&v.toObject()));
> +    {
> +        JSAutoCompartment ac(cx, origFunObj);
> +        // Note: only the arguments are cloned nor the |this| or the |callee|.

s/nor/not/

::: js/xpconnect/tests/chrome/test_evalInWindow.xul
@@ +41,5 @@
> +           "Same origin object property access (cloning).");
> +        isnot(executeIn(frame.contentWindow, "document.obj"), frame.contentWindow.document.wrappedJSObject.obj,
> +              "Ensure cloning for js objects.");
> +        is(executeIn(frame.contentWindow, "document"), frame.contentWindow.document,
> +           "Xrayables should just pass without cloning.");

cal we also try some stuff like {a:{doc: document}} to make sure the deep cloning works properly?

@@ +43,5 @@
> +              "Ensure cloning for js objects.");
> +        is(executeIn(frame.contentWindow, "document"), frame.contentWindow.document,
> +           "Xrayables should just pass without cloning.");
> +
> +        //For some reason exception raised in this iframe cannot be detected from outside.

Um, I think we need to debug that and figure out what's going on. Please make sure this test passes.

Comment 41

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #40)
> > +        //For some reason exception raised in this iframe cannot be detected from outside.
> 
> Um, I think we need to debug that and figure out what's going on. Please
> make sure this test passes.

Finally I got some time to look into this. So it's a bit funky. First of, there is this JSOPTION_DONT_REPORT_UNCAUGHT flag we set on the sandbox context so we can avoid AutoLastFrameCheck in JS::Evaluate reports and clears the pending exception. I might or might not be allowed to set this flag temporarily (JS_SetOptions) on the window context, I would need some guidance here. But let's assume I'm allowed to do such thing and then comes the second problem, even if that flag is set, nsJSContext::EvaluateString cares very little about it and reports (and clears) the exception anyway... Would it be a terrible idea to do here a JSOPTION_DONT_REPORT_UNCAUGHT check too instead of reporting the pending exception unconditionally? Or do you have any other idea to fix this issue?

Comment 42

[Bobby Holley (:bholley)]

Yeah, we're going to need to fix this. Please make a separate patch underneath this one that does the following:

* Replace aCoerceToString with an EvaluateOptions structure defined in nsIScriptContext.h and operates similarly to CompileOptions.

* Introduce a reportUncaughtExceptions option on EvaluateOptions, defaulting to false.

* Define an RAII class AutoDontReportUncaught(JSContext* aCx) in nsJSEnvironment.cpp that ensures that JSOPTION_DONT_REPORT_UNCAUGHT is set on aCx for the lifetime of the object.

* Declare a Maybe<AutoDontReportUncaught> along with the scoped JSAutoCompartment. Construct it if reportUncaughtExceptions is false.

* If reportUncaughtExceptions is false, grab the exception value and pass it to the caller in aRetVal.

Then, modify your original patch to pass reportUncaughtExecptions=false, and harvest the exception (if it exists) from aRetVal.

Sound good?

Comment 43

[Bobby Holley (:bholley)]

I'm actually refactoring a bunch of this code, and so it would be easier for me to roll comment 42 into the refactor rather than rebasing. Is that ok with you, or are you in a hurry?

Comment 44

[Gabor Krizsanits (INACTIVE)]

Sounds good. This patch is very easy to refactor anyway, and I planned to do some cleaning up on my code in this file anyway, so it would be nice to see your refactoring first.

Comment 45

[Gabor Krizsanits (INACTIVE)]

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

Comment 46

[Gabor Krizsanits (INACTIVE)]

Attachment: part0: EvaluateOption for nsJSContext::EvaluateString

Comment 47

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: AutoDontReportUncaught for nsJSContext::EvaluateString

Comment 48

[Gabor Krizsanits (INACTIVE)]

Attachment: part 2: export helpers v5

I updated this patch as well. Carried over the flag from earlier, but I think you should really check the changes in evalInWindow around the exception propagation, since that part has not been reviewed yet, so please change the r+ flag accordingly.

Comment 49

[Bobby Holley (:bholley)]

Comment on attachment 790226 [details] [diff] [review]
part0: EvaluateOption for nsJSContext::EvaluateString

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

Wait, what? Why do we need this? The whole point here is to migrate consumers off of nsIScriptContext methods, and start using nsJSUtils directly. That's why I put it on nsJSUtils and not on nsIScriptContext...

Comment 50

[Bobby Holley (:bholley)]

Comment on attachment 790227 [details] [diff] [review]
part1: AutoDontReportUncaught for nsJSContext::EvaluateString

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

As mentioned, I think this should go in nsJSUtils.

Comment 51

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #49)
> Comment on attachment 790226 [details] [diff] [review]
> part0: EvaluateOption for nsJSContext::EvaluateString
> 
> Review of attachment 790226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wait, what? Why do we need this? The whole point here is to migrate
> consumers off of nsIScriptContext methods, and start using nsJSUtils
> directly. That's why I put it on nsJSUtils and not on nsIScriptContext...

Because of this in nsJSContext::EvaluateString:
if (!mScriptsEnabled)

Or is that covered by nsJSUtils and I misunderstood the whole thing?

Comment 52

[Bobby Holley (:bholley)]

You could just call GetScriptsEnabled on the context for now. It's not clear to me whether we should care about whether script is enabled on the callee or not, but it's probably safest just to do that for now.

Comment 53

[Gabor Krizsanits (INACTIVE)]

Attachment: part0: AutoDontReportUncaught

Hmmm... I think we misunderstood each other on IRC the last time. But yeah, this version makes more sense I guess.

Comment 54

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: exportFunction v6

So I'm doing the GetScriptsEnabled bits here, and I use nsJSUtils::EvaluateString now.

Comment 55

[Bobby Holley (:bholley)]

Comment on attachment 790892 [details] [diff] [review]
part0: AutoDontReportUncaught

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

r=bholley

::: dom/base/nsJSUtils.cpp
@@ +193,5 @@
>    *aFunctionObject = JS_GetFunctionObject(fun);
>    return NS_OK;
>  }
>  
> +MOZ_STACK_CLASS class AutoDontReportUncaught {

MOZ_STACK_CLASS goes between |class| and the class name. Also, we should use Gecko style here, so the brace goes on its own line.

@@ +195,5 @@
>  }
>  
> +MOZ_STACK_CLASS class AutoDontReportUncaught {
> +  JSContext* cx;
> +  bool wasSet;

mContext and mWasSet.

@@ +197,5 @@
> +MOZ_STACK_CLASS class AutoDontReportUncaught {
> +  JSContext* cx;
> +  bool wasSet;
> +
> +public: 

whitespace

@@ +251,5 @@
>  
> +  Maybe<AutoDontReportUncaught> dontReport;
> +  if (!aEvaluateOptions.reportUncaught) {
> +    // We need to prevent AutoLastFrameCheck to delete the unreported
> +    // exception.

"We need to prevent AutoLastFrameCheck from reporting and clearing any pending exceptions."

Comment 56

[Bobby Holley (:bholley)]

Comment on attachment 790896 [details] [diff] [review]
part1: exportFunction v6

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3214,5 @@
> +        return false;
> +    }
> +
> +    if (!context->GetScriptsEnabled()) {
> +        JS_ReportError(cx, "Running scripts is not enabled in this window.");

"Scripts are disabled in this window."

@@ +3233,5 @@
> +    // CompileOptions must be created from the context
> +    // we will execute this script in.
> +    JSContext *wndCx = context->GetNativeContext();
> +    JS::CompileOptions compileOptions(wndCx);
> +    compileOptions.setFileAndLine(filename.get(), lineNo);

I think this should go inside the braces after we push the JSContext. We should never pass a cx into the engine when it isn't stack-top.

@@ +3235,5 @@
> +    JSContext *wndCx = context->GetNativeContext();
> +    JS::CompileOptions compileOptions(wndCx);
> +    compileOptions.setFileAndLine(filename.get(), lineNo);
> +
> +    // We don't want the JS engine to auto report any uncaught exception.

We don't want the JS engine to automatically report uncaught exceptions.

@@ +3255,5 @@
> +            // exception is raised.
> +            MOZ_ASSERT(!JS_IsExceptionPending(wndCx),
> +                       "Exception should be delivered as return value.");
> +            if (args.rval().isUndefined())
> +                JS_ReportError(cx, "Script evaluation failed");

We should do:
if (args.rval().isUndefined()) {
  MOZ_ASSERT(rv == NS_ERROR_OUT_OF_MEMORY);
  return rv;
}

And then kill the else { indentation below.

@@ +3263,5 @@
> +                RootedValue exn(wndCx, args.rval());
> +                // First we should reset the return value.
> +                args.rval().set(UndefinedValue());
> +                if (!CloneNonReflectors(cx, &exn)) {
> +                    JS_ReportError(cx, "Exception cannot be cloned");

Doesn't CloneNonReflectors already set an exception if it returns false? In general, that is the convention for JSAPI-aware functions.

@@ +3275,5 @@
> +
> +    // Let's clone the return value back to the callers compartment.
> +    if (!CloneNonReflectors(cx, args.rval())) {
> +        args.rval().set(UndefinedValue());
> +        JS_ReportError(cx, "Cloning result failed");

same here.

Comment 57

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: exportFunction v7

(In reply to Bobby Holley (:bholley) from comment #56)
> We should do:
> if (args.rval().isUndefined()) {
>   MOZ_ASSERT(rv == NS_ERROR_OUT_OF_MEMORY);
>   return rv;
> }

The return type is bool here, so I must report the exception manually. Also, I removed the periods from JS_ReportError messages (they crawled back in evalInWindow somehow...)

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

Comment 58

[Bobby Holley (:bholley)]

Comment on attachment 791264 [details] [diff] [review]
part1: exportFunction v7

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3101,5 @@
> +    AutoObjectVector *reflectors = static_cast<AutoObjectVector *>(closure);
> +    if (IsReflector(obj)) {
> +        if (!reflectors->append(obj))
> +            return false;
> +      

Whitespace error.

@@ +3232,5 @@
> +
> +    // We don't want the JS engine to automatically report
> +    // uncaught exceptions.
> +    nsJSUtils::EvaluateOptions evaluateOptions;
> +    evaluateOptions.setReportUncaught(false);

I would hoist this below with compileOptions.

@@ +3257,5 @@
> +            MOZ_ASSERT(!JS_IsExceptionPending(wndCx),
> +                       "Exception should be delivered as return value.");
> +            if (args.rval().isUndefined()) {
> +                MOZ_ASSERT(rv == NS_ERROR_OUT_OF_MEMORY);
> +                JS_ReportError(cx, "Script evaluation failed");

Please remove the JS_ReportError. In JSAPI convention, OOM is signaled by returning false without setting an exception.

Comment 59

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0a766f2f6b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4c4e56f4bb

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad0a766f2f6b
https://hg.mozilla.org/mozilla-central/rev/9b4c4e56f4bb