Gene Lian (I already quit Mozilla) Reporter
	Description • 11 years ago

This issue comes into my mind again when I touched the system message codes recently. Personally, I think it's a very critical issue although there is no bug addressed for it at this moment. Let me explain this issue with a practical example. The Camera App registers the following activities as "activity"-type system messages:

  "activities": {
    "record": {
      ...
    },
    "pick": {
      ...
    }
  },

During the app registration at start-up, the system message (wrongly) creates 2 separate queues to maintain the pending messages:

  queue-1: []
  queue-2: []

When a "record" activity fires, the queues become:

  queue-1: ["record"]
  queue-2: ["record"]

After the starting app fetches the pending "activity"-type messages from the parent, the queues become:

  queue-1: []
  queue-2: ["record"]

because the parent only returns the first queue matched by the {manifest URL + page URL + "activity"-type}. Supposing another "pick" activity fires, the queues become:

  queue-1: ["pick"]
  queue-2: ["record", "pick"]

After the app fetches the pending messages, the queue become:

  queue-1: []
  queue-2: ["record", "pick"]

You can see the queue-2 is really redundant and keeps growing to explode the memory.

	Gene Lian (I already quit Mozilla) Reporter
	Comment 1 • 11 years ago

Hi Fabrice,

My original solution is very straight forward: why not just creating single queue for all the pending "activity"-type messages (no matter it's for "pick", "record" or blah blah)?

You used to say we might run into some race condition issues if we don't consider the activities as different queues. However, I still don't understand who's going to compete with who?

No matter what the reason is, the system message currently behaves as assuming only one queue exists (other queues are redundant and useless). I'd suggest let's directly fix it (that is, using one queue only for "activity"-type messages) before the purpose of multiple activity queues is clear.

What do you think?

	Gene Lian (I already quit Mozilla) Reporter
	Comment 2 • 11 years ago

There used to be a mailing-list thread for this at [1] but nothing is in agreement yet.

[1] https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/wBVJdotpx0c

	[:fabrice] Fabrice Desré Assignee
	Comment 3 • 11 years ago

Indeed we register duplicate system messages (one per activity) at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#601. We need at least to fix that.

	[:fabrice] Fabrice Desré Assignee
	Comment 4 • 11 years ago

This patch prevents the registration of redundant message queues. Tested locally I could not find anything breaking with this change.

	Gene Lian (I already quit Mozilla) Reporter
	Comment 5 • 11 years ago

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

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

Sweet! That's exactly what I expect! This fix wouldn't be worse than the original logic. The original mechanism creates multiple queues but always uses them as single one, so I believe the fix is backward-compatible and makes us avoid potential bugs.

Regarding to the change within .registerPage(...). I think we can also remove the following logic both in the .sendMessage(...) and broadcastMessage(...), which used to be a work-around solution:

  // Open app pages to handle their pending messages.
  // Note that we only need to open each app page once.
  let key = this._createKeyForPage(aPage);
  if (!pagesToOpen.hasOwnProperty(key)) {
    this._openAppPage(aPage, aMessage);
      pagesToOpen[key] = true;
  }

and just leave:

  this._openAppPage(aPage, aMessage);

since the {"type", "manifest", "uri"} is 100% unique now. ;)

::: dom/messages/SystemMessageInternal.js
@@ +224,5 @@
>        throw Cr.NS_ERROR_INVALID_ARG;
>      }
>  
> +    let uri = aPageURI.spec;
> +    let manifestURI = aManifestURI.spec;

I'd prefer using:

s/uri/pageURL
s/manifestURI/manifestURL

where the xxxURL stands for the string and the xxxURI stands for the nsIURI. Also, pageURL specifically stands for the URL of *page* and the manifestURL specifically stands for the URL of *manifest* file.

I understand we don't have a unified convention within this file, but we may start from now on.

@@ +236,5 @@
> +        debug("Ignoring duplicate registration of " +
> +              [aType, uri, manifestURI]);
> +        return;
> +      }
> +    }

Please use this._isPageMatched(...).

	[:fabrice] Fabrice Desré Assignee
	Comment 6 • 11 years ago

Addressing comments.

	Gene Lian (I already quit Mozilla) Reporter
	Comment 7 • 11 years ago

Comment on attachment 756245 [details] [diff] [review]
patch v2

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

Great! I believe the fix is working well! However, I think lots of common codes for finding the page can be shared and some redundant loops are not really necessary. That would be wonderful if they can all be taken into consideration in this patch.

::: dom/messages/SystemMessageInternal.js
@@ +160,1 @@
>      this._pages.forEach(function(aPage) {

For .sendMessage(...), could you please use |this._pages.some(...)| instead of |this._pages.forEach(...)|? I believe it's worth doing this to avoid redundant loops after the page is matched, thus improving the overall performance. For .broadcastMessage(...), we definitely need to use |this._pages.forEach(...)|.

Also, please do the same trick for the case of "SMM:Message:Return:OK". I believe |.some(...)| should be right logic and the performance can be thus improved.

Finally, that would be a bonus if we can refactorize the following common part into one helper function, which can be shared by lots of logic, like "SMM:GetPendingMessages", "SMM:HasPendingMessages" and "SMM:Message:Return:OK":

_findPage: function _findPage(aType, aPageURL, aManifestURL) {
  let page = null;
  this._pages.some(function(aPage) {
    if (this._isPageMatched(aPage, aType, aPageURL, aManifestURL)) {
      page = aPage;
    }
    return page !== null;
  }, this);
  return page;
}

@@ -165,5 @@
>  
>        // Queue this message in the corresponding pages.
>        this._queueMessage(aPage, aMessage, messageID);
>  
> -      // Open app pages to handle their pending messages.

I think it's worth leaving the original comment:

// Open app pages to handle their pending messages.
this._openAppPage(aPage, aMessage);

@@ -207,5 @@
>  
>          // Queue this message in the corresponding pages.
>          this._queueMessage(aPage, aMessage, messageID);
>  
> -        // Open app pages to handle their pending messages.

I think it's worth leaving the original comment:

// Open app pages to handle their pending messages.
this._openAppPage(aPage, aMessage);

@@ +220,5 @@
> +        debug("Ignoring duplicate registration of " +
> +              [aType, pageURL, manifestURL]);
> +        return;
> +      }
> +    }

This logic is good! But the best is you can even use that refactorized helper function I mentioned above to find the target page:

let page = this._findPage(aType, pageURL, manifestURL);
if (page) {
  debug("...");
  return;
}

	[:fabrice] Fabrice Desré Assignee
	Comment 8 • 11 years ago

Re-tested, and didn't find any regression. We should still let that bake a bit on m-c before asking for uplift on b2g18.

	Gene Lian (I already quit Mozilla) Reporter
	Comment 9 • 11 years ago

I hope to review and land Bug 878395 first (a follow-up of bug 877627) to avoid code merge conflicts. Fabrice, could you please review that? Thanks!

	Gene Lian (I already quit Mozilla) Reporter
	Comment 10 • 11 years ago

Comment on attachment 756798 [details] [diff] [review]
patch v3

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

Sorry, review- because there is one line obviously wrong.

Also, could you please use ._findPage(...) for the following three parts as well?

  - "SMM:GetPendingMessages"
  - "SMM:HasPendingMessages"
  - .registerPage(...) {...}

You only use it in "SMM:Message:Return:OK". However, lots of codes can be perfectly shared.

::: dom/messages/SystemMessageInternal.js
@@ +222,5 @@
>  
> +    let pageURL = aPageURI.spec;
> +    let manifestURL = aManifestURI.spec;
> +
> +    // Don't register duplicates for this tuple.

As mentioned above, I think you can also use ._findPage(...) here:

let page = this._findPage(aType, pageURL, manifestURL);
if (page) {
  debug("Ignoring duplicate registration of " +
        [aType, pageURL, manifestURL]);
  return;
}

@@ +431,1 @@
>            let pendingMessages = aPage.pendingMessages;

s/aPage/page.

The above line is obviously wrong and that's why review-.

	Gene Lian (I already quit Mozilla) Reporter
	Comment 11 • 11 years ago

Ping Fabrice? :) The blocked bugs need to maintain more stable queues since they might want to iterate through the queues to do something. I'm happy to take over the remaining tasks if you're not available recently. The current patch seems very close to be done.

	Gene Lian (I already quit Mozilla) Reporter
	Comment 13 • 11 years ago

Comment on attachment 762958 [details] [diff] [review]
patch v4

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

Nice patch! Thanks Fabrice! Please go ahead to land this.

::: dom/messages/SystemMessageInternal.js
@@ +225,5 @@
> +
> +    // Don't register duplicates for this tuple.
> +    let page = this._findPage(aType, pageURL, manifestURL);
> +    if (page) {
> +      dump("Ignoring duplicate registration of " +

Any reason why using dump(...) here instead of debug(...)?

@@ +226,5 @@
> +    // Don't register duplicates for this tuple.
> +    let page = this._findPage(aType, pageURL, manifestURL);
> +    if (page) {
> +      dump("Ignoring duplicate registration of " +
> +              [aType, pageURL, manifestURL]);

Nit: please align this line with the |"| in the above line.

	[:fabrice] Fabrice Desré Assignee
	Comment 14 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/5fcd3e7dc809

	Ed Morley [:emorley]
	Comment 15 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5fcd3e7dc809