We are randomly experimenting an issue where components "inheriting" from DOMRequestIPCHelper are not properly receiving expected messages coming from the parent. We are seeing this behavior in bug 893800 and bug 876397. Applying the patches at bug 876397, enabling debug for WebApps [1][2] and running the chrome test included in the patches in a loop is a good way to reproduce it. The message sent from [3] is never received at [4]. Sorry for the vague description, but it is quite random. We have a slight suspicious about bug 900221 or even bug 889984 being the reason for this random failure. [1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#34 [2] Add a dump at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#558 [3] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1985 [4] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#558

bug 900221 and bug 889984 are indeed suspicious, especially the latter one. Does anything keep strong reference to WebappsApplication objects or do they just die at some point when CC and GC have run? I'm not familiar with this code (I know about MessageManagers, not about the code using them ;) ), but what for example in the patch for Bug 889984 ensures that stuff is kept alive long enough? Adding some debug code here https://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm?rev=07dafead9048&mark=75-75,78-78,80-80#75 might be useful. Do we end up calling the function at all, and can it get the weak helper or does it just call destroy?

(copy my comment at bug 900221 to here) I'm looking into the DOMRequestHelper.jsm. It looks very weird to me that DOMRequestIpcHelperMessageListener holds just weak reference to DOMRequestIpcHelper. Supposing an (DOMApplication) object inherits from DOMRequestIpcHelper and it can be garbage collected at any time when no JS variables refers to it on the content site, then DOMRequestIpcHelperMessageListener cannot pass its received message to the DOMRequestIpcHelper object because the DOMRequestIpcHelper object might already be released at this moment. Taking IAC Message Port (which I'm working on) for example, the port object should always live with the window so that it can be kept alive to do messaging.

I added a debug trace at https://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm?rev=07dafead9048&mark=75-75,78-78,80-80#75 as requested. The line added was: dump("+*+*+*: DRH: " + helper + "MSG: " + aMsg.name + ", " + aMsg.json.type + "\n"); From what I can see, when it fails, the receiveMessage function isn't called at all. On a run that goes well (the message is received) I get something as: 0:05.98 -*-*- Webapps.jsm : Saving /tmp/tmpGpKMiU/webapps/webapps.json 0:05.99 +*+*+*: DRH: [object Object]MSG: Webapps:Install:Return:OK, undefined 0:05.99 +*+*+*: DRH: [object Object]MSG: Webapps:Install:Return:OK, undefined 0:06.00 6 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/apps/tests/test_operator_app_install.xul | Got oninstall event 0:06.00 -*-*- Webapps.jsm : updateAppHandlers: old=null new=[object Object] 0:06.00 -*-*- Webapps.jsm : Saving /tmp/tmpGpKMiU/webapps/webapps.json 0:06.00 -*-*- Webapps.jsm : About to fire Webapps:PackageEvent 'installed' 0:06.00 +*+*+*: DRH: [object Object]MSG: Webapps:PackageEvent, installed On a run that fails, I get something like this instead: 0:05.83 -*-*- Webapps.jsm : Saving /tmp/tmpemc0A1/webapps/webapps.json 0:05.86 -*-*- Webapps.jsm : updateAppHandlers: old=null new=[object Object] 0:05.86 -*-*- Webapps.jsm : Saving /tmp/tmpemc0A1/webapps/webapps.json 0:05.86 -*-*- Webapps.jsm : About to fire Webapps:PackageEvent 'installed' 0:05.86 +*+*+*: DRH: [object Object]MSG: Webapps:Install:Return:OK, undefined 0:05.86 +*+*+*: DRH: [object Object]MSG: Webapps:Install:Return:OK, undefined 0:05.86 6 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/apps/tests/test_operator_app_install.xul | Got oninstall event 5:04.61 7 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/apps/tests/test_operator_app_install.xul | Test timed out. As you can see, I get the two Webapps:Install events, but I don't get the Webapps:PackageEvent event on the second case. I don't think it's a GC problem in my case, since what I doing is running the mochitest on a while true loop while true do; ./mach mochitest-chrome dom/apps/test/test_operator_app.xul; echo "-----NEXT RUN----" ; done And the browser just installs one application. Plus, the Webapps.js object is alive and well, since I get the 'got oninstall event' correctly. The PackageEvent message just goes AWOL.

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #3) > As you can see, I get the two Webapps:Install events, but I don't get the > Webapps:PackageEvent event on the second case. I don't think it's a GC > problem in my case, since what I doing is running the mochitest on a while > true loop GC may run pretty much at any point when JS runs, and Cycle Collector whenever event loop spins. So for example right before a message is received. > And the browser just installs one application. Plus, the Webapps.js object > is alive and well, since I get the 'got oninstall event' correctly. The > PackageEvent message just goes AWOL. Hmm.

If you want to make this more reproducible, adding calls to SpecialPowers.gc(); to the mochitest may help, as that will force a GC and CC.

I also asked to check whether https://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm?rev=07dafead9048&mark=84-102#84 is called at some point. And if not, I'll try to reproduce (after figuring out what all patches are needed)

For my problem, after adding more traces, the problem is not really caused by the MessageManager. Instead is a race condition where the parent/message emitter sometimes ends all its work before the child/message receiver has time to register itself as a message receiver. So the message isn't received because it isn't actually send.

[17:46] amac smaug: Nope, what's happening is a Webapps problem. The parent process sends a message to the child telling it that there's a new app installed. Then the child process creates a local app object which register itself as a listener for some events [17:46] amac smaug: and the problem is that the events are actually being sent before the child process has time to create the local app object and register the handlers, so the events are not sent (because there's no listener for them) ... [17:48] amac smaug: Yep, but the Webapps code checks if it has any object registered for that and doesn't send it. My problem isn't a MessageManager problem... So is there anything to fix here? Comment 0 talks about WebApps as well, so is this just a bug in WebApps.js[m] or in the code using it?

I need some feedback here. Is there anything to fix in MessageManager? Bug 876397 landed to m-i, at least partially, so I assume this doesn't block that one.

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #7) > For my problem, after adding more traces, the problem is not really caused > by the MessageManager. Instead is a race condition where the parent/message > emitter sometimes ends all its work before the child/message receiver has > time to register itself as a message receiver. So the message isn't received > because it isn't actually send. So it sounds like the the race condition issue has been there at the very first beginning. The recent changes in the DOMRequestHelper.jsm make the initializing process taking longer, thus this racy issue shows up? (In reply to Olli Pettay [:smaug] from comment #9) > I need some feedback here. Is there anything to fix in MessageManager? > Bug 876397 landed to m-i, at least partially, so I assume this doesn't block > that one. Yes. After some thoughts and having discussion with Fernando, the InterAppMessagePort is actually due to another issue. We should use strong reference instead of weak reference for the port object to register its message listeners, because the port had better to live with the window to keep posting/receiving messages. It shouldn't be garbage-collected at any moments.

Well, make sure it is GCed at some point and not kept alive forever ;)

(In reply to Olli Pettay [:smaug] from comment #9) > I need some feedback here. Is there anything to fix in MessageManager? > Bug 876397 landed to m-i, at least partially, so I assume this doesn't block > that one. I don't think the problem is with the MessageManager but with its usage from DOMRequestHelper. As Gene mentioned in comment 10, in some cases we might want to allow strong instead of weak references to message listeners added via DOMRequestIPCHelper. I'm taking this bug to add this modifications to DOMRequestHelper, if that's ok with you.

Just make sure to not regress bug 889984 ;)

This patch does the following: 1. It reverts part of the changes added in bug 889984, specifically the inclusion of the 'DOMRequestIpcHelperMessageListener' object, which is not needed anymore since we've switched too {add,remove}WeakMessageListener. 2. It allows objects inheriting from DOMRequestIPCHelper to add strong listeners. 3. It allows objects inheriting from DOMRequestIPCHelper to add/remove listeners (weaks or not) on demand, not only via 'initDOMRequestHelper'. I'll be creating new bugs for each API where adding/removing listeners for specifics requests is needed, as suggested in https://groups.google.com/d/msg/mozilla.dev.b2g/fRkIJT3J8u8/tjFCG0NIuYEJ Working on tests and waiting for the try build.

This is blocking a potential koi+ issue. Please see bug 915898, comment #3. Nominating for koi+.

The previous try build didn't go well enough, probably because of bug 919429. I'll be trying again with that patch applied. In the mean time I could use some feedback :) Fabrice, if I am not wrong you originally wrote this code and you already reviewed patches for it, so I am setting you as reviewer.

A few more fixes.

Comment on attachment 810593 [details] [diff] [review] v1 Review of attachment 810593 [details] [diff] [review]: ----------------------------------------------------------------- Can you do a try build with these changes and the tests? ::: dom/base/DOMRequestHelper.jsm @@ +32,5 @@ > +this.DOMRequestIpcHelper = function DOMRequestIpcHelper() { > + this._listeners = null; > + this._requests = null; > + this._window = null; > +} nit: add blank line @@ +52,5 @@ > + * where 'name' is the message identifier and 'strongRef' a boolean > + * indicating if the listener should be a strong referred one or not. > + * > + * - or only strings containing the message name, in which case the listener > + * will be added as a weak referred one by default. nit: s/weak reffered one/weak reference @@ +70,2 @@ > > + aMessages.forEach(function(msg) { the new hotness is to do forEach((aMsg) => { }) (here and in other forEach() blocks. @@ +71,5 @@ > + aMessages.forEach(function(msg) { > + let name = msg.name || msg; > + // If the listener is already set and it is of the same type we just > + // bail out. If it is not of the same type, we remove it before adding > + // the new one. Are there valid uses cases for that? Or should we fail hard when trying to add the same message name with a different ownership model? @@ +89,5 @@ > + }, this); > + }, > + > + /** > + * 'aMessages' is expected to be an array of strings containing the message nit: a string or an array of strings... @@ +233,5 @@ > }, > > _getRandomId: function() { > + return Cc["@mozilla.org/uuid-generator;1"] > + .getService(Ci.nsIUUIDGenerator).generateUUID().toString(); nit: align .getService with ["@mozilla...

Thanks Fabrice! This patch addresses your comments.

I've managed to fix a few tests failures with the Webapps modifications as expected, but the try build is still not looking good enough. https://tbpl.mozilla.org/?tree=Try&rev=efccafe02bb5 The dom/apps/tests/test_app_update.html test is throwing an permission error trying to access 'manifest' at [1]: "JavaScript error: http://test/tests/dom/apps/tests/file_app.sjs?apptype=hosted, line 55: Permission denied to access property 'name'" I can to reproduce it locally with the mochitest, but it works with a simple test of the 'getSelf' function. I can access every property in the nsIDOMMozApplication object. Any help would be highly appreciated [1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/file_app.template.html?force=1#55

koi+ as this is blocking a blocker

r=fabrice

Try build looks better now https://tbpl.mozilla.org/?tree=Try&rev=51f9f28b57f6 Still one red to fix though. I'm looking into it.

Comment on attachment 812575 [details] [diff] [review] Part 1: DOMRequestHelper Review of attachment 812575 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/base/DOMRequestHelper.jsm @@ +30,5 @@ > "@mozilla.org/childprocessmessagemanager;1", > "nsIMessageListenerManager"); > > +this.DOMRequestIpcHelper = function DOMRequestIpcHelper() { > + this._listeners = null; Please document what we store in _listeners, since it's not obvious. @@ +106,5 @@ > + return; > + } > + > + this._listeners[aName] ? cpmm.removeMessageListener(aName, this) : > + cpmm.removeWeakMessageListener(aName, this); nit: ':' on the second line @@ +108,5 @@ > + > + this._listeners[aName] ? cpmm.removeMessageListener(aName, this) : > + cpmm.removeWeakMessageListener(aName, this); > + delete this._listeners[aName]; > + }, this); You don't neeed 'this' when using fat arrows functions. @@ +152,5 @@ > + > + if (this._listeners) { > + Object.keys(this._listeners).forEach((aName) => { > + this._listeners[aName] ? cpmm.removeMessageListener(aName, this) : > + cpmm.removeWeakMessageListener(aName, this); nit: ':' on the second line @@ +154,5 @@ > + Object.keys(this._listeners).forEach((aName) => { > + this._listeners[aName] ? cpmm.removeMessageListener(aName, this) : > + cpmm.removeWeakMessageListener(aName, this); > + delete this._listeners[aName]; > + }, this); you don't need the 'this' when using fat arrows functions @@ +232,5 @@ > }, > > _getRandomId: function() { > + return Cc["@mozilla.org/uuid-generator;1"] > + .getService(Ci.nsIUUIDGenerator).generateUUID().toString(); bonus points if we remove the curly braces, but no big deal. @@ +260,2 @@ > } > }, this); remove 'this' @@ +273,2 @@ > } > }, this); idem

Comment on attachment 812577 [details] [diff] [review] Part 3: Webapps Review of attachment 812577 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.js @@ +712,5 @@ > + {name: "Webapps:Uninstall:Return:OK", strongRef: true}, > + {name: "Webapps:Uninstall:Broadcast:Return:OK", strongRef: true}, > + {name: "Webapps:Uninstall:Return:KO", strongRef: true}, > + {name: "Webapps:Install:Return:OK", strongRef: true}, > + {name: "Webapps:GetNotInstalled:Return:OK", strongRef: true}]); nit: for all of these: { name:... strongRef: true }

Try build looks good now https://tbpl.mozilla.org/?tree=Try&rev=272b7d8b7e5d

https://hg.mozilla.org/integration/mozilla-inbound/rev/88cf15fb1be9 https://hg.mozilla.org/integration/mozilla-inbound/rev/2e0d7b9ec205 https://hg.mozilla.org/integration/mozilla-inbound/rev/e59ea92c55fc https://hg.mozilla.org/integration/mozilla-inbound/rev/045ec6585fa7

https://hg.mozilla.org/mozilla-central/rev/88cf15fb1be9 https://hg.mozilla.org/mozilla-central/rev/2e0d7b9ec205 https://hg.mozilla.org/mozilla-central/rev/e59ea92c55fc https://hg.mozilla.org/mozilla-central/rev/045ec6585fa7

https://hg.mozilla.org/releases/mozilla-aurora/rev/1160d240c83a https://hg.mozilla.org/releases/mozilla-aurora/rev/aaa2ff538d5e https://hg.mozilla.org/releases/mozilla-aurora/rev/d3523f3b0bcb https://hg.mozilla.org/releases/mozilla-aurora/rev/7b4da5638a7d

Note, this appears to cause a progressive slow down in b2g app launch times. See bug 924565. This should probably be backed out until we can identify a proper fix.

Hi Ben, Is the back out dependent on any other bug? When can the back out be done?

Preeti, I'm not in a position to answer that question. I think the author of the original patch (ferjm) or the reviewer (fabrice) are better suited to respond. Moving ni? to them.

(In reply to Preeti Raghunath(:Preeti) from comment #38) > Hi Ben, > > Is the back out dependent on any other bug? > Yes. We'll also need to backout Bug 924971, Bug 927398 and Bug 927363 with some merge conflicts involved. So I'd really prefer to find out the reason of the regression instead. > When can the back out be done? I'm waiting for Gabriele's feedback on bug 924565 as he is much more familiar with profile analysis than me.

I think we can't wait more than the end of the week for that being fixed. In any case it's gonna be painful...

(In reply to Fabrice Desré [:fabrice] from comment #41) > I think we can't wait more than the end of the week for that being fixed. In > any case it's gonna be painful... FWIW I think there is no need to backout this since we were able to prove that the regression affects only the tests.