795782 - System Message API: Shouldn't pend messages for running apps to avoid re-firing them when restarting apps

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Gene Lian (I already quit Mozilla)]

This issue is discovered when playing the alarm clock. The STR is described as below:

1. Open alarm clock app, set an alarm, waiting on firing and closing the snoozing page.
2. Long-push the home button and then use the process manager to kill the clock app.
3. Reopen the alarm clock app and the alarm we set at step #1 will be fired again.

After some investigation, I think the root cause is: the SystemMessageInternal would always queue the messages even if they have been sent to the opened apps. However, if the apps have been killed and opened again at run-time, they would redundantly ask the pending messages from SystemMessageInternal and handle them again.

Personally, I feel this one could be a very critical issue because the system message mechanism is applied on lots of apps/features now. If users re-launch the apps due to some reasons, the previous messages would be handled again. Telephony, SMS, STK, Push-Notification could also have chance to encounter the same problem.

I could be wrong. Please feel free to correct me. :)

Comment 1

[Gene Lian (I already quit Mozilla)]

The possible solutions could be: SystemMessageInternal only needs to queue the system messages for non-running apps. When the apps are launched, these pending messages should be handled and cleaned up as expected. After that, we don't need to queue up the coming messages anymore if the apps are already running.

But how to know whether an app is running or not? As my best understanding, the Process Manager is totally handled by Gaia so the Gecko could have difficulties knowing the app's running status (like killed by users). Right?

Comment 2

[Thinker Li [:sinker]]

One fast solution is to send a "SystemMessageManager:GetPending" message back to parent for cleaning pending queue for every received system message.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Gene Lian [:gene] from comment #0)
> After some investigation, I think the root cause is: the
> SystemMessageInternal would always queue the messages even if they have been
> sent to the opened apps. However, if the apps have been killed and opened
> again at run-time, they would redundantly ask the pending messages from
> SystemMessageInternal and handle them again.

If system message cannot manage the state of the message (delivered, pending, etc.), then this is definitely a must-fix item. I wonder what is the other code path that will clean up the message? "Delivered" is the only condition that system message need to clean up the message right?

Comment 4

[Andrew Overholt [:overholt]]

Is this why I get multiple SMS notifications?  Are we clearing the message queue?

Comment 5

[Thinker Li [:sinker]]

AFAIK, only "SystemMessageManager:GetPending" would clean pending queue.

System message dispatches messages to registered listeners.  It does not tell which listener is from target app, I think any one (? not quite sure) can register a listener for messages of given type to give app.  So, add a flag in "SystemMessageManager:GetPending" frame message to indicate hte listener a target app, or target app issue another frame message to clean pending queue explicitly.

Comment 6

[[:fabrice] Fabrice Desré]

(In reply to Thinker Li [:sinker] from comment #5)
> AFAIK, only "SystemMessageManager:GetPending" would clean pending queue.

This clears the queue on the parent side, and it's called when the DOM/content part does either a call to mozSetMessageHandler() or mozHadPendingMessage().

So, according to the steps in comment #0, what's happening is that:
1) We call mozSetMessageHandler(), but the message queue is empty.
2) We send a message, it is handled directly by the app. We don't clear the queue at this point.
3) We re-launch the app, and when we call mozSetMessageHandler() again, the queue is flushed and
   we send the same event back.

So it looks like flushing the pending queue when the content side acknowledges that it received a message would fix that bug. As Thinker notes, this is very similar to the GetPending message, but this should be send async and not return anything.

Comment 7

[Gene Lian (I already quit Mozilla)]

Should be able to upload a patch for solving this later. ;)

Comment 8

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch

Hi Fabrice again :)

Eventually, I found a probably better solution to addressing this issue. I think we can directly rely on the current _listeners[] mechanism to know whether an app is running or not. Two cases can be discussed as below:

1. If the app is running (existing in the _listeners[]), then we can directly send the message to the child process.

2. If the app is NOT running (it would be removed from _listeners[] by "child-process-shutdown"), then we need to queue the message in the parent process until the app is opened and registering handlers.

Therefore, in this patch, the |return;| statements are added to magically solve this issue. ;) Could you please let me know if you don't like this on-going approach (or not)? I'll refine the patch and have more tests on it at the same time. Thanks!

Comment 9

[Thinker Li [:sinker]]

Comment on attachment 666915 [details] [diff] [review]
WIP Patch

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

::: dom/messages/SystemMessageInternal.js
@@ +71,5 @@
>                                     { type: aType,
>                                       msg: aMessage,
>                                       manifest: aManifestURI.spec })
>        });
> +      return;

Is it possible to lost messages if the message was send when the app is LOADING another page.  Another issue is only one page will receive the message if more than one frame request of an app request for the message.  I am not sure if later one is valid.

Comment 10

[Gene Lian (I already quit Mozilla)]

(In reply to Thinker Li [:sinker] from comment #9)
> Comment on attachment 666915 [details] [diff] [review]
> WIP Patch
> 
> Review of attachment 666915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/messages/SystemMessageInternal.js
> @@ +71,5 @@
> >                                     { type: aType,
> >                                       msg: aMessage,
> >                                       manifest: aManifestURI.spec })
> >        });
> > +      return;
> 
> Is it possible to lost messages if the message was send when the app is
> LOADING another page.  Another issue is only one page will receive the
> message if more than one frame request of an app request for the message.  I
> am not sure if later one is valid.

Thanks Thinker for the suggestions! However, I don't quite understand your concerns here. Do you mean we still need to open the app's pages in any way even if we're sure the app's child process is running?

Comment 11

[Gene Lian (I already quit Mozilla)]

After having discussing with Thinker, I understand better his biggest concern: since "child-process-shutdown" is notified by async code, we could have chance to fire a system message to a killed app before the parent process actually receives "child-process-shutdown".

I'll upload a new patch later, which just adopts the solution that Fabrice and Thinker proposed in the previous comments (i.e. the child process will send a acknowledgement to parent process when it receives the system message to clean up the pending message queue).

Comment 12

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Fabrice again :)

The final solution is: the child process will send an acknowledgement to parent process when it receives the system message, so that the parent process can know to clean up the pending messages which shouldn't be handled again by child process. Changes are summarized as below for your convenience:

1. The child process will return the acknowledgement by |"SystemMessageManager:Message:Return:OK"|.

2. In order to identify which message needs to be removed from the queue, we assign each message going to be sent/broadcasted with a UUID.

The rest is code clean-ups (please feel free to let me know if you don't like it):

1. Use |_isPageMatched()| to replace the comparison of {type, manifest, uri} everywhere.

2. s/_processPage()/_openAppPage(), which sounds more descriptive.

3. Better to remove the "webapps-registry-ready" observer after it is received.

4. A reviewer used to suggest me to add braces for each switch-case if we have variable declaration in it.

5. Add |;| for each JS statement.

Thanks for your reviews again! :)

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Patch 1.1

Some fine-tunes. Please see comment #12 for the summary. Thanks!

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: Patch V1.2

Need to clean up the message which might be queued in more than one page. Please see comment #12 for the summary. Thanks!

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 667477 [details] [diff] [review]
Patch V1.2

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

There is no good reason to prevent users of having a sysMsgId fields on their messages. You need to transmit this id in a different way.
Why did you add |uri: aPageURI.spec| to the message ?

Also, instead of doing myArray.some((function(...){..}).bind(this)) you can just do myArray.some(function(...){..}, this);

Comment 16

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #15)
> Comment on attachment 667477 [details] [diff] [review]
> Patch V1.2
> 
> Review of attachment 667477 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is no good reason to prevent users of having a sysMsgId fields on
> their messages. You need to transmit this id in a different way.

Good point! I'll move it to the upper layer: {type, manifest, uri, message, sysMsgId}, instead of putting it in the message.

> Why did you add |uri: aPageURI.spec| to the message ?

Because we need {type, manifest, uri} to find the right page to clear it's pending messages, the message sent to the child process doesn't contain the uri. Does that sound reasonable to you?

Comment 17

[Gene Lian (I already quit Mozilla)]

Attachment: Patch V2

Hi Fabrice,

New patch is done, summarized as below:

1. Addressing comment 15. I pass the message ID beyond the message object. Also, use 2nd parameter to pass |this| for |.forEach()| and |.some()|.

2. s/pending/pendingMessages. This is just a minor code clean-up, which sounds more descriptive to me. Please feel free to let me know if it's not comfortable to you. :)

Thanks for the review!

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 667892 [details] [diff] [review]
Patch V2

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

::: dom/messages/SystemMessageManager.js
@@ +64,5 @@
>        }
>      }
>  
> +    aHandler.handleMessage(
> +      wrapped ? aMessage : ObjectWrapper.wrap(aMessage, this._window));

Nit: If you want to break this up, I find this more readable (align ? and : ) :
aHandler.handleMessage(wrapped ? aMessage
                               : ObjectWrapper.wrap(aMessage, this._window));

Comment 19

[Gene Lian (I already quit Mozilla)]

Attachment: Patch V2.1

Rebased. r=fabrice at comment 15.

Thanks Fabrice for the review!

Try: https://tbpl.mozilla.org/?tree=Try&rev=37ad90134d69

Comment 20

[Mounir Lamouri (:mounir)]

Gene, in-testsuite- means that testing this is not doable. I doubt this is the case here, is it?

Comment 21

[Gene Lian (I already quit Mozilla)]

Yes. Thanks Mounir for pointing this out!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to inbound. Please rebase.

Comment 23

[Gene Lian (I already quit Mozilla)]

(In reply to Ryan VanderMeulen from comment #22)
> This doesn't apply cleanly to inbound. Please rebase.

Actually, the patch had conflicts with my patch has just been backed out few minutes ago. Let's check this in again. Should be clean. ;)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

How convenient!

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

Comment 25

[Ed Morley [:emorley]]

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

Comment 26

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c0a02b1a8a03