Last Comment Bug 764263 - Use the frame message manager in Identity DOM instead of pmm
: Use the frame message manager in Identity DOM instead of pmm
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Identity (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: :Felipe Gomes (needinfo me!)
:
: Fernando Jiménez Moreno [:ferjm]
Mentors:
Depends on:
Blocks: 753239
  Show dependency treegraph
 
Reported: 2012-06-12 21:52 PDT by :Felipe Gomes (needinfo me!)
Modified: 2012-07-11 14:24 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (16.59 KB, patch)
2012-06-12 21:52 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Patch v2 (16.63 KB, patch)
2012-06-12 23:05 PDT, :Felipe Gomes (needinfo me!)
justin.lebar+bug: feedback-
Details | Diff | Splinter Review
Patch v3 (17.14 KB, patch)
2012-06-13 16:53 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review

Description :Felipe Gomes (needinfo me!) 2012-06-12 21:52:56 PDT
Created attachment 632543 [details] [diff] [review]
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 me!) 2012-06-12 23:05:17 PDT
Created attachment 632560 [details] [diff] [review]
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) 2012-06-13 13:50:21 PDT
Oops, this slipped out of my inbox.  Looking at it now.
Comment 3 Justin Lebar (not reading bugmail) 2012-06-13 14:20:22 PDT
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 me!) 2012-06-13 16:53:52 PDT
Created attachment 632962 [details] [diff] [review]
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] (PM me if requests are blocking you) 2012-06-14 00:03:52 PDT
Pushed to pine: https://hg.mozilla.org/projects/pine/rev/dfd8fe4d5abf
Comment 6 Justin Lebar (not reading bugmail) 2012-06-14 07:02:05 PDT
> 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] (PM me if requests are blocking you) 2012-06-18 21:36:20 PDT
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 me!) 2012-06-19 13:56:40 PDT
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] 2012-06-19 14:01:44 PDT
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] 2012-06-19 14:02:37 PDT
(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) 2012-06-19 14:49:29 PDT
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] 2012-06-19 14:59:02 PDT
(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 me!) 2012-06-19 15:05:22 PDT
(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) 2012-06-19 15:09:34 PDT
> (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] (PM me if requests are blocking you) 2012-06-20 15:43:51 PDT
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 me!) 2012-06-20 15:50:47 PDT
In the push above we now store the mm directly in the object, addressing Justin's comment
Comment 17 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-07-11 14:24:10 PDT
Pushed as part of: 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a335f5f3e103
https://hg.mozilla.org/mozilla-central/rev/a335f5f3e103

Note You need to log in before you can comment on or make changes to this bug.