Closed Bug 793361 Opened 8 years ago Closed 8 years ago

Apps, SystemMessages: free message manager references for killed children

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: gwagner, Assigned: gwagner)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently we remove observers during the iteration phase. That causes bad bugs.
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"]);
  },
Summary: Don't do removeObserver during the notify phase → cpmm.sendAsyncMessage in "inner-window-destroyed" observer doesn't work
blocking-basecamp: --- → ?
Testcase, please
(In reply to Olli Pettay [:smaug] from comment #2)
> Testcase, please

I will try to reproduce it on a desktop build.
Blocks: 792846
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.
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.
(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.
blocking-basecamp: ? → +
Depends on: 777508
Gregor, are you investigating?
Assignee: nobody → anygregor
(In reply to Andrew Overholt [:overholt] from comment #7)
> Gregor, are you investigating?

Yes we know what's going on. We need bug 777508.
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.)
Summary: cpmm.sendAsyncMessage in "inner-window-destroyed" observer doesn't work → Apps: free message manager references for killed children
Attached patch patch (obsolete) — Splinter Review
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 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);
(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
Attached patch patchSplinter Review
Attachment #665723 - Attachment is obsolete: true
Summary: Apps: free message manager references for killed children → Apps, SystemMessages: free message manager references for killed children
Duplicate of this bug: 794353
Attachment #666150 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/e3bec3b18597

Should this have a test?
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.