1126119 - sync SystemMessageManager:HasPendingMessages takes too much time (200+ms)

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

+++ This bug was initially created as a clone of Bug #1112989 +++

From bug 1102715 we found that the sync IPC messageSystemMessageManager:HasPendingMessages takes too much time and blocks app startup. Similar to bug 1112989 maybe we could send this information along with LoadURL to child.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

I think HasPendingMessage only guarantees that when it returns true, message handlers will be called as soon as possible asynchronously. When it returns false, message handlers may still be called since messages could be dispatched at any time. I.e. it could be false negative.

We could send first HasPendingMessage state with TabParent::LoadURL to child and asynchronously update live TabParents their HasPendingMessage state. So app could get accurate info at startup and almost accurate info if they want to check that later (I think this use case is rare though).

Comment 2

[[:fabrice] Fabrice Desré]

(In reply to Kan-Ru Chen [:kanru] from comment #1)
> I think HasPendingMessage only guarantees that when it returns true, message
> handlers will be called as soon as possible asynchronously. When it returns
> false, message handlers may still be called since messages could be
> dispatched at any time. I.e. it could be false negative.

HasPendingMessage() is designed to be used before SetMessageHandler(). It just tells that once you call SetMessageHandler() there will be messages ready to be delivered.

> We could send first HasPendingMessage state with TabParent::LoadURL to child
> and asynchronously update live TabParents their HasPendingMessage state. So
> app could get accurate info at startup and almost accurate info if they want
> to check that later (I think this use case is rare though).

Yeah, the "almost accurate" part is annoying in theory, but I agree that our code patterns will not hit that.

Comment 3

[David Flanagan [:djf]]

This bug is referenced in bug 1086963, which is about startup time for the Gallery app.  Since you've pointed out that this is a bottleneck, I'm likely to modify Gallery to remove the call to hasPendingMessage(), and instead just check location.href.

This may still be a good optimization to do, but in terms of speeding up Gallery, it probably will not help if I remove the hasPendingMessage call in that app.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: patch

After patch the hasPendingMessage check disappeared completely.

https://people.mozilla.org/~bgirard/cleopatra/#report=05e2a0f064e9cdc5d3c82c349c32a23835d46fcc&filter=[{"type"%3A"RangeSampleFilter","start"%3A1477,"end"%3A2129}]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38f4d9173091

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

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

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

I should also remove the old hasPendingMessage message handler.

::: dom/messages/SystemMessageManager.js
@@ +195,5 @@
>  
> +    let cache = Cc["@mozilla.org/system-message-cache;1"]
> +                  .getService(Ci.nsISystemMessageCache);
> +
> +    return cache.hasPendingMessage(aType, this._pageURL, this._manifestURL);

I should put a comment here:
/*
 * NB: If the system message is fired after we received the cache and before we
 *     registered the pageURL we will get false negative however this is unlikely
 *     and will do no harm.
 */

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: patch

Comment 7

[[:fabrice] Fabrice Desré]

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

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

the overall design looks fine, but I think there are some issues.
Also, a bunch of .orig files ended up in your patch.

::: dom/messages/SystemMessageCache.js
@@ +51,5 @@
> +
> +  receiveMessage: function(aMessage) {
> +    switch (aMessage.name) {
> +    case "SystemMessageCache:RefreshCache":
> +      this._pagesCache = aMessage.json;

nit: s/aMessage.json/aMessage.data

@@ +52,5 @@
> +  receiveMessage: function(aMessage) {
> +    switch (aMessage.name) {
> +    case "SystemMessageCache:RefreshCache":
> +      this._pagesCache = aMessage.json;
> +      debug("received RefreshCache " + JSON.stringify(aMessage.json));

please don't use stringify in unconditional debug()

::: dom/messages/SystemMessageInternal.js
@@ +182,5 @@
> +    this._pages.forEach(function(aPage) {
> +      if (aPage.manifestURL === aManifestURL) {
> +        pages.push(aPage);
> +      }
> +    }, this);

you don't need the 'this' here.

@@ +365,5 @@
> +    let manifestURL = aManifestURI.spec;
> +    let pages = this._findPagesForApp(manifestURL);
> +    let cache = [];
> +    pages.forEach(function(aPage) {
> +      if (aPage.pendingMessages.length != 0) {

why not move this condition in _findPagesForApp() ?

@@ +480,5 @@
>              winCounts[pageURL]++;
>            }
>          }
>  
> +        this.refreshCache(aMessage.target, msg.manifestURL);

msg.manifestURL is a string here, but refreshCache expected a nsIURI to access the .spec
Something's wrong!

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: cache hasPendingMessage in content process

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8560279 [details] [diff] [review]
cache hasPendingMessage in content process

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

lgtm, but I would like QA to run a smoketest pass with that patch since there is potential for bustage.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

qawanted to run a smoketest with the patch applied, thanks!

Comment 11

[Al Tsai [:atsai]]

Tested with local build, and I didn't encounter any serious failure. Also passed smoke tests.

Gaia      2bef75b
Gecko     74f80ff

remove qawanted and ni

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/e60f5fee730f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8560279 [details] [diff] [review]
cache hasPendingMessage in content process

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: slower app-start-up time when apps use system-message hasPendingMessages()
Testing completed: patches has been stuck on m-c for a while
Risk to taking this patch (and alternatives if risky): app may fail to detect there are pending messages thus display a wrong first page then display the correct one. Alternatively if we don't uplift this patch then apps could be changed to use window.location to detect if it's receiving system-messages
String or UUID changes made by this patch: N/A

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7263340a2474

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Bug 1112989 needed to be backed out and this was blocking it. Backed out as well.
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5481f6e66c8b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/99b0b934f986

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4f8e023af4cc

https://treeherder.mozilla.org/logviewer.html#?job_id=49706&repo=mozilla-b2g37_v2_2

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Patch for v2.2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d1f0a682778f