928535 - Support WeakRef's from c++ to JSImplemented webidl objects

Status: RESOLVED FIXED

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

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 1

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

Attachment: wip

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

Comment 2

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

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

Comment 3

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

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

Comment 4

[Andrew McCreight [:mccr8]]

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?

Comment 5

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

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.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

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

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

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

It was a general design question.

Weakref to #3 makes sense if someone else is keeping it alive, in general....

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 14

[Andrew McCreight [:mccr8]]

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

Comment 15

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

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.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba49eed0460

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ba49eed0460

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Any reason this patch wont work on Aurora?

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c61b077e100a