779048 - Need better representation for callback functions in WebIDL bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we just use a JSObject*.  This is OK for on* event stuff, which has all sorts of machinery to deal, but not great for other cases.

Ideally, we'd codegen a class per callback function type that can be called from C++ and does the call into JS and whatnot.

What all is involved in safely calling into JS nowadays?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Hmm, do we want to push null or some JSContext to context stack.

Comment 3

[Boris Zbarsky [:bzbarsky]]

That's part of the question, yes...

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Because these are generic stuff, I think we need to push cx to pretend to run using the
principal of the binding.

Also, one thing to remember is microtask handling.
nsAutoMicroTask mt; before calling JS.

Comment 5

[Bobby Holley (:bholley)]

We've got a number of vestigial security checks in CAPS and such that depend on the JS Context. My general sense is that you're probably ok if you just enter the compartment on any cx if you don't care about the script entry point. But I don't have a great idea of what all happens in the old crufty stuff, and probably won't until I find the time to sit down and remove it. But from a 1000-ft security view, being in the right compartment is most of the battle.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

We have plenty of checks relying on nsIJSContextStack to give something sane back.
If we just enter to a compartment, we don't push anything to cx stack, right?
But we do end up changing the compartment (and principal etc) of the topmost cx?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #6)
> If we just enter to a compartment, we don't push anything to cx stack, right?
> But we do end up changing the compartment (and principal etc) of the topmost
> cx?

Right, assuming the topmost cx is the one we enter the compartment on.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

oh, hmm.


What happens in the case there is null on the stack.

Comment 9

[Bobby Holley (:bholley)]

Then we need some sort of JS context to use (and push). Maybe the safe JS context?

Like I said, I'm probably not the best person to ask about this stuff right now, because I don't understand the vestigial cx-based security checks. I will soon when I go to remove them, but I don't have a great picture right now.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Implement a parent class for C++ reflections of callback functions in WebIDL.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Rearrange how we do our includes and forward-declares to actually work with callbacks and dictionaries sanely.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Refactor the code we use for generating example declarations a bit so we can reuse it for callbacks.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Implement basic codegen for callbacks, without handling of arguments or return values yet.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Make js-to-native conversion support doing something other than "return false" on JS exceptions.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Handling of return values for callbacks.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Make native-to-JS conversion support doing something other than "return false" on JS exceptions.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Handling of arguments for callbacks. that we're now using the array object as the "obj" when wrapping

sequence members, because we don't have an 'obj' around in our
code... and every single existing sequence-of-interfaces consumer
relied on there being an 'obj' floating about.

Also note that I needed to rearrange the various wrapping helpers so
that we can wrap things that are hanging out in const smartpointers or
in const OwningNonNull or in plain object references.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Give JS-to-native conversion an object pointer to work with so it can convert callback objects.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Start using the new callback codegen in argument and return value conversion.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 676319 [details] [diff] [review]
part 4.  Implement basic codegen for callbacks, without handling of arguments or return values yet.

Grrrr.  This doesn't seem to compile on non-Mac?  :(

Back to trying to figure out how to do this nullptr thing....

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4, should compile on all platforms

Comment 22

[Boris Zbarsky [:bzbarsky]]

Try looks greenish: https://tbpl.mozilla.org/?tree=Try&rev=2d79a6991a84

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 676306 [details] [diff] [review]
part 1.  Implement a parent class for C++ reflections of callback functions in WebIDL.

>+JSContext*
>+CallbackFunction::FindJSContext(nsIScriptContext*& aScriptContext)
>+{
>+  // We need to produce a useful JSContext here.  Ideally one that the callable
>+  // is in some sense associated with, so that we can sort of treat it as a
>+  // "script entry point".
>+
>+  // First, find the global of the underlying callable.
>+  JSObject* realCallable = js::UnwrapObject(mCallable);
>+  JSObject* global = js::GetGlobalForObjectCrossCompartment(realCallable);
>+
>+  // Now get the nsIScriptGlobalObject for this global.  This is much like
>+  // nsJSUtils::GetStaticScriptGlobal, except we don't have a JSContext to work
>+  // with yet.
>+  js::Class* clazz = js::GetObjectClass(global);
This part I don't know about.



>+  nsresult rv = nsContentUtils::GetSecurityManager()->
>+      CheckFunctionAccess(cx, aCallable, thisObjForSecurityCheck);
Would be great to get rid of this. I don't see the reason for this.
Perhaps ask still jst, peterv, mrbkap

>+void
>+CallbackFunction::CallSetup::ReportPendingException(JSContext* cx)
>+{
>+  // set aside the frame chain, since it has nothing to do with the
>+  // exception we're reporting.
>+  if (::JS_IsExceptionPending(cx)) {
>+    bool saved = ::JS_SaveFrameChain(cx);
>+    ::JS_ReportPendingException(cx);
>+    if (saved)
>+      ::JS_RestoreFrameChain(cx);
if (saved) {
  ::JS_RestoreFrameChain(cx);
}

And I'd drop :: before JS

>+  CallbackFunction(JSContext* cx, JSObject* aOwner, JSObject *aCallable,
>+                   bool* aInited)
s/JSObject *aCallable/ JSObject* aCallable/
I hope this gets called automatically when callback is passed to C++ side.


>+  JSObject* Callable() const {
{ should be in the next line

>+    xpc_UnmarkGrayObject(mCallable);
>+    return mCallable;
>+  }
>+
>+protected:
>+  void DropCallback() {
{ should be in the next line

>+  class NS_STACK_CLASS CallSetup {
{ should be in the next line

>+    JSContext* GetContext() const {
>+      return mCx;
>+    }
ditto

Bobby needs to review compartment handling.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 676343 [details] [diff] [review]
part 10.  Start using the new callback codegen in argument and return value conversion.

I hope the JSContext* and ->Callable() are just temporary and in the
future we could just have EventHandler* aCallback

Comment 25

[Boris Zbarsky [:bzbarsky]]

> Would be great to get rid of this. I don't see the reason for this.

Well, at the moment it's the only thing doing an "is script enabled for that window?" check.  I'm happy to try to rationalize this somehow, but would somewhat prefer a followup there...  I'll ask mrbkap/jst/peterv about it.

>if (saved) {
>  ::JS_RestoreFrameChain(cx);
>}
>And I'd drop :: before JS

Will do.  That was direct copy/paste from nsJSEnvironment.  ;)

> I hope this gets called automatically when callback is passed to C++ side.

Yes, absolutely.  The binding code will do this.

> { should be in the next line
(several times)

OK.

> I hope the JSContext* and ->Callable() are just temporary and in the
> future we could just have EventHandler* aCallback

Yes.  That involves fixing nsEventListenerManager to hold EventHandleNonNull* instances and invoke them, which is a bit of a pain because non-WebIDL things like windows and nodes would still be passing in JSObject*.  I'll try to catch you to talk about a sane transition plan here.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #25)
> > Would be great to get rid of this. I don't see the reason for this.
> 
> Well, at the moment it's the only thing doing an "is script enabled for that
> window?" check. 
Ah, right that part. We certainly should do something simpler for that.


> I'm happy to try to rationalize this somehow, but would
> somewhat prefer a followup there...
Followup is fine

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 676306 [details] [diff] [review]
part 1.  Implement a parent class for C++ reflections of callback functions in WebIDL.

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

r=bholley with comments addressed.

::: dom/bindings/CallbackFunction.cpp
@@ +33,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mCallable)
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> +
> +JSContext*
> +CallbackFunction::FindJSContext(nsIScriptContext*& aScriptContext)

I don't really like this outparam here. Is this stuff performance-critical enough that we can't just call GetScriptContextFromJSContext when necessary? We're doing all sorts of junk here, so it seems like the call to JS_GetPrivate and the QI shouldn't be such a big deal.

At the very least, I'd like to null it out at the top of the function, so that various early-return cases don't need to worry about returning garbage to the caller.

@@ +45,5 @@
> +  JSObject* global = js::GetGlobalForObjectCrossCompartment(realCallable);
> +
> +  // Now get the nsIScriptGlobalObject for this global.  This is much like
> +  // nsJSUtils::GetStaticScriptGlobal, except we don't have a JSContext to work
> +  // with yet.

I think we can do better than copy-pasting here. It looks like aContext is used vestigially in GetStaticScriptGlobal - it's only used for JS_GetGlobalForObject, which means that its net effect is to perform an assertSameCompartment(aContext, aObj).

I'd be in favor of nerfing it entirely (replacing the call with js::GetGlobalForObjectCrossCompartment). This would involve removing the parameter from the 8 callsites, which should only take a minute. If you really don't want to do that, I'd accept a poor-man's version whereby aContext becomes optional, and a null aContext promises that aObj is a bonafide global object.

@@ +131,5 @@
> +
> +  // After this point we guarantee calling ScriptEvaluated() if we
> +  // have an nsIScriptContext.
> +  // XXXbz Why, if, say CheckFunctionAccess fails?  I know that's how
> +  // nsJSContext::CallEventHandler works, but is it required?

Given the implementation of ScriptEvaluated(), I don't see any reason why we should call it if we never actually call into JS.

@@ +139,5 @@
> +  // XXXbz Should we be passing in the js::UnwrapObject() of aCallable to
> +  // CheckFunctionAccess?  For that matter, should we be passing the
> +  // js::UnwrapObject() of *aThisObjPtr, in case it got wrapped into whatever
> +  // random compartment is on cx?  The code in there makes no sense if wrappers
> +  // are being passed in!

If we're going to be calling CheckFunctionAccess at all, I think we should be passing the js::UnwrapObject versions of both arguments.

@@ +144,5 @@
> +  JSObject* thisObjForSecurityCheck = *aThisObjPtr;
> +  if (!thisObjForSecurityCheck) {
> +    // This is annoying.  *aThisObjPtr can be null to indicate "use the global",
> +    // but CheckFunctionAccess doesn't deal with null.  But we still have to
> +    // call it, in case our JSContext has script disabled!

How about we alter the API so that CheckFunctionAccess doesn't do the CanExecuteScripts check, and fix the two callers to manually call ssm->CanExecuteScripts()? This will make it simpler to remove CheckFunctionAccess as a followup patch, and avoids this rats' nest here?

@@ +156,5 @@
> +
> +  // Construct a termination func holder even if we're not planning to
> +  // run any script.  We need this because we're going to call
> +  // ScriptEvaluated even if we don't run the script...  See XXX
> +  // comment above.

As mentioned, I don't think we should do that.

@@ +174,5 @@
> +  if (!JS_WrapObject(cx, aThisObjPtr)) {
> +    // Report the exception, since we won't do it in our destructor.
> +    ReportPendingException(cx);
> +    return;
> +  }

I assume the caller appropriately wraps the args into that compartment as well?

@@ +198,5 @@
> +  // XXXbz For that matter why do we need to manually call ScriptEvaluated at
> +  // all?  nsCxPusher::Pop will do that nowadays if !mScriptIsRunning, so the
> +  // concerns from bug 295983 don't seem relevant anymore.  Do we want to make
> +  // sure it's still called when !mScriptIsRunning?  I guess play it safe for
> +  // now and do what CallEventHandler did, which is call always.

Sigh, yeah. It's mostly moot since I'm about to remove termination functions.

@@ +219,5 @@
> +    ::JS_ReportPendingException(cx);
> +    if (saved)
> +      ::JS_RestoreFrameChain(cx);
> +  }
> +}

Hm, what about just making nsJSEnvironment::ReportPendingException public and calling it if we have an nsIScriptContext? Or if not, add a commenting noting that this is cribbed from nsJSContext::ReportPendingException?

::: dom/bindings/CallbackFunction.h
@@ +51,5 @@
> +    MOZ_ASSERT(JS_ObjectIsCallable(cx, aCallable));
> +    // If aOwner is not null, enter the compartment of aOwner's
> +    // underlying object.  Note that we do want to unwrap outer
> +    // proxies here, so we're really dealing with the underlying
> +    // object that the native knows and cares about.

Er, why? The outer window proxy is always same-compartment with its referent. So it's all the same from the perspective of the ensuing AutoCompartment, and unwrapping outer window proxies is generally a dangerous operation we want to avoid if possible (since it adds red flags when auditing). Can we remove the second param here?

@@ +58,5 @@
> +      JSAutoCompartment ac(cx, aOwner);
> +      *aInited = JS_WrapObject(cx, &mCallable);
> +    } else {
> +      *aInited = true;
> +    }

It seems like the only exceptional case we're guarding against here with aInited is when JS_WrapObject fails. Given that, wouldn't it be cleaner to just consolidate the failure logic with an early return under an if (!JS_WrapObject(...))?

it also seems like having an mCallable signals that mCallable has been held. So maybe we should pass nullptr in the initializer list and only set mCallable after the call to NS_HOLD_JS_OBJECTS)?

@@ +89,5 @@
> +  // Find a JSContext (and maybe an nsIScriptContext) to use for our call.  This
> +  // can return null if we decide that we really really shouldn't be making this
> +  // call.  Specifially if mCallable is associated with a navigated-away-from
> +  // window, since in that case the JSContext would not longer have anything to
> +  // do with the callable.

Hm, so this effectively forbids callbacks from running in navigated-away-from windows. Do we really want to do this? I don't see why not in principle, except that this check will become more of a pain to enforce once JSContexts go away (since we won't already be digging around with the DOM window). Do we really want to bake this behavior in?

Comment 28

[Boris Zbarsky [:bzbarsky]]

> Is this stuff performance-critical enough that we can't just call
> GetScriptContextFromJSContext when necessary?

In general, yes.  We're already doing way too much crap here, as you say, and we should try to get rid of as much of it as we can going forward, because I fully expect this to gate our performance in various cases, including event dispatch.

> At the very least, I'd like to null it out at the top of the function

Note that there is precisely one early return case...  I can do the nulling-out if you want, but would you be OK with documentation that says callers must not touch the ctx if cx is null and call that good enough?

> I think we can do better than copy-pasting here.

Sounds good.  I'll slide in a patch under this queue to make GetStaticScriptGlobal not take a cx.

> I don't see any reason why we should call it if we never actually call into JS.

Right, hence the XXX comment.  I'm sort of trying to mitigate risk by doing things in this order:

1)  Land this code looking as much like what we have right now as possible (with no
    consumers so far).
2)  Convert event dispatch to use this code (bug 807226) so it's actually being exercised
    and we might have a hope of finding bugs.
3)  Modify this code to be saner.

This will let me keep straight regressions from #2 and #3, which would get lumped together otherwise.

> If we're going to be calling CheckFunctionAccess at all, I think we should be
> passing the js::UnwrapObject versions of both arguments.

OK.  Will make that change.

> How about we alter the API so that CheckFunctionAccess doesn't do the
> CanExecuteScripts check

Would you be OK with a followup for that, after bug 807226?  See three-step process above.

Note that it's not as simple as moving CanExecuteScripts() to the callers, because whether it's called at all depends on the object principals involved...

> As mentioned, I don't think we should do that.

See above.

> I assume the caller appropriately wraps the args into that compartment as well?

Yes.

> Or if not, add a commenting noting that this is cribbed from
> nsJSContext::ReportPendingException?

I can do that.  Or we could put this as a function in nsJSUtils?

> Er, why? The outer window proxy is always same-compartment with its referent.

Ah, right.  OK, then we don't need to unwrap it here, good.  Will change that.

> Given that, wouldn't it be cleaner to just
> consolidate the failure logic with an early return under an if
> (!JS_WrapObject(...))?

Yes, will do, including passing null in initalizer.  This code sort of grew organically... ;)

> Hm, so this effectively forbids callbacks from running in navigated-away-from
> windows. Do we really want to do this?

I think so, yes.  Once you navigate away from a window, I don't think a user would expect any script from that window to run, really.

> since we won't already be digging around with the DOM window

We sure will, because script can be disabled on a per-window basis, so we need to find the right window to find out whether script is enabled at all.

Comment 29

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #28)
> > Is this stuff performance-critical enough that we can't just call
> > GetScriptContextFromJSContext when necessary?
> 
> In general, yes.  We're already doing way too much crap here, as you say,
> and we should try to get rid of as much of it as we can going forward,
> because I fully expect this to gate our performance in various cases,
> including event dispatch.

Ok.

> 
> > At the very least, I'd like to null it out at the top of the function
> 
> Note that there is precisely one early return case...  I can do the
> nulling-out if you want, but would you be OK with documentation that says
> callers must not touch the ctx if cx is null and call that good enough?

I'd still prefer to just null it out at the top of the function to be sure that we're not returning garbage in the outparam.

> > I don't see any reason why we should call it if we never actually call into JS.
> 
> Right, hence the XXX comment.  I'm sort of trying to mitigate risk by doing
> things in this order:
> 
> 1)  Land this code looking as much like what we have right now as possible
> (with no
>     consumers so far).
> 2)  Convert event dispatch to use this code (bug 807226) so it's actually
> being exercised
>     and we might have a hope of finding bugs.
> 3)  Modify this code to be saner.
> 
> This will let me keep straight regressions from #2 and #3, which would get
> lumped together otherwise.

Ok. As long as you promise to go fix it. :-)

> 
> > How about we alter the API so that CheckFunctionAccess doesn't do the
> > CanExecuteScripts check
> 
> Would you be OK with a followup for that, after bug 807226?  See three-step
> process above.

Ok, sure.
 
> > Or if not, add a commenting noting that this is cribbed from
> > nsJSContext::ReportPendingException?
> 
> I can do that.  Or we could put this as a function in nsJSUtils?

That sounds best IMO.

> We sure will, because script can be disabled on a per-window basis, so we
> need to find the right window to find out whether script is enabled at all.

Ok, sounds reasonable.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: part 0.5. Don't require a JSContext argument for nsJSUtils::GetStaticScriptGlobal.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Ok. As long as you promise to go fix it. :-)

Filed bug 807369, bug 807371.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 1 to address bholley's comments (diff -w)

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 1 to address bholley's comments (diff -w)

Comment 34

[Bobby Holley (:bholley)]

Comment on attachment 677047 [details] [diff] [review]
part 0.5.  Don't require a JSContext argument for nsJSUtils::GetStaticScriptGlobal.

woohoo! r=bholley

Comment 35

[Bobby Holley (:bholley)]

Comment on attachment 677056 [details] [diff] [review]
Interdiff for part 1 to address bholley's comments (diff -w)

>+      if (!JS_WrapObject(cx, &aCallable)) {
>+        *aInited = false;

I'd still prefer to just set this to false at the top of the function to avoid needing to do any state munging in the early return cases.

r=bholley. Thanks for the interdiff!

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 8 update: handle optional arguments sanely

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 with better compartment munging

Comment 38

[Boris Zbarsky [:bzbarsky]]

OK.  So I tried actually using this stuff, and CheckFunctionAccess is being a problem: I need a third arg for it, but to get that third arg I have to call xpconnect, and xpconnect expects a JSContext on the stack.

So I'm going to change part 1 so that we pass null for that third arg, and change the security manager to deal, because I don't think that part of CheckFunctionAccess is doing anything useful.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 1 to pass null as a third arg to CheckFunctionAccess

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 now with more cx-pushing sanity

Comment 41

[Bobby Holley (:bholley)]

Comment on attachment 677871 [details] [diff] [review]
Interdiff for part 1 to pass null as a third arg to CheckFunctionAccess

I'm in transit and jet-lagged, but this seems great to all the parts of my brain that are working right now. r=bholley

Comment 42

[Peter Van der Beken [:peterv]]

Comment on attachment 676313 [details] [diff] [review]
part 2.  Rearrange how we do our includes and forward-declares to actually work with callbacks and dictionaries sanely.

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

::: dom/bindings/Codegen.py
@@ +358,5 @@
> +    """
> +    types = []
> +    curDict = dictionary
> +    while curDict:
> +        types.extend([m.type for m in curDict.members])

No need for those braces (you can avoid creating a list here).

@@ +529,5 @@
> +        name = str(t)
> +        if not name in unionStructs:
> +            providers = getRelevantProviders(descriptor, dictionary,
> +                                             config)
> +            unionStructs[name] = CGUnionStruct(t, providers[0])

I guess unions on workers are broken, do we want to file a bug about that and reference it here?

@@ +539,5 @@
> +                        headers.add("mozilla/dom/TypedArray.h")
> +                    else:
> +                        for p in providers:
> +                            try:
> +                                typeDesc = p.getDescriptor(f.unroll().inner.identifier.name)

Why do we need another unroll() here?

Comment 43

[Peter Van der Beken [:peterv]]

Comment on attachment 676317 [details] [diff] [review]
part 3.  Refactor the code we use for generating example declarations a bit so we can reuse it for callbacks.

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

::: dom/bindings/Codegen.py
@@ +4474,5 @@
>                    'returnType': self.returnType,
>                    'name': self.name,
>                    'const': ' const' if self.const else '',
>                    'args': args,
>                    'body': body })

Can we line these up with templateClause somehow?

@@ +6531,5 @@
> +                                       breakAfterReturnDecl=" "))
> +        # Total hack: We want to declare our CC stuff, but CGClass
> +        # doesn't really have a way to do that.  Similar for our
> +        # destructor, actually.  So just pretend like it's part of the
> +        # return type of GetParentObject.

Seems to me we should at least add a ClassDestructor and use ClassConstructor/ClassDestructor for the constructor/desctructor.
For the CC stuff, maybe we want the ability to pass a generic string somewhere?

Comment 44

[Peter Van der Beken [:peterv]]

Comment on attachment 677872 [details] [diff] [review]
Part 4 now with more cx-pushing sanity

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

::: dom/bindings/Codegen.py
@@ +6718,5 @@
> +
> +    def getCallImpls(self, callCallback):
> +        args = list(callCallback.args)
> +        # Strip out the JSContext*/JSObject* args
> +        # that got added.

We should probably make sure that those are the first two arguments.

@@ +6738,5 @@
> +
> +        bodyWithThis = string.Template(
> +            setupCall+
> +            "// WrapNativeParent is a bit of a Swiss army knife that will\n"
> +            "// wrap anything for us.\n"

I wish we actually had a separate function for this (even if it's just named differently with the same implementation).

@@ +6767,5 @@
> +class CallCallback(CGNativeMember):
> +    def __init__(self, callback, descriptorProvider):
> +        sig = callback.signatures()[0]
> +        self.retvalType = sig[0]
> +        # Make a copy, since we're going to modify it

We don't seem to be modifying it though. If we don't need to then just use sig[1] directly.

Comment 45

[Peter Van der Beken [:peterv]]

Comment on attachment 676320 [details] [diff] [review]
part 5.  Make js-to-native conversion support doing something other than "return false" on JS exceptions.

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

::: dom/bindings/Codegen.py
@@ +2592,5 @@
>          enum = type.inner.identifier.name
>          if invalidEnumValueFatal:
>              handleInvalidEnumValueCode = "  MOZ_ASSERT(index >= 0);\n"
>          else:
> +            assert exceptionCode == "return false;"

I don't understand why we can assert this.

Comment 46

[Peter Van der Beken [:peterv]]

Comment on attachment 676334 [details] [diff] [review]
part 6.  Handling of return values for callbacks.

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

::: dom/bindings/Codegen.py
@@ +6332,3 @@
>          if type.isString():
>              if isMember:
> +                # No need for a third element in the isMember case

This seems to be the only case where isMember matters. Given how different this is from the rest of the code in this function, would it make sense to special-case isString() directly in the one caller that passes True for isMember? I'd really prefer this to always return a tuple with 3 elements, but that doesn't really make sense for isMember=True.

@@ +6374,5 @@
> +                # The decl is an OwningNonNull or nsRefPtr, depending
> +                # on whether we're nullable.
> +                returnCode = "return ${declName}.forget();"
> +            else:
> +                if type.nullable():

elif

@@ +6404,5 @@
> +            if type.nullable():
> +                returnCode = "return ${declName} ? ${declName}->Obj() : nullptr;"
> +            else:
> +                returnCode = ("%s& retval = ${declName};\n"
> +                              "return retval.Obj();" % type.name)

Is there a way to avoid retval here? Would |return static_cast<%s&>(${declName}).Obj();| work?

@@ +6923,5 @@
> +            "valPtr": "&rval",
> +            "holderName" : "rvalHolder",
> +            "declName" : "rvalDecl"
> +            }
> +        exceptionCode=("aRv.Throw(NS_ERROR_UNEXPECTED);\n"

Given this, it seems that in a lot of cases we'll end up with code that does:

ThrowErrorMessage(...);
aRv.Throw(NS_ERROR_UNEXPECTED);
return ...;

which looks odd to me (almost like we throw twice). I wonder if it would be at all possible to have two exceptionCodes, one that takes an error number and one that doesn't. We would then be able to ThrowTypeError/ReportTypeError for the first (maybe we need a ThrowAndReportTypeError?) and aRv.Throw(NS_ERROR_UNEXPECTED) for the second. Up to you.

Comment 47

[Peter Van der Beken [:peterv]]

Comment on attachment 677457 [details] [diff] [review]
Part 8 update: handle optional arguments sanely

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

::: dom/bindings/BindingUtils.h
@@ +709,5 @@
>  }
>  
>  template<class T>
>  inline bool
> +WrapObject(JSContext* cx, JSObject* scope, const nsCOMPtr<T> &p, const nsIID* iid,

Move the & to the left while you're here (and below).

@@ +1049,5 @@
>      MOZ_ASSERT(ptr, "NonNull<T> was set to null");
>      return ptr;
>    }
>  
> +  // Make us work with smart-ptr helpers that expect a get()

We could check for a Ptr() member instead. These get() seem out of place given the other members.

::: dom/bindings/Codegen.py
@@ +6978,5 @@
>          return convertType.define() + "\n" + assignRetval
>  
> +    def getArgConversions(self):
> +        # Just reget the arglist from self.callback, because our superclasses
> +        # just have way to many members they like to clobber, so I can't fine a

s/fine/find/

@@ +7001,5 @@
> +        if arg.optional:
> +            argval = "%s.Value()"
> +        else:
> +            argval = "%s"
> +        argval = argval % arg.identifier.name

argVal = arg.identifier.name
if arg.optional:
    argVal += ".Value()"

@@ +7030,5 @@
> +            conversion = (
> +                CGIfWrapper(CGGeneric(conversion),
> +                            "%s.WasPassed()" % arg.identifier.name).define() +
> +                " else {\n"
> +                "  if (argc == %d) {\n"

else if

Comment 48

[Peter Van der Beken [:peterv]]

Comment on attachment 676343 [details] [diff] [review]
part 10.  Start using the new callback codegen in argument and return value conversion.

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

::: content/base/src/WebSocket.h
@@ +55,3 @@
>                           ErrorResult& aRv)                              \
>    {                                                                     \
> +    JSObject* callback = aCallback ? aCallback->Callable() : nullptr;   \

How about making these |JS::Value callback = aCallback ? JS::ObjectValue(*aCallback->Callable()) : JS::NullValue();|

Comment 49

[Boris Zbarsky [:bzbarsky]]

> No need for those braces (you can avoid creating a list here).

Done.

> I guess unions on workers are broken, do we want to file a bug about that

Bug 809899.

> Why do we need another unroll() here?

We don't.  Removed.

> Can we line these up with templateClause somehow?

Done.

> Seems to me we should at least add a ClassDestructor and use
> ClassConstructor/ClassDestructor for the constructor/desctructor.

OK, I'll do that.  Do you want to see an interdiff for that?  And here, or followup?

> For the CC stuff, maybe we want the ability to pass a generic string somewhere?

Same thing: here or followup, and do you want to see an interdiff?

> We should probably make sure that those are the first two arguments.

Added asserts:

        assert args[0].name == "cx" and args[0].argType == "JSContext*"
        assert args[1].name == "aThisObj" and args[1].argType == "JSObject*"

> I wish we actually had a separate function for this (even if it's just named
> differently with the same implementation).

OK.  Added a function called WrapCallThisObject, which just calls WrapNativeParent and then makes sure the resulting object is in the right compartment.  Sound ok?

> We don't seem to be modifying it though.

Indeed, that's a holdover.  Removed.

> I don't understand why we can assert this.

Hmm.  I thought this was only true for attributes, but looks like it's actually true for method arguments.  I guess I need to handle this instead of just asserting...

Comment 50

[Boris Zbarsky [:bzbarsky]]

Will look at rest of review comments later today.

Comment 51

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #49)
> OK, I'll do that.  Do you want to see an interdiff for that?  And here, or
> followup?
> 
> > For the CC stuff, maybe we want the ability to pass a generic string somewhere?
> 
> Same thing: here or followup, and do you want to see an interdiff?

Given the stack of patches here, if you want to do some of these as an additional patch on top of the rest that's fine by me. I could easily look that over.

> OK.  Added a function called WrapCallThisObject, which just calls
> WrapNativeParent and then makes sure the resulting object is in the right
> compartment.  Sound ok?

Yes.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff that addresses part 3 review comments

Comment 53

[Boris Zbarsky [:bzbarsky]]

> I thought this was only true for attributes

Ah, here we go.  invalidEnumValueFatal is _false_ only for attributes.  So my new assert will never be reached unless attribute "arg conversion" code starts passing in an exceptionCode.  At which point we'll need to figure out what that even means.  So I think the assert is fine for now.

Comment 54

[Boris Zbarsky [:bzbarsky]]

> This seems to be the only case where isMember matters.

It also matters in the isGeckoInterface() case.

Would you be OK with just putting None in the third slot if isString() and isMember, and documenting that this will be the case?

> Would |return static_cast<%s&>(${declName}).Obj();| work?

Yes, done.

> which looks odd to me (almost like we throw twice).

This case is a  tiny bit weird, since it's calling from C++ to JS.  The ThrowErrorMessage will throw a JS exception (which will then later get reported).  The aRv.Throw() is to let us know that our call failed... we don't examine aRv ourselves in the CallbackFunction code.  So I'm not quite sure how the proposal would help here.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Attachment: Address part 6 review comments

Comment 56

[Boris Zbarsky [:bzbarsky]]

> Move the & to the left while you're here (and below).

Done.

> We could check for a Ptr() member instead. These get() seem out of place given the other
> members.

Hmm.  That would significantly complicate our template stuff...  I'm not sure it's worth it.  But I can do a followup if you'd like.

> s/fine/find/

Done.

> argVal = arg.identifier.name
> if arg.optional:
>    argVal += ".Value()"

Done.

> else if

Done.

> How about making these |JS::Value callback = aCallback ?
> JS::ObjectValue(*aCallback->Callable()) : JS::NullValue();|

This code is completely going away in bug 807226 (specifically in part 7 there), so I'm not going to worry about it here if that's ok.

Comment 57

[Peter Van der Beken [:peterv]]

Comment on attachment 679785 [details] [diff] [review]
Interdiff that addresses part 3 review comments

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

Thanks!

::: dom/bindings/Codegen.py
@@ +4844,5 @@
>  
> +class ClassDestructor(ClassItem):
> +    """
> +    Used for adding a constructor to a CGClass.
> +    

Trailing whitespace.

@@ +6854,3 @@
>          methodDecls.insert(0,
>                             ClassMethod("GetParentObject",
> +                                       getParentObjectReturnType,

I'd just drop getParentObjectReturnType and use |descriptor.name + "*"| here.

Comment 58

[Boris Zbarsky [:bzbarsky]]

> I'd just drop getParentObjectReturnType and use |descriptor.name + "*"| here.

That would lose the TODO comment I want to appear in the generated code.

Comment 59

[Boris Zbarsky [:bzbarsky]]

           http://hg.mozilla.org/integration/mozilla-inbound/rev/61d0e897a9f7
           https://hg.mozilla.org/integration/mozilla-inbound/rev/172241f160f1
           https://hg.mozilla.org/integration/mozilla-inbound/rev/c348f36c317b
           https://hg.mozilla.org/integration/mozilla-inbound/rev/e012fb02c566
           https://hg.mozilla.org/integration/mozilla-inbound/rev/fd824d059eb1
           https://hg.mozilla.org/integration/mozilla-inbound/rev/f53d2f3833fc
           https://hg.mozilla.org/integration/mozilla-inbound/rev/e32756c083ef
           https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8298f46852
           https://hg.mozilla.org/integration/mozilla-inbound/rev/07b9c4137182
           https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3ec856b05e
           https://hg.mozilla.org/integration/mozilla-inbound/rev/983d136130c5

Comment 60

[Boris Zbarsky [:bzbarsky]]

Filed bug 810324 on comment 54 last paragraph.

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5266596c96ac as a followup to add a comment.

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61d0e897a9f7
https://hg.mozilla.org/mozilla-central/rev/172241f160f1
https://hg.mozilla.org/mozilla-central/rev/c348f36c317b
https://hg.mozilla.org/mozilla-central/rev/e012fb02c566
https://hg.mozilla.org/mozilla-central/rev/fd824d059eb1
https://hg.mozilla.org/mozilla-central/rev/f53d2f3833fc
https://hg.mozilla.org/mozilla-central/rev/e32756c083ef
https://hg.mozilla.org/mozilla-central/rev/ed8298f46852
https://hg.mozilla.org/mozilla-central/rev/07b9c4137182
https://hg.mozilla.org/mozilla-central/rev/ae3ec856b05e
https://hg.mozilla.org/mozilla-central/rev/983d136130c5
https://hg.mozilla.org/mozilla-central/rev/5266596c96ac

Comment 62

[Boris Zbarsky [:bzbarsky]]

Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Callback_function_types and https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#CallbackFunction though I'd appreciate someone giving the docs a second look!