Three changes: 1) If we're not used, we never HoldJSObjects, which avoids all that work. 2) If we're not used, we never get the incumbent global. 3) If we're not used, we never get the stack (when stack saving is enabled).

Actually, nevermind on #2 and #3. I just realized that it fails if the callee calls right back into our callback. :(

Terrence, could you check the Trace method here in particular?

Comment on attachment 8753537 [details] [diff] [review] part 1. Add a way to construct a callback object without calling HoldJSObjects >+ // Places that use this need to ensure that the callback is traced (e.g. via a >+ // Rooted) until the HoldJSObjects call happens. >+ struct FastCallbackConstructor { >+ }; ok, this is one approach. Other would be perhaps to pass some enum to the ctor an based on the value call holdjs. But perhaps this is safer. Or could the public ctor call this protected ctor and explicitly call HoldJS? I guess that would be uglier since Init() is called in many places. ok, this is fine. >+ >+ // Just like the public version without the FastCallbackConstructor argument, >+ // except for not calling HoldJSObjects. If you use this, you MUST ensure >+ // that the object is traced until the HoldJSObjects happens! >+ CallbackObject(JSContext* aCx, JS::Handle<JSObject*> aCallback, >+ nsIGlobalObject *aIncumbentGlobal, Nit, could be consistent with style. * goes with type.

> But perhaps this is safer. Yeah, I wanted to make sure random consumers couldn't trigger the no-holding mode. > Nit, could be consistent with style. * goes with type. Will fix. I copy/pasted existing code. :(

Comment on attachment 8753539 [details] [diff] [review] part 2. Add a way to trace a RefPtr<T> or OwningNonNull<T> via a Rooted Review of attachment 8753539 [details] [diff] [review]: ----------------------------------------------------------------- That looks correct to me.

Comment on attachment 8753541 [details] [diff] [review] part 3. Add the codegen bits and Trace implementation to allow Web IDL callbacks to not have to HoldJSObjects until the bindings have determined that someone is actually holding on to the callback object Review of attachment 8753541 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't seem too unsavory, although it would be nice if you could get away with just Rooted<RefPtr<T>>. I guess perfect forwarding didn't work out?

I needed the nontrivial destructor here to do the real work. But yeah, I couldn't get forwarding in the assignment operator to work...

Comment on attachment 8753539 [details] [diff] [review] part 2. Add a way to trace a RefPtr<T> or OwningNonNull<T> via a Rooted rs+, this is all in terrence's world. It is interesting that those operators work, even though they all just return self.get(). I guess some nested operator magic makes it all good.

Comment on attachment 8753541 [details] [diff] [review] part 3. Add the codegen bits and Trace implementation to allow Web IDL callbacks to not have to HoldJSObjects until the bindings have determined that someone is actually holding on to the callback object > # Do codegen for all the callback interfaces. Skip worker callbacks. > cgthings.extend([CGCallbackInterface(x) for x in callbackDescriptors if > not x.workers]) > >+ cgthings.extend([CGNamespace('binding_detail', >+ CGFastCallback(x.interface)) >+ for x in callbackDescriptors if not x.workers]) >+ I wish I understood why we're skipping worker callbacks for callback interfaces but not for callbacks. Not really about this bug though. >+ traceMethod = ClassMethod("Trace", "void", >+ [Argument("JSTracer*", "aTracer")], >+ inline=True, >+ bodyInHeader=True, >+ visibility="public", >+ body="%s::Trace(aTracer);\n" % baseName) >+ holdMethod = ClassMethod("HoldJSObjectsIfMoreThanOneOwner", "void", >+ [], >+ inline=True, >+ bodyInHeader=True, >+ visibility="public", >+ body=( >+ "%s::HoldJSObjectsIfMoreThanOneOwner();\n" % >+ baseName)) I don't understand why we need these methods when the base class already has them. Just to make them public? I wouldn't mind seeing generated code before and after this patch. You tested the Optional<> case?

> It is interesting that those operators work, even though they all just return self.get(). self.get() returns the type the Rooted is templated over. Then that type itself may have implicit conversions to the actual return type of the operator (and does). > I wish I understood why we're skipping worker callbacks for callback interfaces but not for callbacks. I think just because https://hg.mozilla.org/mozilla-central/rev/3b16cca767c4 didn't add them. We needed the worker-only callbacks for something, but not worker-only callback interfaces... > I don't understand why we need these methods when the base class already has them. Just to make them public? Yes. The JS GC template machinery needs them public, or friend decls, and I didn't want random people calling it and putting in the right friend decls is a pain. So I just used public methods forwarding to the protected ones. > You tested the Optional<> case? Kinda. It failed to compile due to type mismatches (Optional<RootedCallback<OwningNonNull<Foo>>> being passed where the callee expected an Optional<OwningNonNull<Foo>>) in some of our bindings. If we cared, we could make it work with some effort (basically provide a conversion operator on a specialization of Optional<RootedCallback> that returns a new Optional with the thing inside the RootedCallback or something), but I decided it wasn't worth it.

Diff, then the full method before and the full method after.

(In reply to Boris Zbarsky [:bz] from comment #12) > Yes. The JS GC template machinery needs them public, or friend decls, and I > didn't want random people calling it and putting in the right friend decls > is a pain. So I just used public methods forwarding to the protected ones. ok > Kinda. It failed to compile due to type mismatches > (Optional<RootedCallback<OwningNonNull<Foo>>> being passed where the callee > expected an Optional<OwningNonNull<Foo>>) in some of our bindings. ok thanks. > If we > cared, we could make it work with some effort (basically provide a > conversion operator on a specialization of Optional<RootedCallback> that > returns a new Optional with the thing inside the RootedCallback or > something), but I decided it wasn't worth it. Yeah, probably not worth it. thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c08f417d85fd https://hg.mozilla.org/integration/mozilla-inbound/rev/9806d7a88802 https://hg.mozilla.org/integration/mozilla-inbound/rev/8f319062be85

OK, I backed all that out. I ran into two problems: 1) We actually need different destructors for the RefPtr and OwningNonNull case, because they need to check for "never assigned" differently. 2) Once I did that, MSVC refused to compile things for reasons I could not figure out; it kept thinking OwningNonNull is not a template or something. I guess I get to have the fun of try-debugging on Windows.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab87e09f57b

I tried bdb5b50fd858 with VS2015 locally, and saw the identical issue you met with the infra. And I found out that prefixing OwningNonNull in the constructor with mozilla:: works around this failure. You may want to try that.

Yeah, I figured out what's going on. It was seeing the typedef from part 2 that lives up the ancestor chain. I just removed the typedef and that made things happy.

Comment on attachment 8754459 [details] [diff] [review] Changes to the destructor to make things happy >+ template<typename U> >+ static bool IsInitialized(U& aArg); // Not implemented >+ >+ template<typename U> >+ static bool IsInitialized(RefPtr<U>& aRefPtr) >+ { >+ return aRefPtr; >+ } >+ >+ template<typename U> >+ static bool IsInitialized(OwningNonNull<U>& aOwningNonNull) >+ { >+ return aOwningNonNull.isInitialized(); >+ } >+ Curious, could this stuff be private? Isn't it used only in dtor. Wondering in which case IsInitialized returns false for OwningNonNull case? I mean I would expect OwningNonNull to be always initialized before trying to pass the callback to some (C++) method.

> Curious, could this stuff be private? Isn't it used only in dtor. Yes, it could. > Wondering in which case IsInitialized returns false for OwningNonNull case? Same as in <https://bug1273661.bmoattachments.org/attachment.cgi?id=8754083>. I'll add a comment here pointing to that one.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1af1c31b07a https://hg.mozilla.org/integration/mozilla-inbound/rev/bcaee6a9d587 https://hg.mozilla.org/integration/mozilla-inbound/rev/1287ce64f050

https://hg.mozilla.org/mozilla-central/rev/a1af1c31b07a https://hg.mozilla.org/mozilla-central/rev/bcaee6a9d587 https://hg.mozilla.org/mozilla-central/rev/1287ce64f050