764263 - Use the frame message manager in Identity DOM instead of pmm

Status: RESOLVED FIXED

(Core Graveyard :: Identity, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v1

We need to use the frame message manager instead of the parent/child process message manager in order to properly support b2g.

This patch should do it, but I've yet to test it.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v2

Hi Justin, this patch implements the frame message manager usage in the Identity code, hopefully the way we discussed it a few weeks ago.

nsDOMIdentity.js is the DOM binding in the child (initialized through nsDOMGlobalPropertyInitializer), and DOMIdentity.jsm is the corresponding "manager" in the parent.  The fmm now substitutes the use of cpmm/ppmm in this code.

Asking review from you to see if this is how you were expecting it to be set up, and also because you've probably been using the mm a lot these days so you can tell if my usage is correct.

Fabrice, any feedback or review is also welcome.

Some manual testing worked, I'm gonna do some more tests with this before pushing to the pine tree.

Comment 2

[Justin Lebar (not reading bugmail)]

Oops, this slipped out of my inbox.  Looking at it now.

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 632560 [details] [diff] [review]
Patch v2

You should probably get someone who's looked at this code to review it, but I'm happy to provide feedback.

This looks like totally the right idea, but I have enough nits for f-.

>+// Maps the callback objects with the frame message manager
>+// to be used to send the message back to the child.

Maps the callback objects /to/ the fmm.

>+let mm = {
>+  targets: new WeakMap(),
>+
>+  set: function(ref, fmm) {
>+    this.targets.set(ref, fmm);
>+  },
>+
>+  get: function(ref) {
>+    return this.targets.get(ref);
>+  }
>+}

Why isn't this just |let mm = new WeakMap()|?  Also, it should have a better name; mmMap or something.

>@@ -39,24 +49,24 @@ function IDPProvisioningContext(aID, aOr
> IDPProvisioningContext.prototype = {
>   get id() this._id,
>   get origin() this._origin,
> 
>   doBeginProvisioningCallback: function IDPProvisioningContext_doBeginProvisioningCallback(aID, aCertDuration) {
>     let message = new IDDOMMessage(this.id);
>     message.identity = aID;
>     message.certDuration = aCertDuration;
>-    ppmm.sendAsyncMessage("Identity:IDP:CallBeginProvisioningCallback", message);
>+    mm.get(this).sendAsyncMessage("Identity:IDP:CallBeginProvisioningCallback", message);
>   },

Why not store a ref to the mm on the IDProvisioningContext object, instead of
using the weakmap?  (Same for other objects.)

> let DOMIdentity = {
>   // nsIFrameMessageListener
>   receiveMessage: function(aMessage) {
>     let msg = aMessage.json;
>+
>+    // the origin frame message manager
>+    // to be used to send replies back
>+    let target = aMessage.target
>+                         .QueryInterface(Ci.nsIFrameLoaderOwner)
>+                         .frameLoader.messageManager;

"origin" isn't idiomatic.  Maybe "The message manager which called us."  Also, please linewrap comments appropriately, start sentences with a capital letter, and end sentences with periods.

>   // nsIObserver
>   observe: function(aSubject, aTopic, aData) {
>-    if (aTopic != "xpcom-shutdown") {
>-      return;
>+    switch (aTopic) {
>+      case "domwindowopened":
>+      case "domwindowclosed":
>+        let win = aSubject.QueryInterface(Ci.nsIInterfaceRequestor)
>+                          .getInterface(Ci.nsIDOMWindow);
>+        this._setUpMessages(win, aTopic == "domwindowopened");
>+        break;
>+
>+      case "xpcom-shutdown":
>+        Services.ww.unregisterNotification(this);
>+        Services.obs.removeObserver(this, "xpcom-shutdown");

I don't think you need to unregister your shutdown observer, and I doubt you have to unregister the window watcher observer.

>-  _watch: function(message) {
>+  _setUpMessages: function(aWindow, aRegistering) {
>+    if (!aWindow.messageManager)
>+      return;
>+ 
>+    let func = aRegistering ? "addMessageListener" : "removeMessageListener";
>+
>+    for (let message of this.messages) {
>+      aWindow.messageManager[func](message, this);
>+    }
>+  },

This function needs a better name.  You're setting up or tearing down something, depending on aRegistering.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v3

Thanks for the feedback. I made the changes mentioned. Some comments:

(In reply to Justin Lebar [:jlebar] from comment #3)
> >+let mm = {
> >+  targets: new WeakMap(),
> >+
> >+  set: function(ref, fmm) {
> >+    this.targets.set(ref, fmm);
> >+  },
> >+
> >+  get: function(ref) {
> >+    return this.targets.get(ref);
> >+  }
> >+}
> 
> Why isn't this just |let mm = new WeakMap()|?  Also, it should have a better
> name; mmMap or something.

changed to |let mmMap = new WeakMap()|.  I had some extra debugging functions on this object but ended up reducing it to just a WeakMap.



> >-    ppmm.sendAsyncMessage("Identity:IDP:CallBeginProvisioningCallback", message);
> >+    mm.get(this).sendAsyncMessage("Identity:IDP:CallBeginProvisioningCallback", message);
> >   },
> 
> Why not store a ref to the mm on the IDProvisioningContext object, instead of
> using the weakmap?  (Same for other objects.)

These objects get passed around a bit and I've had trouble in the past holding refs to the message manager so I'd rather use weak refs. I wanted it to stay idiomatically similar to ppmm.send* so I was using mm.get(this).
But I've improved a bit on this version by adding a getter in each object which just access the weakMap, so we can simply do this.mm.send*. Also I set the mapping in the constructors instead of the caller, seems much better now.



> >+      case "xpcom-shutdown":
> >+        Services.ww.unregisterNotification(this);
> >+        Services.obs.removeObserver(this, "xpcom-shutdown");
> 
> I don't think you need to unregister your shutdown observer, and I doubt you
> have to unregister the window watcher observer.

many users of xpcom-shutdown that I see in the tree unregister it. I'll leave it in for now and figure out later if I can remove it.



Instead of getting a review for this patch independently we'll include it as part of the patch that implements the DOM binding  (as it would not make much sense to let someone review the cpmm code that this patch removes).

Posting the updated patch. Matt, feel free to apply it locally for you and push to pine if everything still works

Comment 5

[Matthew N. [:MattN]]

Pushed to pine: https://hg.mozilla.org/projects/pine/rev/dfd8fe4d5abf

Comment 6

[Justin Lebar (not reading bugmail)]

> These objects get passed around a bit and I've had trouble in the past holding refs to the message 
> manager so I'd rather use weak refs.

Although the code is cleaner now (thanks!), I really don't get why this is necessary.  You're storing a variable on an object.  Just store it on the object.  If that caused problems, it would almost certainly be a platform bug which we'd want to investigate.

Comment 7

[Matthew N. [:MattN]]

This patch has been included in attachment 634291 [details] [diff] [review].  jlebar, feel free to give review comments on that patch.

Felipe, can you address comment 6?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

I like weak maps, less work for the gc :) Unless Justin really wants me to just store the mm in the object, I'd prefer not to change it :P

Comment 9

[Andrew McCreight [:mccr8]]

Make sure you are only using nodes as keys in the weak map.  If you aren't, you'll get a pretty incomprehensible message in the error console, but it may still appear to work...

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Justin Lebar [:jlebar] from comment #6)
> Just store it on the object.  If that caused problems, it would almost certainly
> be a platform bug which we'd want to investigate.

Expandos only work on wrapper cached objects, or something like that.  That could be a problem.

Comment 11

[Justin Lebar (not reading bugmail)]

The object that we're putting the expando /on/ is entirely in JS, in this case, although that could of course explain what Felipe saw before.  There's no limitation on what can be stored /in/ an expando, right?

> I like weak maps, less work for the gc :)

I think this is a premature optimization.  But moreover I think it's a misconception -- if the key is alive, the GC has to trace the value in the weakmap.  It would be a correctness error if weakmaps produced less for the GC to trace in this case than in the case when we store the mm on the object.

In general, we should write straightforward code, free of premature optimizations.  I don't think this should get a pass, but it's not my call, and it's not the end of the world either way.

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Justin Lebar [:jlebar] from comment #11)
> The object that we're putting the expando /on/ is entirely in JS, in this
> case, although that could of course explain what Felipe saw before.  There's
> no limitation on what can be stored /in/ an expando, right?

Oh, okay.  Never mind then!  You should be okay for both an expando and a weak map key, for just a pure JS object.

> > I like weak maps, less work for the gc :)
> 
> I think this is a premature optimization.  But moreover I think it's a
> misconception -- if the key is alive, the GC has to trace the value in the
> weakmap.  It would be a correctness error if weakmaps produced less for the
> GC to trace in this case than in the case when we store the mm on the object.

Yeah, weak maps are kind of a pain for the GC.  They have to be dealt with in a special separate phase.  They are mainly useful when you might want to stick data on something, so the data will go away when the object goes away, but you can't for some reason.  If you can just put things on the object, that might work better.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

(In reply to Justin Lebar [:jlebar] from comment #11)
> The object that we're putting the expando /on/ is entirely in JS, in this
> case, although that could of course explain what Felipe saw before.  There's
> no limitation on what can be stored /in/ an expando, right?

Ok, if that is a reasonable explanation I'm gonna give a try in storing it straight in the object and see what happens.

> 
> > I like weak maps, less work for the gc :)
> 
> I think this is a premature optimization.  But moreover I think it's a
> misconception -- if the key is alive, the GC has to trace the value in the
> weakmap.  It would be a correctness error if weakmaps produced less for the
> GC to trace in this case than in the case when we store the mm on the object.

Being more precise, I think I meant to say CC, not GC.  TBH I'd rather see us doing more optimizations where we can instead of letting the collectors do all the hard work for us. (and weak maps give me a peace of mind that storing a ref to a window doesn't). But if I test the more direct approach and am able to tell that it won't cause any weird leaks, I'll go with it.

Comment 14

[Justin Lebar (not reading bugmail)]

> (and weak maps give me a peace of mind that storing a ref to a window doesn't)

I think you may be confused about how weakmaps work.  The /keys/ to the weakmap are weak refs.  But the values are strong refs.

So there is no difference, in terms of what you're storing a strong reference to, between the weakmap approach and the expando approach.  Both approaches store a strong reference to the message manager.  In both approaches, the strong reference to the mm goes away after the object (the key to the weakmap) is unreachable.

Comment 15

[Matthew N. [:MattN]]

These changes were incorporated in pine[1] and attachment 635100 [details] [diff] [review].

[1] https://hg.mozilla.org/projects/pine/rev/05ef1e5f5b1e

Comment 16

[:Felipe Gomes (needinfo for replies!)]

In the push above we now store the mm directly in the object, addressing Justin's comment

Comment 17

[Matthew N. [:MattN]]

Pushed as part of: 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a335f5f3e103
https://hg.mozilla.org/mozilla-central/rev/a335f5f3e103