793361 - Apps, SystemMessages: free message manager references for killed children

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

Currently we remove observers during the iteration phase. That causes bad bugs.

Comment 1

[Gregor Wagner [:gwagner]]

Ok the removal is not the problem.
Calling cpmm.sendAsyncMessage is the problem.
Some dumps show me that we never return from the call and we also don't throw an exception.

For example this uninit never returns:

  uninit: function() {
    this._mgmt = null;
    cpmm.sendAsyncMessage("Webapps:UnregisterForMessages",
                          ["Webapps:Install:Return:OK"]);
  },

Comment 2

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

Testcase, please

Comment 3

[Gregor Wagner [:gwagner]]

(In reply to Olli Pettay [:smaug] from comment #2)
> Testcase, please

I will try to reproduce it on a desktop build.

Comment 4

[Gregor Wagner [:gwagner]]

It seems we are closing the process.

cjones on irc:
that can happen if something tries to send an IPC message after the pipe is closed

We want to send a message to the parent and hit quickExit which does _exit(0) :(

The best solution is to fix bug 777508.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If I understood what was being proposed on IRC, it wouldn't work in general, even if we didn't QuickExit() during shutdown, so we need a different approach.

Comment 6

[Gregor Wagner [:gwagner]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> If I understood what was being proposed on IRC, it wouldn't work in general,
> even if we didn't QuickExit() during shutdown, so we need a different
> approach.

We should just remove all the unregistering via sendAsyncMessage and keep enough information in the parent that we can do it with a child-shutdown notification.

Otherwise we would need a notification when we can still send messages.

Comment 7

[Andrew Overholt [:overholt]]

Gregor, are you investigating?

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Andrew Overholt [:overholt] from comment #7)
> Gregor, are you investigating?

Yes we know what's going on. We need bug 777508.

Comment 9

[Philipp von Weitershausen [:philikon]]

Going to morph this bug to deal with the Apps DOM API, morphing bug 794353 to deal with SystemMessages. Dupe or whatever if these things are going to be handled in the same bug. (I smell a helper.)

Comment 10

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 11

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665723 [details] [diff] [review]
patch

Review of attachment 665723 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by...

::: dom/apps/src/Webapps.jsm
@@ +423,5 @@
>  
>        let index;
> +      if ((index = msg.indexOf(aMm)) != -1) {
> +        debug("Remove dead mm at index " + index);
> +        msg.children.splice(index, 1);

I think you want

  s/msg.children/this.children/

Did you test this code? :)

@@ +433,5 @@
>      // nsIPrefBranch throws if pref does not exist, faster to simply write
>      // the pref instead of first checking if it is false.
>      Services.prefs.setBoolPref("dom.mozApps.used", true);
>  
> +    let msg = aMessage.json || {};

Nit: aMessage.json is being deprecated. Use aMessage.data.

::: dom/messages/SystemMessageInternal.js
@@ +49,5 @@
>  SystemMessageInternal.prototype = {
>    sendMessage: function sendMessage(aType, aMessage, aPageURI, aManifestURI) {
>      debug("Broadcasting " + aType + " " + JSON.stringify(aMessage));
>      if (this._listeners[aManifestURI.spec]) {
> +      for (let i in this._listeners[aManifestURI.spec]) {

Do you mean a `for ... of` loop here (the new way to write `for each ... in`). Again, seems like this code is untested.

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665723 [details] [diff] [review]
patch

>+  removeMessageListener: function(aMm) {
>+    for (let i = this.children.length - 1; i >= 0; i -= 1) {
>+      msg = this.children[i];
> 
>       let index;
>-      if ((index = this.children[aMsgName].indexOf(aMm)) != -1) {
>-        this.children[aMsgName].splice(index, 1);
>+      if ((index = msg.indexOf(aMm)) != -1) {
>+        debug("Remove dead mm at index " + index);
>+        msg.children.splice(index, 1);

Actually, I just realized, why not simply

  let index = this.children.indexOf(aMm);
  if (index == -1) {
    return;
  }
  this.children.splice(index, 1);

Comment 13

[Gregor Wagner [:gwagner]]

(In reply to Philipp von Weitershausen [:philikon] from comment #12)
> Comment on attachment 665723 [details] [diff] [review]
> patch
> 
> >+  removeMessageListener: function(aMm) {
> >+    for (let i = this.children.length - 1; i >= 0; i -= 1) {
> >+      msg = this.children[i];
> > 
> >       let index;
> >-      if ((index = this.children[aMsgName].indexOf(aMm)) != -1) {
> >-        this.children[aMsgName].splice(index, 1);
> >+      if ((index = msg.indexOf(aMm)) != -1) {
> >+        debug("Remove dead mm at index " + index);
> >+        msg.children.splice(index, 1);
> 
> Actually, I just realized, why not simply
> 
>   let index = this.children.indexOf(aMm);
>   if (index == -1) {
>     return;
>   }
>   this.children.splice(index, 1);

We have an array of arrays here. children[msgName] stores the mm

Comment 14

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 16

[Gregor Wagner [:gwagner]]

Comment on attachment 666150 [details] [diff] [review]
patch

try run: https://hg.mozilla.org/try/rev/3719ecc88cab

Comment 17

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3bec3b18597

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e3bec3b18597

Should this have a test?