818000 - system messages are ignored if the handler pathname is not the same as the foreground application pathname

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Salvador de la Puente González [:salva]]

According to this: 
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L1234

When an application is in foreground and the system message delivered is asking for a handler which URL is not the URL of the application being displayed, the system message is just ignored.

This not only betray one the system-message's goals (its delivery is guaranteed) but force the developer to repeat the code handling the system messages in every view the application could appear.

Comment 1

[Salvador de la Puente González [:salva]]

I'm always talking within the same application (same manifest or same domain name).

Comment 2

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

You are very welcome to normalize the rule and try not to trigger problem like bug 805547, especially the part regarding handling hashes and query strings in the URL.

Comment 3

[Salvador de la Puente González [:salva]]

I only open the bug to leave constancy. In my opinion, we need to deeply review how to handle different kind of entry points. Anyway, can you talk me about the main problems of this and what are the main constrains to meet?

Comment 4

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

Please read bug 805547 comment 15 ...

We need to establish a solid spec on which frame should handle which message/activity, before working on the code. Otherwise that's just a workaround on top of another workaround, and the whole thing is a hack.

Comment 5

[Gene Lian (I already quit Mozilla)]

(In reply to Salvador de la Puente González [:salva] from comment #0)
> According to this: 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> window_manager.js#L1234
> 
> When an application is in foreground and the system message delivered is
> asking for a handler which URL is not the URL of the application being
> displayed, the system message is just ignored.
> 
> This not only betray one the system-message's goals (its delivery is
> guaranteed) but force the developer to repeat the code handling the system
> messages in every view the application could appear.

Agreed. I don't understand what the spec else needs to be redefined. The window_manager.js should be capable of opening that specific window page in any way to run its .mozSetMessageHandler(...).

Comment 6

[Gene Lian (I already quit Mozilla)]

Although the Gecko might not really be involved, blocking the system-message-api to easily track the system-message related bugs.

Comment 7

[Wilson Page [:wilsonpage]]

Experiencing this issue in camera. When our 'record' activity registers an 'href' (index.html#record) in the manifest and we trigger that activity from another app whilst the camera is running, our .mozSetMessageHandler('activity') callback never fires.

If the activity definition has no 'href' attribute, all works well.
If the activity definition has 'href' matching the app default (/index.html) Camera crashed, Gaia become unusable and requires a forced power off to recover.

Comment 8

[Wilson Page [:wilsonpage]]

Any movement/info on this?

Comment 9

[Wilson Page [:wilsonpage]]

System message debug output when invoking Camera app from Video app:

I/Gecko   ( 1259): -- SystemMessageInternal 1398275082579 : Sending activity {"id":"{34a5e8a6-3d36-4567-89c1-ec126dfcc488}","payload":{"name":"record","data":{"type":"videos"}},"target":{"filters":{"type":["photos","videos"]},"disposition":"window","href":"app://camera.gaiamobile.org/index.html#record"}} for app://camera.gaiamobile.org/index.html#record @ app://camera.gaiamobile.org/manifest.webapp; extra: {"manifestURL":"app://video.gaiamobile.org/manifest.webapp","pageURL":"app://video.gaiamobile.org/index.html"}
I/Gecko   ( 1259): -- SystemMessageInternal 1398275082582 : Returned status of sending message: 2
I/Gecko   ( 1259): -- SystemMessageInternal 1398275082583 : _getMessageConfigurator for type: activity
I/Gecko   ( 1259): -- SystemMessageInternal 1398275082584 : Asking to open {"pageURL":"app://camera.gaiamobile.org/index.html#record","manifestURL":"app://camera.gaiamobile.org/manifest.webapp","type":"activity","extra":{"manifestURL":"app://video.gaiamobile.org/manifest.webapp","pageURL":"app://video.gaiamobile.org/index.html"},"target":{"filters":{"type":["photos","videos"]},"disposition":"window","href":"app://camera.gaiamobile.org/index.html#record"},"onlyShowApp":false,"showApp":true}

Comment 10

[Wilson Page [:wilsonpage]]

logcat output when camera launched fresh from video app: https://pastebin.mozilla.org/4919944

Comment 11

[Wilson Page [:wilsonpage]]

Recreated on sms app too:

STEPS:

1. Add a 'href' property to the 'share' activity (index.html#share)
2. make reset-gaia
3. Open the sms app
4. Open the gallery app
5. Share a picture through the sms app

ACTUAL:

SMS app launches but fails to launch message composer as `navigator.mozSetMessageHandler` callback never fires.

EXPECTED:

SMS app should launch message composer as a result of `navigator.mozSetMessageHandler` callback firing.

Comment 12

[Salvador de la Puente González [:salva]]

Maybe we can take a look at this for 1.5 or 1.6.

Comment 13

[Gene Lian (I already quit Mozilla)]

Let me take a look. This is definitely a bug. I'd plan to dispatch this issue to a newbiew in our team and I'll mentor him to solve this. Thanks Wilson for bringing this up again and providing a nice STR.

Comment 14

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Hi Wilson,

I was trying to follow your STR about sms app, but unfortunately couldn't reproduce it against the latest code base (Gecko: mozilla-central, Gaia: master) on Unagi. Neither did another issue reported between video and camera apps. Yet the attached logs look alright to me since it indicates the app was brought into running and the correspondent messages were dispatched.

On the other hand, we realize there were code changes checked into Gecko and Gaia in the past couple weeks. So could you double check if the issue still happen at your end? If it does, it would help a lot if more details could be provided.

Comment 15

[Wilson Page [:wilsonpage]]

seanlin: This commit https://github.com/mozilla-b2g/gaia/commit/224a6f4d85a6436330b702c4f08a93a615f74cb4 (6 days ago) changed the 'share' activity from `"disposition": "window"` to `"disposition": "inline"`. The bug I experienced only affects "window" activities.

Comment 16

[Gene Lian (I already quit Mozilla)]

I didn't follow well. So, is the STR at comment #11 still valid? This issue seems no longer valid to us.

Or do you have any new STR based on the recent change at comment #15?

Thank you for the investigation!

Comment 17

[Wilson Page [:wilsonpage]]

UPDATED STEPS: 

1. Add a 'href' property to the 'share' activity (index.html#share)
2. Change 'disposition' to 'window'
3. make reset-gaia
4. Open the sms app
5. Open the gallery app
6. Share a picture through the sms app

Comment 18

[Sean Lin [:seanlin] (inactive from Mar 2016)]

wilsonpage: Thanks for clarification. The issue is reproducible on my side now.

According to my log pasted at https://pastebin.mozilla.org/5013001, it appears the sms app has a specific bug only with window disposition after it was invoked with the system message. The last line in the log is listed below

E/GeckoConsole( 3557): [JavaScript Error: "TypeError: thread is undefined" {file: "app://sms.gaiamobile.org/js/message_manager.js" line: 332}]

It looks the logic at https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager.js#L332 is not null-safe or well handled.

Overall, the behaviors of system messages on Gecko look alright for me in this case. Yet I'll file a separate bug to sms app for further investigation.

Comment 19

[Salvador de la Puente González [:salva]]

Please, do not close the bug without checking the description in comment 0 holds or not. Let's check if the original behaviour is reproducing. As the line in description is no longer valid, could someone from system app help us if the window_manager is still ignoring the system message?

Comment 20

[Gene Lian (I already quit Mozilla)]

Hi Salvador, do you have any interesting examples reproducing comment #0? That would be helpful for us to understand the insufficiency of the current system message design. We'll talk to someone else if we need to adjust the logic of window manager on the Gaia side.

Comment 21

[Julien Wajsberg [:julienw]]

Cc Michael because this is related to some of our late discussions.

Comment 22

[Julien Wajsberg [:julienw]]

See Bug 995907 comment 18 that is related.

Comment 23

[Wilson Page [:wilsonpage]]

seanlin: I didn't suspect there was a bug within the SMS app, I just used the SMS app as an example. I found this bug in Camera app when making changes to the way activities were handled.

Comment 24

[Michael Henretty [:mikehenrty][:mhenretty]]

I'm going to share my very limited knowledge of system messages in case it helps this bug.

There are three parts to a system message:
1 - gecko tries to send system message to app [1]
2 - if that fails gecko tells the gaia system app to launch the target app [2]
3 - the newly opened target app sets a system message handler, which tells gecko to fetch pending system messages and fire the newly registered handler(s) [3]

The code mentioned in comment 0 [4] only pertains to the app launching part, but the system message _should_ still fire directly from gecko to the registered handler in the running app [5]. Perhaps the page URL check right above is not "hash" aware? [6]


** Note that the system app has changed significantly since comment 0, but I believe the intent was to keep the same functionality in regards to system messages.

1.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageInternal.js#l194
2.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageInternal.js#l213
3.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageManager.js#l147
4.) https://github.com/mozilla-b2g/gaia/blob/8f3578363db5ad9aaf25d7a86335fedbed6519c8/apps/system/js/window_manager.js#L1234
6.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageInternal.js#l613
5.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageInternal.js#l628

Comment 25

[Sean Lin [:seanlin] (inactive from Mar 2016)]

mhenretty: In response to the following concern

"The system message _should_ still fire directly from gecko to the registered handler in the running app [1]. Perhaps the page URL check right above is not "hash" aware? [2]"

Actually it's hash aware. The URL check at [2] is to see if there's any listener associated with the pageURL of the app (recognized by manifestURL). If all of the pages have no registered handler, it implies the target app is not running and then follows step 2 and 3 in comment 24. 

On the other hand, after the message is sent at [1], the SystemMessageManager instance on the child process (app) gets it [3] and eventually fires the registered handler [4]. (The SystemMessageInternal instance is running on the parent process.)

In addition, the number of listeners for a (pageURL, manifestURL) tuple used at [2] is only adjusted when an app is launched or killed. That is, every time when an app is launched, the SystemMessageManager instance on the app process send a system message to the SystemMessageInternal instance on the parent process [5]. And the pageURL is determined by the associated window [6]. For example, if the user open SMS activity by sharing a picture from gallery, the page URL would be app://sms.gaiamobile.org/index.html#activity-share; whereas it's app://sms.gaiamobile.org/index.html if the user launches SMS app from home screen. Likewise, similar logic happens when an running app is killed.

Hope this would help clarify the bug. And it would help a lot if some reproducible steps could be provided.

1.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageInternal.js#l628
2.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageInternal.js#l613
3.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageManager.js#l203
4.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageManager.js#l93
5.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageManager.js#l314
6.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageManager.js#l272

Comment 26

[Michael Henretty [:mikehenrty][:mhenretty]]

Thanks for the info Sean. I'm not sure why you are flagging me for need info though, comment 24 was just my brain dump for what I knew about system messages.

Wilson, can you provide us with some STR in the camera app, since comment 18 suggests there is a separate bug with SMS that is obfuscating the issue there?

Comment 27

[Wilson Page [:wilsonpage]]

I'd like to clarify that this is bug is not exhibited in SMS or Camera at the moment. We worked around the it by not adding and `href` property to the 'record' activity definition. Although here are the steps required to reproduce the bug in Camera.

CAMERA STEPS: 

1. Add a 'href' property to the 'record' activity in manifest.webapp (index.html#record)
3. `$ make reset-gaia`
4. Open the camera app
5. Open the video app
6. Tap the camera icon

ACTUAL:

Camera app is loaded in 'camera' mode

EXPECTED:

Camera app is loaded in 'video' mode

Comment 28

[Michael Henretty [:mikehenrty][:mhenretty]]

Thanks Wilson!

Sean, is this enough information for you to reproduce the issue?

Comment 29

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Thank you guys! We've figured out the root cause and summarize as follows.

1. When the user first open the camera app from screen, the system app launches the camera app and instantiates a SystemMessageManager along with it. While the SystemMessageManager instance is initialized, it registers the page URL determined by the associated window [1] to SystemMessageInternal on the parent process [2]. In this case, it's app://camera.gaiamobile.org/index.html seemly from the launch_path of the app.
2. Once the user follows the STR to open the camera app with window disposition from the video app, the page URL it's looking for is app://camera.gaiamobile.org/index.html#record. SystemMessageInternal on the parent process cannot find any listener for this page URL [3], so it tells the system app to launch the camera app [4] and queues the pending message.
3. However, it appears the system app uses the current running camera app instance without actually launching. So no new SystemMessageManager is initialized to tell the parent process to listen to the page URL in step 2 (app://camera.gaiamobile.org/index.html#record), and therefore no newly opened app to set the handler and fetch the pending message for that specific page URL.

A possible fix could be done in the system app to trigger the handler-setting and pending-message-fetching mechanism for the page URL.

1.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageManager.js#l272
2.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageManager.js#l314
3.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageInternal.js#l613
4.) http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/SystemMessageInternal.js#l213

Comment 30

[Julien Wajsberg [:julienw]]

To be crystal clear: resolving this bug is absolutely necessary to allow separate pages to handle system messages without resorting to ugly workarounds.

Comment 31

[Salvador de la Puente González [:salva]]

Notice I was talking about a problem in Gaia, no longer located in the same place. Here is what I refer on v1-train. I don't know where this code went:
https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/system/js/window_manager.js#L1564

Comment 32

[Sean Lin [:seanlin] (inactive from Mar 2016)]

After discussing with Alive and Gene, a fix at the system app to switch the URL for the existing displayed window looks the right way to do.

A short summary:
The mechanism of system messages at Gecko appears more page-based; whereas at Gaia the window is maintained in a more app-based way (in consideration of consumed resources). So when a non-activity system message is sent from Gecko for app://camera.gaiamobile.org/index.html#record, the Gaia system app tries to reuse an existent window belonging to the same app, with URL of app://camera.gaiamobile.org/index.html, to handle it. Thus, switching the URL looks inevitable and should be well-maintained for such scenario.

Though no idea about either the reason why the original implementation ignored this, or where the logic eventually went in the current code base, some work might need to do at app.reviveBrowser(). [1] Yet someone with better Gaia expertise could be a more proper candidate to fix this issue, and might know more about some past decisions as well. So I'll unassign myself and let someone takes over it.

1.) https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window_factory.js#L167

Comment 33

[Julien Wajsberg [:julienw]]

Gregor, is it possible to find someone to fix this? This is necessary to properly separate applications in separate HTML files. For example, the SMS app could use a light page to handle "sms-received" and the clock could use a light page to handle the alarm, and costcontrol could stop using an iframe in the widget.

FTR I disagree with comment 32 if the URL is a different endpoint (different file), but agree if this is the same endpoint with a different hash.

But maybe we need something more involved if the URL is a different file, because we probably don't want to launch a new window if an opened window has already a registered JS handler.

* if the app has an opened window
  o if the system message's endpoint is the same as one the opened window's endpoint, except hash
    - change this opened window's URL
    - send the system message to this window
  o else if one of the opened window has a JS handler registered for this system message
    - send the system message to this window

* if the system message has still not been sent
  o launch a new window with the system message's endpoint URL
  o send the system message to this window

How does this sound?

Comment 34

[Julien Wajsberg [:julienw]]

I was thinking that if we want to be more explicit, we could add all possibly endpoints in the manifest, in a array. The first element would be the endpoint to use if no window is launched yet. Maybe a good idea to ask on dev-webapi.

Comment 35

[Julien Wajsberg [:julienw]]

Bug 1156464 is another manifestation of this issue...

Comment 36

[Julien Wajsberg [:julienw]]

Fabrice, I don't know if you can make this move forward?

I think we'll need a proper support of system messages accross different views with the new architecture...

Comment 37

[[:fabrice] Fabrice Desré]

(In reply to Julien Wajsberg [:julienw] (PTO -> Apr 27) from comment #36)
> Fabrice, I don't know if you can make this move forward?
> 
> I think we'll need a proper support of system messages accross different
> views with the new architecture...

right... Can you provide me a small test app?

Comment 38

[Julien Wajsberg [:julienw]]

Hey Fabrice, you can find a test app here: https://github.com/julienw/app-system-messages

There are 3 branches:
* the master branch works with always the same URL for the system messages.
* the "using-hashes" uses different hashes for the messages "sms-received" and "notification".
* the "using-separate-url" uses different URLs for these messages.

You can easily install the app using sole's tool install-to-adb [1].

Here are the STR that are problematic with the 2 using-* branches:

STR1:
1. launch the app
2. receive a SMS

=> no notification is displayed.

STR2:
1. check the app is closed.
2. receive a SMS
3. tap the notification
4. receive another SMS

=> no notification is displayed

You'll notice that the "notification" case works quite well but this is because I also use the "onclick" handler of the Notification object. Actually the system message doesn't work either with this STR:

STR3:
1. check the app is closed.
2. receive a SMS
=> a notification is added but please ignore it for now.
3. force close the app.
4. receive another SMS.
=> another notification is added
5. tap the first notification
=> nothing happens because this notification's "onclick" handler is not accessible anymore.
=> tapping the second notification works thanks to the "onclick" handler.

(Actually this STR does not work in the current SMS app either because we're actually using a hash now... see bug 1156464)

[1] https://github.com/sole/install-to-adb

Comment 39

[Julien Wajsberg [:julienw]]

Actually I think all these STR are broken in the current SMS app.

Comment 40

[Julien Wajsberg [:julienw]]

In this app, all html files listen to all system messages, but in real life we'll want that only one html file listen to some system messages. I can do another branch with this case on monday.

Comment 41

[Julien Wajsberg [:julienw]]

Pushed a 4th branch: using-separate-url-and-less-handlers
(reminder: repository is https://github.com/julienw/app-system-messages)

In that branch we register only the expected handlers in the various pages.

We have the same issues currently of course, but I expect that the fix should work with all these branches.

I think my 2 proposals in comment 33 and 34 both work with all the branches.

A 3rd simpler proposal could be:

* if the app has an opened window and the URL is the one registered for this system message:
    - send the system message to this window

* if the app has an opened window
  AND if one of the opened window has a JS handler registered for this system message:
    - send the system message to this window

* otherwise:
  o launch a new window with the system message's endpoint URL
  o send the system message to this window



This is somewhat close to the second proposal -- in the second proposal, we know this window can handle the message if the URL is in the array specified in the manifest, while in this proposal we know the window can handle the message if it registered a JS handler.

Comment 42

[Julien Wajsberg [:julienw]]

Pushed a 5th branch: using-hashes-and-removing-after-handling

In that branch, the "sms-received" handler uses no hash, but the "notification" handler still uses a hash. But we remove the hash after handling the system message.

STR for this branch only:
0. Ensure all apps are closed.
1. Send a SMS to this device.
2. kill the "System messages" application.
3. tap the notification (not the SMS one, the System Messages one).
4. send another SMS to the device
=> Observe there is a notification from the SMS app, not from the System Messages app.
5. kill the app.
6. open the System Messages app from its icon
=> Observe the notification is then sent because the app gets its queued system messages.


So there are 2 issues here:
1. when we change the hash, we should get the messages for the new URL, but we don't.
2. when we change the hash, we should get the queued messages (I can't really test this with this STR because this would be a race but we need to take care to this case).

This specific issue is blocking us in bug 1156464 where we want to have a different behavior very early at startup whether the user tapped a notification or launched the app from the icon.

Comment 43

[Gregor Wagner [:gwagner]]

What do you guys think?

Comment 44

[Kevin Grandon :kgrandon]

Seems like Julien/Fabrice probably have a better idea of what's going on here than I do. If I'm following this correctly, I would be in support of ignoring hashes in the URL for messages.

Comment 45

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Kevin Grandon :kgrandon from comment #44)
> Seems like Julien/Fabrice probably have a better idea of what's going on
> here than I do. If I'm following this correctly, I would be in support of
> ignoring hashes in the URL for messages.

+1 to ignore hash; hash should be only meaningful to the application. The same page with different hash should be treated as the same target in gecko.

Comment 46

[[:fabrice] Fabrice Desré]

By ignoring hashes, do you mean that if a message is registered for page.html we would deliver it to page.html#foo ?
I would be ok with that, but we'll have to check how many apps on the marketplace that could break. Also, what about query strings? do we deliver the page.html message to an already opened page.html?foo=bar page?

Comment 47

[Julien Wajsberg [:julienw]]

Even if we ignore hashes and/or query strings, we also need to somehow deliver if the app is opened in another page. Currently the message is only queued until the app is opened in the "right" page...

See my comment 33 and comment 34 and comment 41 for 3 different ways it could work. I just don't know which ones are easiest, or which ones can break existing apps.
Though given the issue of not delivering the message at all I think all apps use the same endpoint for the app and for the system messages endpoints...

Comment 48

[Michael Henretty [:mikehenrty][:mhenretty]]

To state the obvious, the real way to fix this problem is to kill system-messages, and instead allow ServiceWorkers to handle everything system messages were used for. Anything we do in the meantime will be a "temporary" workaround and should be thought of as such. The only question is how soon will we get SW?

With that said, I agree ignoring hashes is fine for now (but not query string params since that signifies a different page). However, when we are on a different page I don't think we can automatically navigate to the message handler page, since that would interrupt the user. Opening a new "background" window sounds nice, but what happens when that window calls mozApp.launch()? Is there a way for that window to know the user is currently in their app in another window? I can't think of one, and this might interrupt the user just like automatically navigating. Seems like a big change to make for app developers right before we switch it to service workers anyway. Personally, I don't think queuing is that bad of an idea. Perhaps we could navigate the window to the message handler page on visibilitychange to make sure we receive the system message in a more timely manner.

So my proposal is mostly like comment 41:
* If the app is opened, and there is a system message handler -> send it

* If the app is opened, but no message handler do one of the following:
  * If app is in foreground -> wait for it to go background, than navigate to message url and send
  * If app is in background -> navigate to message url and send

* If app is closed -> create window with message url, and send it

Of course this all changes if we have multiple windows for apps, and I really think in this case we need service workers to do message handling in that case.

Comment 49

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Michael Henretty [:mhenretty] from comment #48)
> To state the obvious, the real way to fix this problem is to kill
> system-messages, and instead allow ServiceWorkers to handle everything
> system messages were used for. Anything we do in the meantime will be a
> "temporary" workaround and should be thought of as such. The only question
> is how soon will we get SW?

Killing system-messages will takes time.

In the meantime this stuff is needed to validate the NGA. I assume this work falls into under system team.

The only question is who from this team is going to work on it ? :)

Comment 50

[Michael Henretty [:mikehenrty][:mhenretty]]

Vivien or Julien, can you give some more context about what we will need here to support the new architecture? I understand something here is blocking that work, can you be specific?

So far I see we need comment 47:
> we also need to somehow deliver if the app is opened in another page.

And sadly, ignoring hashes probably won't cut it for that. What else do we need?

Comment 51

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

Ken, is there someone in Taipei that could help this?

Comment 52

[[:fabrice] Fabrice Desré]

tbh, I can probably take it if someone write a *clear* set of expected behavior. Digging through comments and comments on comments is a bit hard here.

Comment 53

[Julien Wajsberg [:julienw]]

OK, I'll try to summarize the proposals for how I think it should work. I also agree with the comments that system messages should rather work with service workers (but in that case we need really different proposals). Feel free to go this route if you think it's better.

My favorite proposal is proposal 3 from comment 41:

* if the app has an opened window and the URL is the one registered for this system message:
    - send the system message to this window

* if the app has an opened window
  AND if one of the opened window has a JS handler registered for this system message:
    - send the system message to this window

* otherwise:
  o launch a new (background) window with the system message's endpoint URL
  o send the system message to this window



All other proposals change the URL of an existing window at some point, and the more I think of it, the more I think it's not what we want -- especially if the URL change is more than a hash change.


Here is a simpler proposal adapted from Michael's comment 48 to remove the URL change:

* If the app is opened, and there is a system message handler for this message
    - send it

* otherwise:
    - create background window with message url, and send it

That means we ignore the endpoint information if the app is opened and there is a system message handler. If this works for you this works for me too.

Comment 54

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Julien Wajsberg [:julienw] from comment #53)
> Here is a simpler proposal adapted from Michael's comment 48 to remove the
> URL change:
> 
> * If the app is opened, and there is a system message handler for this
> message
>     - send it
> 
> * otherwise:
>     - create background window with message url, and send it

This works for me too, and seems the most simple.

The one problem I see is the following scenario:
1.) an app is in the foreground without registering the handler
2.) system message comes in for that app
3.) system creates new window to handle the system message
4.) that window calls app.launch()

Here the user will experience something jarring similar to if we automatically navigated their current window. But perhaps it's the least of the evils we are dealing with here.

Comment 55

[Julien Wajsberg [:julienw]]

Good point, what should happen at the 4th step?

In the SMS app we call "app.launch()" in 3 cases:
* we receive a class-0 SMS
* the user taps the notification (comes from either the onclick handler or the 'notification' system message)

In all these cases I think we want the new window to be open, not the old one. But this is in a several-windows-per-app world. I don't know how possible this is now.

For the case of the user tapping the notification we currently display a warning if the user had a "dirty" composer. For the case of the class-0 SMS this is to display a dialog so once the dialog is dismissed we're back at the previous state. None of this is possible if we navigate to a new URL...

Comment 56

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Sean Lin [:seanlin] from comment #25)
> mhenretty: In response to the following concern
> 
> "The system message _should_ still fire directly from gecko to the
> registered handler in the running app [1]. Perhaps the page URL check right
> above is not "hash" aware? [2]"
> 
> Actually it's hash aware. The URL check at [2] is to see if there's any
> listener associated with the pageURL of the app (recognized by manifestURL).
> If all of the pages have no registered handler, it implies the target app is
> not running and then follows step 2 and 3 in comment 24. 
> 
> On the other hand, after the message is sent at [1], the
> SystemMessageManager instance on the child process (app) gets it [3] and
> eventually fires the registered handler [4]. (The SystemMessageInternal
> instance is running on the parent process.)
> 
> In addition, the number of listeners for a (pageURL, manifestURL) tuple used
> at [2] is only adjusted when an app is launched or killed. That is, every
> time when an app is launched, the SystemMessageManager instance on the app
> process send a system message to the SystemMessageInternal instance on the
> parent process [5]. And the pageURL is determined by the associated window
> [6]. For example, if the user open SMS activity by sharing a picture from
> gallery, the page URL would be
> app://sms.gaiamobile.org/index.html#activity-share; whereas it's
> app://sms.gaiamobile.org/index.html if the user launches SMS app from home
> screen. Likewise, similar logic happens when an running app is killed.
> 
> Hope this would help clarify the bug. And it would help a lot if some
> reproducible steps could be provided.
> 
> 1.)
> http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/
> SystemMessageInternal.js#l628
> 2.)
> http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/
> SystemMessageInternal.js#l613
> 3.)
> http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/
> SystemMessageManager.js#l203
> 4.)
> http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/
> SystemMessageManager.js#l93
> 5.)
> http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/
> SystemMessageManager.js#l314
> 6.)
> http://hg.mozilla.org/mozilla-central/file/b227a707080f/dom/messages/
> SystemMessageManager.js#l272

I read the code and found another potential issue. SystemMessageInternal seems to store the page by the initial page url as its key. So this means if there is a page changes its url by pushstate change (inner window id is not changed) from a.html(not registered to listen to system message) to b.html(registered to listen to system message), SMI won't work because it does not know.

Comment 57

[Ken Chang[:kenkai]]

Hi Hsinyi, is there anyone from your team interested in working on this bug.

Comment 58

[Julien Wajsberg [:julienw]]

FYI I intend to work on this starting this week, but taking a totally different path.

Instead of trying to fix the current implementation, which would be really tricky (especially not regressing all apps), I intend to implement receiving system messages in service workers so that there is no way to regress existing apps but still provide a way to modern apps to have serveral pages.

Comment 59

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Julien Wajsberg [:julienw] from comment #58)
> FYI I intend to work on this starting this week, but taking a totally
> different path.
> 
> Instead of trying to fix the current implementation, which would be really
> tricky (especially not regressing all apps), I intend to implement receiving
> system messages in service workers so that there is no way to regress
> existing apps but still provide a way to modern apps to have serveral pages.

That's great, Julien.

I had a short chat with Henry about your proposal that makes sense to us, too. Kind information,the work might be that 1) to make sure every service or utility API SystemService uses works upon a worker thread, and 2) to have a new way to identify a service worker who registers a handler as now we use a combination of window, appId, ...

Comment 60

[Julien Wajsberg [:julienw]]

We won't have service workers so I'll work on implementing the last proposal in comment 53 and find a proper behavior for what Michael said in comment 55.

Comment 61

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Julien Wajsberg [:julienw] from comment #60)
> We won't have service workers so I'll work on implementing the last proposal
> in comment 53 and find a proper behavior for what Michael said in comment 55.

What about broadcasting the message to all views (or a set of views listed in the manifest) as we discussed?

Comment 62

[Julien Wajsberg [:julienw]]

(In reply to Fernando Jiménez Moreno [:ferjm] (PTO July 27th - July 31th and Aug10th - Aug 24th) from comment #61)
> (In reply to Julien Wajsberg [:julienw] from comment #60)
> > We won't have service workers so I'll work on implementing the last proposal
> > in comment 53 and find a proper behavior for what Michael said in comment 55.
> 
> What about broadcasting the message to all views (or a set of views listed
> in the manifest) as we discussed?

Yeah, I thought a little more about this, looked at the code, and it seems to me it's not easier and more risky. Then I looked further to what we said here and I thought it was better, so that's why I follow this direction now.

Preliminary work:
Gecko (preliminary structure change only, same behavior as before for now): https://github.com/mozilla/gecko-dev/compare/master...julienw:818000-system-messages
Gaia (just started): https://github.com/mozilla-b2g/gaia/compare/master...julienw:818000-system-messages

Comment 63

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #55)
> Good point, what should happen at the 4th step?
> 
> In the SMS app we call "app.launch()" in 3 cases:
> * we receive a class-0 SMS
> * the user taps the notification (comes from either the onclick handler or
> the 'notification' system message)
> 
> In all these cases I think we want the new window to be open, not the old
> one. But this is in a several-windows-per-app world. I don't know how
> possible this is now.

In the current patch, when the app calls "app.launch()", it always launches the "launch_path".
* If a window is not opened with this URL, then we open a new window, even if there is an existing window for this app.
* If there is an existing window with this URL, we reuse it.

I'll need to test various cases that we use in SMS to see if this works for us. Especially in the case of the user tapping a notification, we need to be able to enter the right thread. There are 2 things I want to try: "window.open", and registering the "notification" system message on the main window, but the "sms-received" system message on a separate window.

Comment 64

[BMO Automation]

Firefox OS is not being worked on