Closed Bug 800431 Opened 12 years ago Closed 6 years ago

System Message for Alarm delivers message to page that added the alarm, and not the page declared in the manifest

Categories

(Firefox OS Graveyard :: General, defect)

Other
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-basecamp:-)

RESOLVED WONTFIX
DeveloperPhone
blocking-basecamp -

People

(Reporter: jmcf, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file The test case
The bug can be reproduced by using the attached test-case (it is also included at test case in a standalone app that works perfectly). To check whether the message is delivered or not it is enough to kill the app and check if the app is restarted after a minute (by watching logcat). 


Note: In a standalone app the test works properly provided the URL is set to /index.html, otherwise the test fails.
Fabrice says he needs to investigate a bit.
Whiteboard: [blocked-on-input Fabrice Desré]
The alarm API works in such a way that it only delivers system messages to the page that called the API. So if you do mozAlarms.add() from point1/index.html but have set a message handler for point2/index.html, this can't work.

Is this the issue you see?
First of all sorry in the delay responding as Friday was a bank holiday in Spain.

Yes, now I have realized that, but what's the point then in having the manifest declaration? In addition this limitation seems to be too restrictive. I think that a system message potentially could be delivered to any page belonging to the app. All in all is the app which creates the alarm. 

My use case is the following:

I need to setup an Alarm to update periodically the cached data from Facebook (this is contractual requirement from FB). The alarm is going to be set from the import page but my intention is that the sync process is performed by another page (a clean page) in another entry point of the same app (Comms App). Such a page will only handle the synchronization. IMHO such an approach would make things cleaner and easier.

Is there any possibility of changing that or should I need to find a workaround?
The manifest declaration is needed to register messages for pages that have not been opened yet. Gecko needs this to know where to route messages (especially the broadcasted ones).

I can understand that you find that too restrictive, but we won't make this kind of changes for v1.
Flags: needinfo?(fabrice)
Whiteboard: [blocked-on-input Fabrice Desré]
As per #4, not for v1.
blocking-basecamp: ? → -
blocking-kilimanjaro: ? → ---
> I can understand that you find that too restrictive, but we won't make this
> kind of changes for v1.

Then, please, update the wiki because this is not the specification. (FAQ 1st point: https://wiki.mozilla.org/WebAPI/AlarmAPI)
(In reply to Salvador de la Puente González [:salva] from comment #6)
> > I can understand that you find that too restrictive, but we won't make this
> > kind of changes for v1.
> 
> Then, please, update the wiki because this is not the specification. (FAQ
> 1st point: https://wiki.mozilla.org/WebAPI/AlarmAPI)

Thanks for the reminder. Done by FAQ #2.
Just making this block System Message API and Alarm API to easily keep track of that in the future.
Flags: needinfo?(fabrice)
This is a first shot at delivering messages to the pages specified in the manifest. I don't like the part where it requires iterating over this._pages twice. If this is the right approach I'll fix it.
Attachment #733585 - Flags: feedback?(gene.lian)
Comment on attachment 733585 [details] [diff] [review]
add sendMessageToApp which sends messages to pages registered in manifest

Hi Nikhil,

Sorry for the delayed response. I'm wondering why don't you just the existing API:

void sendMessage(in DOMString type, in jsval message, in nsIURI pageURI, in nsIURI manifestURI);

and check if the |pageURI| is null or not in the implementation? In this way, you don't need a new API.
Attachment #733585 - Flags: feedback?(gene.lian) → feedback-
(In reply to Gene Lian [:gene] from comment #10)
> Sorry for the delayed response. I'm wondering why don't you just the
> existing API:
> 
> void sendMessage(in DOMString type, in jsval message, in nsIURI pageURI, in
> nsIURI manifestURI);
> 
> and check if the |pageURI| is null or not in the implementation? In this
> way, you don't need a new API.

I'll try that, I was aiming for an API where null doesn't have special meaning.
Gene, please review the following changes:
1) Changes the order of params to sendMessage() and makes pageURI optional.
I could've not changed the order, but I don't think that is a good API with arguments.length checks, especially since manifest and page url have the same types.

2) Modifies callers of sendMessage to use the new ordering. I've not removed the pageURI from callers I don't know about, so their semantics should be unaffected.

3) I *have* modified the Alarm API sendMessage() call to use the manifest only form, since this bug originally asks for that.

Doug, can you check the PushService modification?
Assignee: fabrice → nsm.nikhil
Attachment #733585 - Attachment is obsolete: true
Attachment #757476 - Flags: review?(gene.lian)
Attachment #757476 - Flags: review?(doug.turner)
Comment on attachment 757476 [details] [diff] [review]
Use manifest messages entries.

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

::: dom/alarm/AlarmService.jsm
@@ +243,2 @@
>  
> +    messenger.sendMessage("alarm", this._publicAlarm(aAlarm), manifestURI);

This is a bit controversial... Asking Jonas.

::: dom/messages/SystemMessageInternal.js
@@ +146,5 @@
>  
>      debug("Sending " + aType + " " + JSON.stringify(aMessage) +
> +      " for " + (aPageURI ? aPageURI.spec : "") + " @ " + aManifestURI.spec);
> +
> +    if (!aPageURI) {

Could you please add some comments for describing the purpose before |if (!aPageURI)|?

@@ +148,5 @@
> +      " for " + (aPageURI ? aPageURI.spec : "") + " @ " + aManifestURI.spec);
> +
> +    if (!aPageURI) {
> +      this._pages.forEach(function(aPage) {
> +        if (aPage.manifest === aManifestURI.spec) {

This is wrong. You also need to check the system message type:

if (aPage.manifest === aManifestURI.spec && aPage.type === aType) {

@@ +149,5 @@
> +
> +    if (!aPageURI) {
> +      this._pages.forEach(function(aPage) {
> +        if (aPage.manifest === aManifestURI.spec) {
> +          this.sendMessage(aType, aMessage, aManifestURI, Services.io.newURI(aPage.uri, null, null));

Please fold this line:

this.sendMessage(aType, aMessage, aManifestURI,
                 Services.io.newURI(aPage.uri, null, null));
Attachment #757476 - Flags: review?(gene.lian) → review-
Hi Jonas and Mounir,

Supposing we have page_A and page_B which belong to the same app and we have the following manifest.webapp:

"messages": [
  { "alarm": page_A, "alarm": page_B }
]

Supposing we set an alarm by calling mozAlarms.add(...) in the page_A. When the alarm fires, can page_B also receive the "alarm" system message? In the current design, only the page_A that set the alarm can receive that system message [1].

[1] https://wiki.mozilla.org/WebAPI/AlarmAPI, FAQ #2.
Flags: needinfo?(mounir)
Flags: needinfo?(jonas)
If we really need to fix this, we need to fix Bug 874339 first to ensure the registered pages are not duplicated.
Depends on: 874339
Oops! I wrongly described the way of registering multiple pages for one type in the manifest. Anyway, you know what I mean. :p
Please see my post on dev.webapi [1] which is relevant to this bug. I believe only a part of this bug should be fixed, which is to parse the "messages" section only for certain WebAPIs, of which Alarms is not one since the web developer can select a per alarm handler.

[1] https://groups.google.com/d/msg/mozilla.dev.webapi/diddFoHYLVs/69ayu8U8jiMJ
Comment on attachment 757476 [details] [diff] [review]
Use manifest messages entries.

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

looks like we need a new patch here.  I am fine with the push changes.
Attachment #757476 - Flags: review?(doug.turner)
Generally speaking system messages should always go to the page that is registered in the manifest. Is there a reason for the explicit limitation in the Alarm API of only allowing the registering page to receive the alarm? If not, I'd prefer to remove it.

(In reply to Gene Lian [:gene] from comment #14)
> Hi Jonas and Mounir,
> 
> Supposing we have page_A and page_B which belong to the same app and we have
> the following manifest.webapp:
> 
> "messages": [
>   { "alarm": page_A, "alarm": page_B }
> ]

It doesn't really make sense to register two pages to receive a system message. Whenever a message is delivered we should navigate the app to that page and send the message to that page. That obviously isn't possible to do to two pages at the same time.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #19)
> Generally speaking system messages should always go to the page that is
> registered in the manifest. Is there a reason for the explicit limitation in
> the Alarm API of only allowing the registering page to receive the alarm?

No, I don't know either. It was designed as it was in the first place. We'll remove that. Thanks Jonas!

> It doesn't really make sense to register two pages to receive a system
> message. Whenever a message is delivered we should navigate the app to that
> page and send the message to that page. That obviously isn't possible to do
> to two pages at the same time.

It's my misunderstanding. Sorry about that.


Hi Nikhil, please go ahead to come up with the new patch based on the comment #13 except for the controversial part. Thanks!
Flags: needinfo?(mounir)
If I understand correctly, the summary of the bug no longer matches what is being fixed. Is that correct?
Flags: needinfo?(nsm.nikhil)
I added a note about this limitation at https://developer.mozilla.org/en-US/Apps/Developing/Manifest#messages so I'm adding dev-doc-needed so this note will be removed when the bug is fixed.
Keywords: dev-doc-needed
Flags: needinfo?(nsm.nikhil)
Summary: System Message for Alarm is not delivered when multiple entry points are present → System Message for Alarm delivers message to page that added the alarm, and not the page declared in the manifest
It seems we are moving away from system messages with a lot of the newer APIs relying on Service Workers. Nobody seems want it either :) I am unassigning myself.
Assignee: nsm.nikhil → nobody
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.