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?

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

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

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.

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.

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?

(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.

oh, hmm. What happens in the case there is null on the stack.

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.

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 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....

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

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 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

> 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.

(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 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?

> 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.

(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.

> Ok. As long as you promise to go fix it. :-) Filed bug 807369, bug 807371.

Comment on attachment 677047 [details] [diff] [review] part 0.5. Don't require a JSContext argument for nsJSUtils::GetStaticScriptGlobal. woohoo! r=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!

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 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 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 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 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 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 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 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 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();|

> 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...

Will look at rest of review comments later today.

(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.

> 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.

> 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.

> 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 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.

> I'd just drop getParentObjectReturnType and use |descriptor.name + "*"| here. That would lose the TODO comment I want to appear in the generated code.

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

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.

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

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!