797803 - System Message API: System App fails to receive system messages

Status: RESOLVED FIXED

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

Description

[Gene Lian (I already quit Mozilla)]

Please refer to Bug 796670. If we call navigator.mozSetMessageHandler() from system app which is launched super-early when booting up, we would get the following order:

1. SystemMessageManager.init() attempts to send an async message "SystemMessageManager:Register" to parent (well, for a system app child process is just a parent process).

2. SystemMessageInternal cannot receive #1 because it has not yet done the process of adding the message listener for "SystemMessageManager:Register".

3. As a result, the system app process target cannot be saved in the SystemMessageInternal._listeners[].

4. Later, for any system messages (like "alarm") going to be sent to system app will fail.

Comment 1

[Gene Lian (I already quit Mozilla)]

This should also be a basecamp-blocker because it blocks bug 796670.

Comment 2

[Andrew Overholt [:overholt]]

Blocking based on comment #1.

Comment 3

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

Fabrice, Gene, 

What if we load system app (shell.js L189) only until we are getting |webapps-registry-ready| observer message (shell.js L466), does that solve the issue? That would get rid of one ChromeEvent too :P The only thing I would be worrying is that it will increase the boot up time (a bit, might still acceptable).

Comment 4

[[:fabrice] Fabrice Desré]

Tim,

We can try that, but that would make the startup time really longer. Probably several seconds on device. Please test and come back with numbers!
I have a planned improvement to startup time (when registering activities) but I'm not sure when I'll have time to work on that.

Comment 5

[Gene Lian (I already quit Mozilla)]

In the back-end, we've already buffer system messages until |webapps-registry-ready| is received (bug 795209). I don't think it's a good idea to further use it to decide whether to load apps or not. As Tim's mention, they would probably increase the booting-up time.

I will have a patch for this later. ;)

Comment 6

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

Hum, base on the comment 0, I think I can workaround this bug in Gaia by running |navigator.mozSetMessageHandler| all the way till I really needed it ... that would help right? 

/me rewriting my patch and try right now.

Comment 7

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #6)
> Hum, base on the comment 0, I think I can workaround this bug in Gaia by
> running |navigator.mozSetMessageHandler| all the way till I really needed it
> ... that would help right? 
> 
> /me rewriting my patch and try right now.

Good news! This indeed works. Let's de-dependent the bugs re-review the blocking decision.

Comment 8

[Eric Chou [:ericchou] [:echou]]

This will block Bluetooth file-sending function. We use system message to notify system app (notification center) that a remote device would like to send a file to us. 

Gaia bug: https://github.com/mozilla-b2g/gaia/issues/3441

Comment 9

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Fabrice,

Could you please review this patch when you have a chance? To consider the existence of System App, two cases are valid to register the manifest from the SystemMessageManager process:

  1. This is asked by a child process (parent process must be ready).

  2. Parent process has already constructed the |SystemMessageInternal|.

Otherwise, delay to do it until the |SystemMessageInternal| is ready (SystemMessageManager will be notified by the |"system-message-internal-ready"| observer message from |SystemMessageInternal|).

Please let me know if your have any questions or suggestions.

Comment 10

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #8)
> This will block Bluetooth file-sending function. We use system message to
> notify system app (notification center) that a remote device would like to
> send a file to us. 
> 
> Gaia bug: https://github.com/mozilla-b2g/gaia/issues/3441

Eric, Ian, Evelyn,

I thought it is the Bluetooth Sharing app that is going to receive the initial send-a-file request, and system message that goes to System app is the only about the progress to show on the status bar? If so the flow should not by affected by this bug.

Even if the send-a-file request is indeed handle by the System app, according to Gene this should be easily workaround in System app by defering |navigator.mozSetMessageHandler()| till |webapps-registry-ready| or |applicationready| event. Is that correct?

Comment 11

[Andrew Overholt [:overholt]]

Re-+ing due to comment #8.

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #10)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #8)
> > This will block Bluetooth file-sending function. We use system message to
> > notify system app (notification center) that a remote device would like to
> > send a file to us. 
> > 
> > Gaia bug: https://github.com/mozilla-b2g/gaia/issues/3441
> 
> Eric, Ian, Evelyn,
> 
> I thought it is the Bluetooth Sharing app that is going to receive the
> initial send-a-file request, and system message that goes to System app is
> the only about the progress to show on the status bar? If so the flow should
> not by affected by this bug.

Yes, an alternative is to let Bluetooth Sharing App handle the system message since System App might still have problem that needs to be fixed in this bug. In summary, all the non-System App should be able to receive the system messages.

> 
> Even if the send-a-file request is indeed handle by the System app,
> according to Gene this should be easily workaround in System app by defering
> |navigator.mozSetMessageHandler()| till |webapps-registry-ready| or
> |applicationready| event. Is that correct?

Yes. If you still hope to handle system messages in the System App, you need to wait on those events. However, this solution is a kind of workaround. In my opinion, it would be better to fix that; otherwise, we will have special hard-codes in System App from the point of view of Gaia.

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Patch V2

Find a better a way to do this without modifying shell.js. Please see comment 9 for the summary.

Thanks Fabrice again for the review!

Comment 14

[Eric Chou [:ericchou] [:echou]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #10)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #8)
> > This will block Bluetooth file-sending function. We use system message to
> > notify system app (notification center) that a remote device would like to
> > send a file to us. 
> > 
> > Gaia bug: https://github.com/mozilla-b2g/gaia/issues/3441
> 
> Eric, Ian, Evelyn,
> 
> I thought it is the Bluetooth Sharing app that is going to receive the
> initial send-a-file request, and system message that goes to System app is
> the only about the progress to show on the status bar? If so the flow should
> not by affected by this bug.
> 

According to the UX spec, we are going to have a file-sending progress bar in notification center but not bluetoot file-sharing app. If this is the case, we will still need a solution to get progress-update system message in System app. Please feel free to tell me if I realized wrong. :)

> Even if the send-a-file request is indeed handle by the System app,
> according to Gene this should be easily workaround in System app by defering
> |navigator.mozSetMessageHandler()| till |webapps-registry-ready| or
> |applicationready| event. Is that correct?

No idea about this, will let experts answer. :)

Comment 15

[[:fabrice] Fabrice Desré]

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

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

::: dom/messages/SystemMessageManager.js
@@ +219,5 @@
> +      if (ready.length == 0 || !ready[0]) {
> +        readyToRegister = false;
> +      }
> +    }
> +    if (readyToRegister)

Nit: { } even for one-line if blocks.

Comment 16

[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 17

[Ryan VanderMeulen [:RyanVM]]

Need an updated patch from bug 795782 before this can land (please make sure this one applies cleanly after rebasing that one!)

Comment 18

[Gene Lian (I already quit Mozilla)]

(In reply to Ryan VanderMeulen from comment #17)
> Need an updated patch from bug 795782 before this can land (please make sure
> this one applies cleanly after rebasing that one!)

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

Reminder: please check in bug 795782 first before this.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47be91155e43

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/47be91155e43

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4121f31e6c04