Closed Bug 797803 Opened 7 years ago Closed 7 years ago

System Message API: System App fails to receive system messages

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
This should also be a basecamp-blocker because it blocks bug 796670.
blocking-basecamp: --- → ?
Blocking based on comment #1.
blocking-basecamp: ? → +
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).
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.
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. ;)
Assignee: nobody → clian
Summary: System Message API: System App fails to register handler → System Message API: System App fails to receive system messages
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.
Assignee: clian → nobody
Summary: System Message API: System App fails to receive system messages → System Message API: System App fails to register handler
Assignee: nobody → clian
Summary: System Message API: System App fails to register handler → System Message API: System App fails to receive system messages
(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.
No longer blocks: 796670
blocking-basecamp: + → ?
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
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #668420 - Flags: review?(fabrice)
(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?
Re-+ing due to comment #8.
blocking-basecamp: ? → +
(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.
Attached patch Patch V2 (obsolete) — Splinter Review
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!
Attachment #668420 - Attachment is obsolete: true
Attachment #668420 - Flags: review?(fabrice)
Attachment #668721 - Flags: review?(fabrice)
(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 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.
Attachment #668721 - Flags: review?(fabrice) → review+
Attached patch Patch V2.1Splinter Review
Rebased. r=fabrice at comment 15.

Thanks Fabrice for the review!

Try: https://tbpl.mozilla.org/?tree=Try&rev=37ad90134d69
Attachment #668721 - Attachment is obsolete: true
Attachment #669389 - Flags: review+
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: Please check this in after bug 795782.
Flags: in-testsuite- → in-testsuite?
Need an updated patch from bug 795782 before this can land (please make sure this one applies cleanly after rebasing that one!)
Keywords: checkin-needed
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/47be91155e43
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 803490
Flags: in-testsuite?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.