WeakRefs from JS to JS seems to "just work". Not so for WeakPtr references from c++ to JSImplemented webidl objects. Right now, I can specify this in my JSImplemented object, to no effect: QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports, Ci.nsISupportsWeakReference]), because it doesn't affect the BindingCode. Even if I could get to this interface, going from the weak-ref to the implementation to the top webidl object is not possible. Example where this is needed: mozRTCPeerConnection (in JS webidl) is a front-end which passes a PeerConnectionObserver (in JS webidl) into the PeerConnectionImpl member (in c++ webidl) implementation (which is not cycle-collected because multiple threads access it). After dispatching to other threads and back (which isn't relevant) it uses a WeakPtr reference to the observer to call callbacks on it with results. Right now, the c++ WeakPtr cannot point directly to the PeerConnectionObserver, which is what it needs to do in order to call the callbacks. That code is currently using a workaround class called WeakConcretePtr, which holds a direct pointer *and* a WeakPtr to a pure JS member instead as a guard. It checks that the member exists before using the direct pointer. FYI.

Doesn't handle the case when interface extends some other interface.

Comment on attachment 819204 [details] [diff] [review] wip Andrew, what you think of this? Supporting weakref should be rather simple this way. (need to still figure out the inheritance case.)

Perhaps we should support weakref only if the interface is annotated to support it?

Wait, you are manipulating a JS-implemented WebIDL object from C++? That seems weird to me. I thought it was only supposed to be used from JS?

Why would it be? If JS implemented webidl object is passed to a C++ implemented Webidl object, sure you can take a ref to it. and that ref could be weak.

(In reply to Olli Pettay [:smaug] from comment #3) > Perhaps we should support weakref only if the interface is annotated to > support it? Is there a cost or downside? Since JS to JS "just works"...

Keep in mind that there are 4 separate objects here: 1) The JSObject implementing the interface. 2) The CallbackObject subclass that keeps that JSObject alive. 3) The C++ object implementing the WebIDL interface. 4) The content-side JS wrapper for the C++ object. Which one are you trying to hold a weak reference to? I assume #3. But if you don't actually hold a ref to object #3, it'll just die. But if you hold a ref to it, it keeps object #2 alive, which holds object #1 alive. Also, what is the semantic meaning of this weak reference? "Keep objects 3 and 2 alive as long as object 1 is alive"? But again, object 2 keeps object 1 alive, so object 1 can't die as long as object 2 is alive... So what does "weak reference" mean here?

Weakref means what that wip does. On C++ side one shouldn't need to care about whether some binding is implemented in C++ or JS, there is anyway the C++ layer there. So weak reference means xpcom weakref pointing to (3).

(In reply to On vacation Oct 12 - Oct 27 from comment #7) > So what does "weak reference" mean here? Not sure whether this is an implementation question for Olli or an intent question for me. Feel free tolook at my current workaround in bug 928221 for context if that helps (also should you find any problems with that workaround, feel free to let me know - it seems to work though I didn't know there were four objects ;-) )

It was a general design question. Weakref to #3 makes sense if someone else is keeping it alive, in general....

Comment on attachment 819204 [details] [diff] [review] wip This looks reasonable to me, generally speaking. I think we would want it to be opt-in, though, as I think there's some overhead to supporting weak refs. Though maybe it isn't enough to worry about in the context of the monstrous hybrid JS implemented WebIDL object. I also share bz's questions about the design or whatever here. I guess I need to find some time to dig through the workaround code.

(In reply to Andrew McCreight [:mccr8] from comment #11) > Comment on attachment 819204 [details] [diff] [review] > wip I've tested it and it works. Thanks! > I think we would want it > to be opt-in, though, as I think there's some overhead to supporting weak > refs. Though maybe it isn't enough to worry about in the context of the > monstrous hybrid JS implemented WebIDL object. Yeah, I think that dwarfs anything added by this.

What's the timeframe on this? Bug 933447 depends on it and probably needs uplift to Aurora. Everything works at the moment with this AFAICT.

Olli wrote the patch, he's a better person to ask.

Comment on attachment 819204 [details] [diff] [review] wip Actually, I don't want to add SupportsWeakReference in case the class is inheriting some base class. The base class should inherit SupportsWeakReference. This way we don't accidentally have cases when we inherit SupportsWeakReference twice.

Any reason this patch wont work on Aurora?

Comment on attachment 819204 [details] [diff] [review] wip [Approval Request Comment] Bug caused by (feature/regressing bug #): So we can land dependent bugs User impact if declined: Testing completed (on m-c, etc.): Tested on m-c. Risk to taking this patch (and alternatives if risky): Low. New codegen feature is unused by default. String or IDL/UUID changes made by this patch: None