Closed Bug 782489 Opened 12 years ago Closed 12 years ago

B2G SMS: Use system messages to notify incoming sms events

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Project Flags:
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: cjones, Assigned: hsinyi)

References

Details

(Whiteboard: [LOE:S] [WebAPI:P0])

Attachments

(1 file)

patch
1.65 KB, patch
philikon
: review+
sicking
: superreview+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #782488 +++

We're currently forced to load a background service to work around this.  That adds startup and memory overhead, and is extremely fragile --- if the service dies, from memory pressure e.g., we can miss incoming sms events.  (Even if we auto-restart it there are still pernicious race conditions.)

We should use system messages for this.  It's what they're designed for! :)
This will depend on bug 781127 so System app window manager can make proper judgement as the message came (we should not launch/bring the sms app to the foreground in this case).

Steve, please make sure that SMS app devs know this change of plan.
Depends on: 781127
No longer depends on: 782488
Hi all!, Could you explain more the new behaviour? Because I would like to estimate the impact of changing it for V1 and if it's feasible... Other question, changing it will change the event registration paradigm?  Because related with the comment https://bugzilla.mozilla.org/show_bug.cgi?id=782488#c5 I have some doubts about what is the flow of the event in a 'incoming' message. Thanks!
This isn't something we're planning on doing for v1, right? If it is, please don't hesitate to nominate for blocking.
blocking-basecamp: --- → -
blocking-kilimanjaro: --- → +
What blocks is either
 - implement this
 - have a plan for the SMS service/app crashing

I suspect implementing this is considerably easier.  It also saves the memory we burn on the service.

Input welcome.
blocking-basecamp: - → ?
There is also onsent/ondelivered events in SmsManager. Are we going to replace them with System Message as well? By "replace", does it mean we'll remove these event targets and the only way the get SMS incoming event is through System Message?
There's a few things that we need to know before we can make a decision on if we're going to do this for v1.

Does anyone have any idea about how much memory we are using by keeping the sms app running in the background at all times?


Justin: How much work would it be to add to the mozapp API (which is the same as the mozbrowser API, right?) to get notifications when an app crashes so that gaia can restart the telephony app any time it crashes.
(In reply to Jonas Sicking (:sicking) from comment #6)
> Justin: How much work would it be to add to the mozapp API (which is the
> same as the mozbrowser API, right?) to get notifications when an app crashes
> so that gaia can restart the telephony app any time it crashes.

I thought we already had something like that.
Also note, the question isn't well-formed because we're discussing eliminating the gaia "telephony/sms service" in exchange for doing the work in gecko.
FYI, I've submit a pull request in Gaia to relaunch bg service when it crashes

https://github.com/mozilla-b2g/gaia/pull/3824
Based on comments in bug 782488, it sounds like we need to block here.  We can always un-+ if we come to a better conclusion.
blocking-basecamp: ? → +
Assignee: nobody → htsai
Depends on: 787064
Whiteboard: [LOE:S]
Attached patch patch — — Splinter Review 
Send system message 'sms-received' to notify incoming sms.
Attachment #658735 - Flags: review?(philipp)
Hsin, shouldn't you remove the DOM event if you use system messages?
Whiteboard: [LOE:S] → [LOE:S] [WebAPI:P0]
(In reply to Mounir Lamouri (:mounir) from comment #13)
> Hsin, shouldn't you remove the DOM event if you use system messages?
Hi Mounir, 
I discussed this with Vicamo. He told me that according to the conclusion he and cjones made, this issue isn't planning to change API nor to remove DOM event targets. That's why I keep DOM event even I use system messages. 

Vicamo and cjones, please correct me if I am wrong. Thanks.
I don't see the point in having a DOM Event and a System Message for the same thing. Consumers of the API might happen to handle the two events.
Chris and Vicamo, why do you guys want that?

Note that we could keep DOM Events for the non-B2G implementations though.
There's no reason sms clients must use listen to system messages.  Why overcomplicate things with special cases.
But note, I haven't looked at the system-messages API being implemented here.
Comment on attachment 658735 [details] [diff] [review]
patch

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

This looks good. Thanks!

I agree with cjones and the others that having this system message in place doesn't mean we have to get rid of the DOM event. The way I see it, system messages are a short-circuit to avoid allocating memory by having a separate background service.
Attachment #658735 - Flags: review?(philipp) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> There's no reason sms clients must use listen to system messages.  Why
> overcomplicate things with special cases.

I don't understand this: if you don't listen to system messages, there is no point in doing that. Or are you assuming this is only done so the app is opened by the system message?

Hsin, could you wait for an agreement before pushing this patch?
(In reply to Mounir Lamouri (:mounir) from comment #19)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> > There's no reason sms clients must use listen to system messages.  Why
> > overcomplicate things with special cases.
> 
> I don't understand this: if you don't listen to system messages, there is no
> point in doing that. Or are you assuming this is only done so the app is
> opened by the system message?
> 
> Hsin, could you wait for an agreement before pushing this patch?
No problem. Let's get an convincing conclusion for everybody first. :)
(In reply to Mounir Lamouri (:mounir) from comment #19)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> > There's no reason sms clients must use listen to system messages.  Why
> > overcomplicate things with special cases.
> 
> I don't understand this: if you don't listen to system messages, there is no
> point in doing that.

Why not?  Please argue from concrete uses cases.

In gaia, we already have a client of the SMS API that doesn't want to handle SMS system messages.
I don't understand what you guys want to do.

Anyway, leaving system messages and DOM events together can be wrong for two reasons:
1. you can listen to both and handle an event twice;
2. someone might register to a system message just to get the app launched will use the DOM events to handle the events which will at best be flaky and might even not work.

Which means, in my opinion, we shouldn't have system messages and DOM events for the same thing.
Version: unspecified → Trunk
Is it possible to land this code and open a different issue to discuss about sms events removal or not?
(In reply to Vivien Nicolas (:vingtetun) from comment #23)
> Is it possible to land this code and open a different issue to discuss about
> sms events removal or not?

Yes please. Move fast and land things.
Keywords: checkin-needed
Comment on attachment 658735 [details] [diff] [review]
patch

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

No. My concerns about firing two events for the same things haven't been answered. I still have no idea what the plan is.
Attachment #658735 - Flags: superreview-
(In reply to Mounir Lamouri (:mounir) from comment #25)
> Comment on attachment 658735 [details] [diff] [review]
> patch
> 
> Review of attachment 658735 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> No. My concerns about firing two events for the same things haven't been
> answered. I still have no idea what the plan is.

Can you please phrase this as a specific question?  (Or point at where it's been asked previously.)
I know that there are a lot of discussion about how to implement this issue, and about 'system messages' vs 'Background services' paradigm, but we have to focus in that we have a deadline quite close and this change has to be implemented in  Gaia's apps as well.
I think it's a great idea to have it landed without removing 'SMS Events' (so SMS/Cost Control apps will be working and we are adding 'system message' mechanism to our platform) and then we should open a new bug (not-blocking) about this issue in order to analyze it properly. 
We have only 9 days for landing this patch, making all changes in Gaia Apps that uses SMS Api (SMS App and Cost Control) and testing. IMHO I would take the option that Vivien suggested previously and then we will have time to discuss it properly. Thanks all!
(In reply to Chris Jones [:cjones] [:warhammer] from comment #26)
> (In reply to Mounir Lamouri (:mounir) from comment #25)
> > Comment on attachment 658735 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 658735 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > No. My concerns about firing two events for the same things haven't been
> > answered. I still have no idea what the plan is.
> 
> Can you please phrase this as a specific question?  (Or point at where it's
> been asked previously.)

It's not clear to the folks working on this bug what the specific concerns are here.  This request for clarification has gone unanswered for 2 days.

If your sr load is too high, please let us know and we can find someone else to follow up on the issues you see here.
Let me try to understand the arguments here. I've read through the bug, but I might definitely have missed something.

Mounir's concern is that having both the DOM Event and the system message means duplicating functionality through two APIs. Multiple ways of doing the same thing is almost always bad unless there's different advantages to the two approaches. Additionally if someone uses the system message to get woken up, but then the DOM Event to get the notification, this will be racy as there's no guaranteed order of the two mechanisms and there's no guarantee that the app starts quickly enough that it has time to register for the event before it fires.


Chris mention in comment 21 that we do have callsites which want to use DOM Events but doesn't need system messages.

It's unclear though what part of system messages that app doesn't need. I'll take a shot at a guess below.


The way system messages work right now, you have to register for a system message. This is done statically in the webapp manifest and can't be done at app-runtime.

If you are registered for a system message, you always get woken up when it's sent.

So there's no way to say "I want to listen to incoming SMS messages right now. But I'll want to stop listening for them once X happens. Once I stop listening I don't want to get woken up if there's an incoming SMS message".

I *believe* that we have an app which has the need to do that. The cost control app sends SMS messages and waits for an SMS message to get sent back. But it doesn't otherwise care about being notified about incoming text messages.

Right now I don't think there is any way for the cost control app to implement that using system messages.

It does seem like a use case that we should consider for system messages, but it doesn't exist right now. It is however possible to do if we keep the DOM Event.


*If* the above is correct, it seems like we need to keep the DOM Event for now. While I agree that having two ways to do the same thing is generally bad, it seems like we in this case fulfill the requirement that the two ways have different enough capabilities that we need both.


I do think we should file a followup on removing the DOM Event though, and add enough capabilities to system messages that the use cases can be solved using them. At least it's something we should look at to see if it would result in a cleaner API.
I think Jonas did a good summary of my concerns.
Comment on attachment 658735 [details] [diff] [review]
patch

Hi Jonas,
Per discussion with Mounir on irc, he suggested asking you to take a decision on this. 
Would you please help super review this patch? Thanks.
Attachment #658735 - Flags: superreview- → superreview?(jonas)
Comment on attachment 658735 [details] [diff] [review]
patch

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

Could you file a followup on enabling using system messages to cover the use-cases that we're using the DOM Event for? I.e. being able to receive these system messages without having to register in the manifest and thus get woken up if not running.
Attachment #658735 - Flags: superreview?(jonas) → superreview+
Blocks: 793946
No longer blocks: 793946
(In reply to Jonas Sicking (:sicking) from comment #32)
> Comment on attachment 658735 [details] [diff] [review]
> patch
> 
> Review of attachment 658735 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you file a followup on enabling using system messages to cover the
> use-cases that we're using the DOM Event for? I.e. being able to receive
> these system messages without having to register in the manifest and thus
> get woken up if not running.

I have filed a follow-up, bug 793946, for adding capabilities of system messages. Thanks!
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/473a5fa0d336
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: Use system messages to notify incoming sms events → B2G SMS: Use system messages to notify incoming sms events
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: