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

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gene Lian (I already quit Mozilla), Assigned: Gene Lian (I already quit Mozilla))

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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. :)
(Assignee)

Updated

5 years ago
Blocks: 755245
blocking-basecamp: --- → ?
(Assignee)

Comment 1

5 years ago
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

5 years ago
One fast solution is to send a "SystemMessageManager:GetPending" message back to parent for cleaning pending queue for every received system message.
(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?
Is this why I get multiple SMS notifications?  Are we clearing the message queue?
blocking-basecamp: ? → +

Comment 5

5 years ago
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.
(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.
(Assignee)

Comment 7

5 years ago
Should be able to upload a patch for solving this later. ;)
Assignee: nobody → clian
(Assignee)

Comment 8

5 years ago
Created attachment 666915 [details] [diff] [review]
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!
Attachment #666915 - Flags: feedback?(fabrice)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
(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?
(Assignee)

Comment 11

5 years ago
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).
(Assignee)

Comment 12

5 years ago
Created attachment 667406 [details] [diff] [review]
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! :)
Attachment #666915 - Attachment is obsolete: true
Attachment #666915 - Flags: feedback?(fabrice)
Attachment #667406 - Flags: review?(fabrice)
(Assignee)

Comment 13

5 years ago
Created attachment 667409 [details] [diff] [review]
Patch 1.1

Some fine-tunes. Please see comment #12 for the summary. Thanks!
Attachment #667406 - Attachment is obsolete: true
Attachment #667406 - Flags: review?(fabrice)
Attachment #667409 - Flags: review?(fabrice)
(Assignee)

Comment 14

5 years ago
Created attachment 667477 [details] [diff] [review]
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!
Attachment #667409 - Attachment is obsolete: true
Attachment #667409 - Flags: review?(fabrice)
Attachment #667477 - Flags: review?(fabrice)
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);
Attachment #667477 - Flags: review?(fabrice) → review-
(Assignee)

Comment 16

5 years ago
(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?
(Assignee)

Comment 17

5 years ago
Created attachment 667892 [details] [diff] [review]
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!
Attachment #667477 - Attachment is obsolete: true
Attachment #667892 - Flags: review?(fabrice)
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));
Attachment #667892 - Flags: review?(fabrice) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 669384 [details] [diff] [review]
Patch V2.1

Rebased. r=fabrice at comment 15.

Thanks Fabrice for the review!

Try: https://tbpl.mozilla.org/?tree=Try&rev=37ad90134d69
Attachment #667892 - Attachment is obsolete: true
Attachment #669384 - Flags: review+
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Gene, in-testsuite- means that testing this is not doable. I doubt this is the case here, is it?
Flags: in-testsuite- → in-testsuite?
(Assignee)

Comment 21

5 years ago
Yes. Thanks Mounir for pointing this out!
This doesn't apply cleanly to inbound. Please rebase.
Keywords: checkin-needed
(Assignee)

Comment 23

5 years ago
(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. ;)
Keywords: checkin-needed
How convenient!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab04b3c23f0d
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ab04b3c23f0d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
https://hg.mozilla.org/releases/mozilla-aurora/rev/c0a02b1a8a03
status-firefox18: --- → fixed
(Assignee)

Updated

5 years ago
Blocks: 800169
You need to log in before you can comment on or make changes to this bug.