Closed Bug 801351 Opened 12 years ago Closed 12 years ago

Remove the SMS and communications background services

Categories

(Firefox OS Graveyard :: Gaia, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia
Type:
defect

Tracking

(blocking-basecamp:+)

Status:
RESOLVED FIXED
Project Flags:
blocking-basecamp +

People

(Reporter: cjones, Assigned: etienne)

References

Details

(Whiteboard: QARegressExclude)

Attachments

(2 files, 4 obsolete files)

Test case to check how it is possible to register to sms-received without the sms permission
886 bytes, patch
Details | Diff | Splinter Review
Pointer to gaia pull request
464 bytes, text/html
vingtetun
: review+
Details 
I noticed in background_service.js that

    var backgroundOutOfProcessBlackList = [
      'Messages',

      // XXX: https://bugzilla.mozilla.org/show_bug.cgi?id=783066
      'Communications'
    ];

.  However, SMS should be using system messages now, so I don't quite understand what the background service does.

Similarly, what function does the communications background service perform?

Also, if the comm service has a purpose, then bug 783066 has been fixed for a very long time, so no need to blacklist it.
The Communications app does not use a background service anymore, so we can definitely remove it from the list.

But I'm afraid the SMS app is still using one...
SMS App is using it due to:
https://bugzilla.mozilla.org/show_bug.cgi?id=782489
There were a discussion about the usage of System Message for SMS, owing to the implications of changing it (not only for SMS App, also for 'Cost-control' one) they decided to keep the current behaviour for V1, and include the patch in Gecko for having this System Message mechanism available. Probably you could talk with Mounir or Jonas about the final decision about it.
Bug 782489 was fixed and landed.  Can you explain more about why we're not using that code?
https://bugzilla.mozilla.org/show_bug.cgi?id=782489 was landed, but as you can see in the discussion this code keeps both mechanism for notifying incoming sms (event subscription and system messages). So this patch only add a new way for being notified (we could use it or not, but is not mandatory). This is not a problem, but Cost Control works on this way. As there is no final decision (and only using system message will not work properly for Cost-Control) we have kept background services for V1.
Hi Borja,

Sorry, but I don't see any arguments here for not removing the service except for inertia.  That's a fair argument, but unfortunately memory consumption and startup time is pretty close to the #1 complaint we're getting from HW vendors right now.  Removing these useless services will save us 2-4MB, which is one of the biggest easy memory wins we have available.

We need to find resources to fix this.
Blocks: slim-fast
Summary: Do we need the SMS and communications background services? → Remove the SMS and communications background services
Note, removing the services here will be a memory win that doesn't require bug 775997.
Hi Chris,
For sure that removing background services it's a good idea (totally agree) but there were some concerns about using System Messages in SMS  (that's why there was a large discussion about it and that's why we are keeping in Backend both methods of notification). 
AFAIK if we use system messages, every time we receive a SMS we will launch all the apps that are listening this message without any policy, and 'cost control' doesn't require to be launched if a SMS is received (only needs to store data and statistics for showing it on demand). The concept of System Message (at least in my opinion) needs a review, or at least we should specify a policy for using it. For example, if you create an App which listen to 'incoming-call' system message, and this app uses the attention screen, every time you receive a call you are gonna launch two apps, dialer and our new app. In this case we are going to have a race condition  (attention screen is a resource that is used at the same time for two apps...) because we have no policy about which app should be launched or not. 
We should confirm with Backend team which is the best way of doing it, and if there is any decision about using system message or not in SMS. Because if there is, backend should remove one of the method for being notified of an incoming sms.
I'm a little unclear on what the current status of things are.

There was some discussion about if we should keep the DOM Events or not, in addition to having the system messages. We made the decision to keep the DOM Events since system messages can't yet solve all our current usecases. Specifically the Cost-control app can't use system messages.

This is the decision that we have made for v1. We might change things in v2 by improving system messages such that they can be used by the cost-control app, but that's not something we should worry about now.

But I didn't think anyone was using the background service any more. Is that not correct? I thought that the communications app used plain system messages and the cost-control app used plain DOM events.
Josh, can you help us out here?
blocking-basecamp: --- → ?
IMHO I think that the best option (for avoiding make all regression tests again, and focusing in other things that are bugs -SMS it's working with DOM Events paradigm and will be working while cost-control does-) could be opening a non-blocking bug for updating SMS App to System Message as Vivien suggested https://bugzilla.mozilla.org/show_bug.cgi?id=782489#c23 . As soon as we fix all bugs where we are working on, we will address this functionality in SMS App.
blocking-basecamp: ? → -
Triage call: We are not going to block on this bug, as this specific case is not a requirement to ship on its own merits.  Memory usage is an important issue, so we will consider a patch on a risk/reward basis, but we will not block on individual "make X better" bugs that would not hold the release on their own merits.
Assigning to Boris based on comment #10 (even though we've blocking-'d this bug).
Assignee: nobody → fbsc
This is a huge chunk of memory that we can't reclaim. I don't think we can afford not removing the background service. cjones?
Since as you say cost control only needs the SMS data on demand, why would it even try listen to incoming DOM events or system messages?  It should only look up messages to the magic number in the SMS database when the information is needed.

And then we need to add a hack in the SMS app to ignore messages from that magic number, but whatever.
Hello

I was on vacations so I did not see this thread until now.

First, one remark. Despite background black list does not include the Cost Control (CC) application, it is using one. Instead of using `background` manifest field, a "driver" in System application is in charge of loading the CC Service using window.open(), then both widget (at the bottom of the notification tray) and application (running INSIDE THE PROCESS) can communicate with the service. The service performs two main activities: do periodic tasks and keep the synchronization between widget and application.

Now, Cost Control needs SMS to check the balance and to perform the top up operation (to increase the credit). To check the balance is a periodic task (now using timers but this will be replaced by using Alarm API) so CC needs to send a query message from time to time, then it starts to listen for the response SMS and it stops to listen when a timeout value is reached.

The meat: by using system messages and registering the CC app to this message, the message will wake up the application at a moment the user does not require this. And this is a memory issue as well mainly due to two reasons:

  1. As the CC app is registered for every incoming message, the application will be opened when it is not needed forcing itself to close when the message is not intended to be received by the CC app. So it consumes memory for nothing.

  2. If the message is indeed a CC message, the application is raised for a very short **user time** or when it was not even needed (ok, you can argue B2G will close it in the future but this implicates other problems related with its INSIDE THE PROCESS nature).
I don't understand why the cost control application needs to listen to incoming SMSes.  Can someone explain that?

If we make the cost control application *not* listen to incoming SMSes, then that resolves everyone's concerns.  Right?
As said before, CC app needs to send a credit request every hour, then it waits for the response SMS and parse it in order to get the current credit.
It doesn't need to immediately parse the response.  It only needs to parse it when the value needs to be shown somewhere.  To do so, all it needs to do is query the SMS database.
Just so we're all on the same page, here's my proposal
 - stop having the communications server listen for incoming SMS
 - have only the SMS app listen the incoming-sms system message
 - have cost control send its "poll usage data" message off an alarm (sounds like this is already planned)
 - when cost control UI needs latest usage data, query the SMS DB for messages received from the magic number
 - add a hack to the SMS app to ignore messages received from the magic number
 - remove the parts of the "control control service" and "SMS service" that are always loaded

This gets the code into a state where both can be moved OOP, but that's orthogonal.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> Just so we're all on the same page, here's my proposal
>  - stop having the communications server listen for incoming SMS
>  - have only the SMS app listen the incoming-sms system message

Just a question here. AFAIK, if we register our application to listen for 'sms-received', we will launched every time a message is received. What happen with the SMS permission?

>  - have cost control send its "poll usage data" message off an alarm (sounds
> like this is already planned)

This is planned. Ok.

>  - when cost control UI needs latest usage data, query the SMS DB for
> messages received from the magic number

Technically possible. This can introduce a little issue: for testing, CC SMSes are not being deleted after parsing but it is a must if don't want to saturate the SMS database. With the new approach I can not distinguish what of those messages were queried by CC app or what of them were queried by the user.

>  - add a hack to the SMS app to ignore messages received from the magic
> number

This concurs in another problem. The black-list of SMS application is the white-list of CC application. Note what amount of useless work we are doing here:

- Each time a message is received, both CC and SMS applications will wake and load. The CC application will kill itself and the SMS application will be launched every time the message is not a credit message. Or, the CC applications stands and the SMS application kill itself every time the SMS is a credit-related one (hourly with the current implementation).

>  - remove the parts of the "control control service" and "SMS service" that
> are always loaded

I'm going to talk with Vivien about this today. This is not as simple as it could seem and it is related with https://bugzilla.mozilla.org/show_bug.cgi?id=799634

> 
> This gets the code into a state where both can be moved OOP, but that's
> orthogonal.

As said, for Cost Control is more coupled that it seems at first glance.
Let me make sure you understand what this work is intended to buy us.  It's not a semantic refactoring, for semantics' sake.  It's to save a constant locked 2-4MB of memory in the main b2g process, that can never be freed up.  Relatedly, because the SMS service is so coupled currently with the cost control application, it's additionally to save 10MB+ locked up by the SMS app when it's loaded in process, which can never be freed up.

Memory usage is one of the two biggest concerns (along with startup time) from downstream partners about the b2g v1 product.

(In reply to Salvador de la Puente González [:salva] from comment #20)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> > Just so we're all on the same page, here's my proposal
> >  - stop having the communications server listen for incoming SMS
> >  - have only the SMS app listen the incoming-sms system message
> 
> Just a question here. AFAIK, if we register our application to listen for
> 'sms-received', we will launched every time a message is received. What
> happen with the SMS permission?
> 

I don't understand this question.

> >  - when cost control UI needs latest usage data, query the SMS DB for
> > messages received from the magic number
> 
> Technically possible. This can introduce a little issue: for testing, CC
> SMSes are not being deleted after parsing but it is a must if don't want to
> saturate the SMS database. With the new approach I can not distinguish what
> of those messages were queried by CC app or what of them were queried by the
> user.
> 

I don't understand this.  Are SMS's to magic number ever deleted by CC currently or not?

> - Each time a message is received, both CC and SMS applications will wake
> and load.

Nope.  CC never receives incoming SMS.

> > 
> > This gets the code into a state where both can be moved OOP, but that's
> > orthogonal.
> 
> As said, for Cost Control is more coupled that it seems at first glance.

The SMS service and cost control are already tightly coupled.  If they weren't, we would have no work to do here! :)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> Let me make sure you understand what this work is intended to buy us.  It's
> not a semantic refactoring, for semantics' sake.  It's to save a constant
> locked 2-4MB of memory in the main b2g process, that can never be freed up. 
> Relatedly, because the SMS service is so coupled currently with the cost
> control application, it's additionally to save 10MB+ locked up by the SMS
> app when it's loaded in process, which can never be freed up.
> 
> Memory usage is one of the two biggest concerns (along with startup time)
> from downstream partners about the b2g v1 product.
> 
> (In reply to Salvador de la Puente González [:salva] from comment #20)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> > > Just so we're all on the same page, here's my proposal
> > >  - stop having the communications server listen for incoming SMS
> > >  - have only the SMS app listen the incoming-sms system message
> > 
> > Just a question here. AFAIK, if we register our application to listen for
> > 'sms-received', we will launched every time a message is received. What
> > happen with the SMS permission?
> > 
> 
> I don't understand this question.

Suppose I'm registered to 'sms-received' but I don't declare SMS permission on the manifest. Am I allowed to handle input messages? If so, there is a semantic inconsistency because I have not the SMS permission.

> 
> > >  - when cost control UI needs latest usage data, query the SMS DB for
> > > messages received from the magic number
> > 
> > Technically possible. This can introduce a little issue: for testing, CC
> > SMSes are not being deleted after parsing but it is a must if don't want to
> > saturate the SMS database. With the new approach I can not distinguish what
> > of those messages were queried by CC app or what of them were queried by the
> > user.
> > 
> 
> I don't understand this.  Are SMS's to magic number ever deleted by CC
> currently or not?

Currently, CC is not deleting any message but it should do it to prevent filling the SMS database. Do you understand?

> > - Each time a message is received, both CC and SMS applications will wake
> > and load.
> 
> Nope.  CC never receives incoming SMS.

Ok. I see, then only SMS application.

> 
> > > 
> > > This gets the code into a state where both can be moved OOP, but that's
> > > orthogonal.
> > 
> > As said, for Cost Control is more coupled that it seems at first glance.
> 
> The SMS service and cost control are already tightly coupled.  If they
> weren't, we would have no work to do here! :)

No, the problem is not with the SMS application. The problem is related with the synchronization between the CC widget on the notification tray and the CC application. Coupling is with System app not with SMS.
I understand what is the goal for this. Indeed, the problem with CC is not to remove the DOM API for SMS. Your suggestion is possible and I think there should be some walk around to handle the problem of deleting useless SMS.

My concern is more related with turning CC into OOP. I see the integrity, security and memory implications of keeping CC not OOP but I think this is another different problem than that we are discussing now.
(In reply to Salvador de la Puente González [:salva] from comment #22)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> > (In reply to Salvador de la Puente González [:salva] from comment #20)
> > > (In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> > > > Just so we're all on the same page, here's my proposal
> > > >  - stop having the communications server listen for incoming SMS
> > > >  - have only the SMS app listen the incoming-sms system message
> > > 
> > > Just a question here. AFAIK, if we register our application to listen for
> > > 'sms-received', we will launched every time a message is received. What
> > > happen with the SMS permission?
> > > 
> > 
> > I don't understand this question.
> 
> Suppose I'm registered to 'sms-received' but I don't declare SMS permission
> on the manifest. Am I allowed to handle input messages? If so, there is a
> semantic inconsistency because I have not the SMS permission.
> 

No.

> > 
> > > >  - when cost control UI needs latest usage data, query the SMS DB for
> > > > messages received from the magic number
> > > 
> > > Technically possible. This can introduce a little issue: for testing, CC
> > > SMSes are not being deleted after parsing but it is a must if don't want to
> > > saturate the SMS database. With the new approach I can not distinguish what
> > > of those messages were queried by CC app or what of them were queried by the
> > > user.
> > > 
> > 
> > I don't understand this.  Are SMS's to magic number ever deleted by CC
> > currently or not?
> 
> Currently, CC is not deleting any message but it should do it to prevent
> filling the SMS database. Do you understand?
> 

Yes.  So, that's an orthogonal bug then, right?

Although, I thought we might be keeping old messages around for a billing cycle or so to display historical graphs of data usage.  But still, that's an orthogonal bug.
(In reply to Salvador de la Puente González [:salva] from comment #23)
> My concern is more related with turning CC into OOP. I see the integrity,
> security and memory implications of keeping CC not OOP but I think this is
> another different problem than that we are discussing now.

I'm not sure that I understand this.  Can you explain more about what you're worried about?
(In reply to Salvador de la Puente González [:salva] from comment #22)
> Suppose I'm registered to 'sms-received' but I don't declare SMS permission
> on the manifest. Am I allowed to handle input messages? If so, there is a
> semantic inconsistency because I have not the SMS permission.

You can't even register to "sms-received" if you do not have SMS permission. To register to that event, you need to have access to navigator.mozSms which will not be the case if the app doesn't have the SMS permission.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> Just so we're all on the same page, here's my proposal
>  - stop having the communications server listen for incoming SMS
>  - have only the SMS app listen the incoming-sms system message
>  - have cost control send its "poll usage data" message off an alarm (sounds
> like this is already planned)
>  - when cost control UI needs latest usage data, query the SMS DB for
> messages received from the magic number

What problem are you trying to solve with the above? Note that the cost control app can simply use a DOM event to be notified any time a SMS message is received. That way the cost control app doesn't get woken up every time there's an incoming SMS, but if it's already running it can update it's current state.

On startup it could either use the latest stored value since that will only be wrong if the cost control app was closed/crashed in the short timespan between when it sent a "please tell me remaining money" message and the resulting "here's how much money is left" message. And it would only be out-of-date until the next update.

I believe that this is how the cost control app already works, but I'm not fully sure about that.

>  - add a hack to the SMS app to ignore messages received from the magic
> number

This sounds ok to me. It could even delete it from the database right away if the do the above.

>  - remove the parts of the "control control service" and "SMS service" that
> are always loaded

Agreed.
(In reply to Jonas Sicking (:sicking) from comment #27)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> >  - when cost control UI needs latest usage data, query the SMS DB for
> > messages received from the magic number
> 
> What problem are you trying to solve with the above?

"How do I get usage data if I'm not listening to every incoming SMS."

> Note that the cost
> control app can simply use a DOM event to be notified any time a SMS message
> is received. That way the cost control app doesn't get woken up every time
> there's an incoming SMS, but if it's already running it can update it's
> current state.
> 
> On startup it could either use the latest stored value since that will only
> be wrong if the cost control app was closed/crashed in the short timespan
> between when it sent a "please tell me remaining money" message and the
> resulting "here's how much money is left" message. And it would only be
> out-of-date until the next update.
> 

This is all fine, and mostly orthogonal I think.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
> > What problem are you trying to solve with the above?
> 
> "How do I get usage data if I'm not listening to every incoming SMS."

My suggested solution only makes the cost control app listen to every incoming SMS as long as the cost control app is running. If that is not acceptable, could you elaborate on why?
(In reply to Jonas Sicking (:sicking) from comment #27)
> I believe that this is how the cost control app already works, but I'm not
> fully sure about that.

Yes, exactly. The only problem is now the CC application is not OOP and it's started with system and kept opened. This is why I'm discussing with Vivien.

> This sounds ok to me. It could even delete it from the database right away
> if the do the above.

Now is working in this way. There is a blacklist.

> >  - remove the parts of the "control control service" and "SMS service" that
> > are always loaded
> 
> Agreed.

This is part of https://bugzilla.mozilla.org/show_bug.cgi?id=799634

Solution proposed by [:cjones] to remove listeners is technically acceptable (this does not mean it is feasible for V1, it needs a double-check) but the remark introduced by [:sicking] is valid as well and a strong argument to keep this implementation for V1 at least.

Please, there are some problems with system messages, let's focus on that instead of Cost Control OOP problems. Borja and I are reviewing some issues related with system messages. I'll post a summary soon.
(In reply to Jonas Sicking (:sicking) from comment #29)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
> > > What problem are you trying to solve with the above?
> > 
> > "How do I get usage data if I'm not listening to every incoming SMS."
> 
> My suggested solution only makes the cost control app listen to every
> incoming SMS as long as the cost control app is running. If that is not
> acceptable, could you elaborate on why?

I think you were proposing the same essential thing I was plus listening to incoming SMS while cost control is running.  I don't have any evidence the latter part is important, but it doesn't seem harmful.  Hence, mostly orthogonal ;).
With this patch, `sms` permission is removed and `sms-received` system message is added. A simple callback to dump SMS information is registered. With latest build, this is completely possible:

> E/GeckoConsole(  709): Content JS LOG at app://sms.gaiamobile.org/js/background.js:6 in anonymous: {"id":1,"delivery":"received","sender":"800186","receiver":{},"body":"Asdf","timestamp":1351156177000,"read":false}

> E/GeckoConsole(  841): Content JS LOG at app://sms.gaiamobile.org/js/background.js:6 in anonymous: {"id":2,"delivery":"received","sender":"800186","receiver":{},"body":"1234","timestamp":1351156460000,"read":false}

> E/GeckoConsole(  987): Content JS LOG at app://sms.gaiamobile.org/js/background.js:6 in anonymous: {"id":1,"delivery":"received","sender":"800186","receiver":{},"body":"DEADBEEF","timestamp":1351157221000,"read":false}

Please :cjones, :mounir, can you double check this?
Oh... that is using the System Messages. Fabrice, do we have any permission checks for system messages? That scares me. That means any system message is readable by any app?
Thanks for finding this Salva!

Let's stay focused on removing the services in this bug, and work on comment 32 in another.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #34)
> [...] and work on comment 32 in another.

bug 805655

And indeed, thanks for finding this Salvador, could have been very painful to find it later.
Thanks to Borja.

He was who points me this could happen. Indeed, he discovered this because it is happening with the message for incoming calls too.
This is the only "different phone" win we have left in gaia.
Severity: normal → critical
Priority: -- → P1
Looks like if we kill the sms app we then have DOMAppRegistry getSelf **** out during the next system message callback.

will investigate a bit more when I get cell reception.
(In reply to Etienne Segonzac (:etienne) from comment #39)
> Created attachment 680231 [details] [diff] [review]
> Gaia patch to remove the SMS app background service
> 
> Looks like if we kill the sms app we then have DOMAppRegistry getSelf
> **** out during the next system message callback.
> 
> will investigate a bit more when I get cell reception.

Looks like it's working well, got all the smses, and only once, even with an app kill in the middle \o/
Attachment #680231 - Flags: review?(fbsc)

        Attachment #680231 -
        Attachment is obsolete: true

        Attachment #680231 -
        Flags: review?(fbsc)

        Attachment #680614 -
        Flags: review?(fbsc)

    
  

        Attachment #680614 -
        Flags: review?(francisco.jordano)

    
    

    
  
Comment on attachment 680614 [details] [diff] [review]
Rebased Patch - Removing the SMS background service

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

r+ just one thing, should we remove the l10n strings that we won't be using like 'alert-tones'?

Perhaps we could do that for all apps automagically.

        Attachment #680614 -
        Flags: review?(francisco.jordano) → review+

    
    

    
  
[:etienne] Could you confirm us that 'system message' will not be available for 'not certified' Apps until V2? I would like  to be sure that this 'system-message' is not gonna be used for any other 'SMS' App of a third party until V2.

    
    

    
  
(In reply to Borja Salguero [:borjasalguero] from comment #43)
> [:etienne] Could you confirm us that 'system message' will not be available
> for 'not certified' Apps until V2? I would like  to be sure that this
> 'system-message' is not gonna be used for any other 'SMS' App of a third
> party until V2.

Fabrice, any idea?

That said I'm not sure it's relevant to this patch since gecko is already sending those system messages whether the SMS app use them or not.
Flags: needinfo?(fabrice)

    
    

    
  
We don't limit system messages to certified apps. Any app can register system message handlers and we'll happily send it messages if you use the broadcast mechanism.

If you want to control which app receives a message, use sendMessage() instead. This is what the alarm and web activities APIs are doing.
Flags: needinfo?(fabrice)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Encountered 2 issues while dogfooding with this patch:
- bad contact lookup
- sometimes 2 notifications instead of one

This updated patch fixes both.
Assignee: fbsc → etienne

        Attachment #680614 -
        Attachment is obsolete: true

        Attachment #680614 -
        Flags: review?(fbsc)

        Attachment #681108 -
        Flags: review?(francisco.jordano)

        Attachment #681108 -
        Flags: review?(fbsc)
No longer blocks: 809551

    
    

    
  
Hi there, some issues.

[:fabrice] Let other third party apps being subscribed to 'system-message' would be a problem with memory comsuption. If we are removing 'background' service for saving memory, and with 'system-message' Im gonna open all apps subscribed with no criteria, memory comsuption will suffer a huge peak every time we receive a SMS... How are we going to handle it?

[:etienne] The code looks great! Only one thing. As we are moving this code from background to 'system-message' handler, could you add this fix as well? https://bugzilla.mozilla.org/show_bug.cgi?id=803935
The scenario will be that if you receive an email and your visibility is true, you dont have to create a notification. Please add this small fix and for me would be r+. Thanks for your support with SMS App!

    
    

    
  
[:etienne] Even scenario is more simpler. If you receive a SMS while you are inside the thread --> NO notifications. For other scenarios, we will show notifications keeping the code. Thanks!!
No longer blocks: 803935

    
    

    
  
(In reply to Borja Salguero [:borjasalguero] from comment #47)
> Hi there, some issues.
> 
> [:fabrice] Let other third party apps being subscribed to 'system-message'
> would be a problem with memory comsuption. If we are removing 'background'
> service for saving memory, and with 'system-message' Im gonna open all apps
> subscribed with no criteria, memory comsuption will suffer a huge peak every
> time we receive a SMS... How are we going to handle it?

As I said in comment 45, we can limit which app receives a system message. It's an SMS API design choice to broadcast or not, not an issue with the System Messages API itself. What we plan to do though, is to only let apps that have the permission to access e.g. the SMS API to receive SMS related system messages. This is tracked in bug 805655
Depends on: 805655

    
    

    
  

      
      
      
      

        Attached file
          
        Pointer to gaia PR (obsolete)
        — 
      

    


  

        Attachment #681108 -
        Attachment is obsolete: true

        Attachment #681108 -
        Flags: review?(francisco.jordano)

        Attachment #681108 -
        Flags: review?(fbsc)

        Attachment #681456 -
        Flags: review?(fbsc)

        Attachment #681456 -
        Flags: approval-gaia-master?(francisco.jordano)

    
    

    
  
Comment on attachment 681456 [details]
Pointer to gaia PR

Looking good to me, just waiting for Borja review.

As Fabrice commented, lets fix this system message thing, and in other bug we'll ensure the message can be received by the proper apps.

Thanks folks.

        Attachment #681456 -
        Flags: approval-gaia-master?(francisco.jordano) → approval-gaia-master+

    
    

    
  
Comment on attachment 681456 [details]
Pointer to gaia PR

After checking with :etienne and :fbsc i'm setting the a=- they have raised a problem with notifications.

If the app is killed the callback is gc and we cannot launch the thread from the notifications. :etienne has pointed out that is a common problem with notifications so we will need to figure out a way of solving it before merging this changes.

Cheers!

        Attachment #681456 -
        Flags: approval-gaia-master+ → approval-gaia-master-

    
    

    
  
Update: the gaia work is done, reviewed and can be found here:
https://github.com/mozilla-b2g/gaia/pull/6400

But we're facing a dilemma.

When a new SMS comes in we create a mozNotification with a click callback that basically does a mozApps.mgmt.getSelf(launch()).

In the current implementation the SMS background service does this.
- The good thing is that clicking the SMS notification always takes you to the SMS app.
- The bad thing is that the SMS background service is always running and using our precious memory

In the new implementation the SMS app creates the notification itself.
- The good thing is that we're freeing a lot of memory by removing the background service.
- The bad thing is when the SMS app is closed we're losing the notification click callback, and taping the notification will do nothing, which is a bad experience.

What should we do?
Flags: needinfo?(jones.chris.g)

        Attachment #681456 -
        Flags: review?(fbsc) → review+

    
    

    
  
Please address the comments in the PR. Thanks!

    
    

    
  
(In reply to Francisco Jordano [:arcturus] from comment #52)
> Comment on attachment 681456 [details]
> Pointer to gaia PR
> 
> After checking with :etienne and :fbsc i'm setting the a=- they have raised
> a problem with notifications.
> 
> If the app is killed the callback is gc and we cannot launch the thread from
> the notifications. :etienne has pointed out that is a common problem with
> notifications so we will need to figure out a way of solving it before
> merging this changes.
> 
> Cheers!

This is a much bigger problem than just this application actually. Dialer, Calendar and potentially email will suffer this too. So I guess we need a way to workaround that globally.

Also I don't really know why this bug has been set blocking-. Let's renom it.

This bug is about saving 6mb of RAM in a permanent fashion on the device. It also one of the 2 pieces that miss that would let us get rid of background service.
blocking-basecamp: - → ?

    
    

    
  
I agree that we are going to save 6Mb, but we cant save Mb breaking other functionalities... Do we need to open a bug for creating the 'workaround' that you mention and block this bug until the workaround will be available?

    
    

    
  
(In reply to Borja Salguero [:borjasalguero] from comment #56)
> I agree that we are going to save 6Mb, but we cant save Mb breaking other
> functionalities... 

I have not said that.

> Do we need to open a bug for creating the 'workaround'
> that you mention and block this bug until the workaround will be available?

I would like to raise this issue to some of our platform folks.

cjones, sicking, dougt?

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) from comment #55)
> (In reply to Francisco Jordano [:arcturus] from comment #52)
> > Comment on attachment 681456 [details]
> > Pointer to gaia PR
> > 
> > After checking with :etienne and :fbsc i'm setting the a=- they have raised
> > a problem with notifications.
> > 
> > If the app is killed the callback is gc and we cannot launch the thread from
> > the notifications. :etienne has pointed out that is a common problem with
> > notifications so we will need to figure out a way of solving it before
> > merging this changes.
> > 
> > Cheers!
> 
> This is a much bigger problem than just this application actually. Dialer,
> Calendar and potentially email will suffer this too. So I guess we need a
> way to workaround that globally.

Yup, totally agree, is not just SMS, is potentially, any app using the notifications.

> 
> Also I don't really know why this bug has been set blocking-. Let's renom it.

Agree again, should be a b+, but we don't have a solution yet for the problem of notifications not working when the app is killed (which happens to be an excellent name for a bug).

> 
> This bug is about saving 6mb of RAM in a permanent fashion on the device. It
> also one of the 2 pieces that miss that would let us get rid of background
> service.

:vingtetun, what about open a new bug for solving the notifications problem, putting this as dependent and let the flow go?

My only concern is if we land this right now, despite of saving 6MB (which I consider a tremendous improvement) we are 'breaking' (yes we are) a functionality, that is not in the smoke test (but could) and at this stages of the project stability is a must.

With that said, I really would like to hear the opinion of the platform gurus as well :)

Thanks!

    
    

    
  
Can't the notifications click callbacks just be closures that launch activities received by their respective app?

This sounds too simple so I'm probably missing something!

    
    

    
  
I'm very surprised that clicking notifications doesn't launch() the app that created them, and I agree with comment 55.

Can we change that easily in gaia?
Flags: needinfo?(jones.chris.g)

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #60)
> Can we change that easily in gaia?

If we can't, then we should still remove the background service.  The SMS app will be launched when it receives an SMS, and it will most likely still be running when the user taps the notification (with all the other memory savings we have lined up).  If the SMS app is killed for some reason, then the workaround is to just tap the SMS app icon to launch it.  I consider that a polish bug.

    
    

    
  
Hey guys, 

I can't follow the technical discussion all the way to the bottom, but one thing is pretty clear to me: having a broken notification that might not act on a user's tap is *totally* unacceptable.

Whatever app / system responsible for displaying a notification must make sure tapping it will *always* open the app as expected.

    
    

    
  
(In reply to Fabrice Desré [:fabrice] from comment #59)
> Can't the notifications click callbacks just be closures that launch
> activities received by their respective app?
> 
> This sounds too simple so I'm probably missing something!

I don't think we have the ability to make zombie-closures outliving the app window :)

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #60)
> I'm very surprised that clicking notifications doesn't launch() the app that
> created them, and I agree with comment 55.
> 
> Can we change that easily in gaia?

Currently Gaia doesn't know which app sent the notification, we get a mozChromeEvent looking like this:

{"type":"desktop-notification","id":"alert0","icon":"app://sms.gaiamobile.org/style/icons/60/Sms.png","title":"+336XXXXXXX","text":"Plop"}

If we add  the same |url| and |manifestURL| fields than the open-app mozChromeEvent the gaia system app could ensure that the sending app is at least launched (which will probably be acceptable UX wise).

    
    

    
  
The problem is not the callback is collected by the GC, the problem is that the memory of the process has been completely wiped out. Doesn't it?

But please, explain me something I don't understand. Suppose we were keeping the process alive, the notification object created by `mozNotification.createNotification()` has an attribute `onclick` with a callback and this callback is invoked by the system running in another process. How is this possible? In terms of low level implementation, what is a reference to a function? Some kind of pair (<number of the process>, <pointer to the function>)?

Now, returning to the discussion. Suppose we leave the background process, but with the registration of the system message handler instead of listening through the DOM API. Why this approach, which does not even open the application (so, it does not load any resource neither) can occupy 6MiB? It can not be the closure in background.js Is it the background iframe overhead? I hope no but I don't know.

With this, the user could not close the SMS application completely (he could close the SMS iframe in the card view, releasing the resources, but not the service itself). Furthermore, the OOM killer could still kill the SMS application if device is out of memory. Am I wrong?

    
    

    
  
Fabrice do you think bug 778668 can be resurected?

    
    

    
  
Launching the app itself is not enough, IMHO. We should strive to keep the notification in context and jump to the thread, email, etc. it refers to. Not doing so might feel reasonable a priori, but it'll be an utter pain in two days of use.

    
    

    
  
(In reply to Rafael Rebolleda [:rafaelrebolleda] from comment #67)
> Launching the app itself is not enough, IMHO. We should strive to keep the
> notification in context and jump to the thread, email, etc. it refers to.
> Not doing so might feel reasonable a priori, but it'll be an utter pain in
> two days of use.

I don't think creating a background service for each app using the desktop notifications API is technically reasonable either...

Launching the app sounds like a good compromise, the app can be smart and save its state in order to be launched on the good screen.

    
    

    
  
(In reply to Salvador de la Puente González [:salva] from comment #65)
> Why this approach, which does not even open the
> application (so, it does not load any resource neither) can occupy 6MiB? It
> can not be the closure in background.js Is it the background iframe
> overhead? I hope no but I don't know.
> 

The background service's iframe spins another process, which is about 6MB I've been told.

    
    

    
  
(In reply to Etienne Segonzac (:etienne) from comment #69)
> The background service's iframe spins another process, which is about 6MB
> I've been told.

First, I think we need to check this. :borja, can you check the background process really allocates such amount of memory?

Second, if every process allocates 6MiB, then I think we should change Gecko to optimize 'background' iframes because we know (it is in the `background` definition) that those windows open with flag `background` will never be shown. The only thing we need here is the JS machine but correct me I I'm wrong.
Flags: needinfo?(etienne)

    
    

    
  
(In reply to Etienne Segonzac (:etienne) from comment #69)
> The background service's iframe spins another process, which is about 6MB
> I've been told.

First, I think we need to check this. :borja, can you check the background process really allocates such amount of memory?

Second, if every process allocates 6MiB, then I think we should change Gecko to optimize 'background' iframes because we know (it is in the `background` definition) that those windows open with flag `background` will never be shown. The only thing we need here is the JS machine but correct me I I'm wrong.

    
    

    
  
There is no "good state". For SMS or email, different messages will belong in different threads, and therefore screens. The good screen would only be so if the notification happens to point, by chance, to that thread.

I'm not saying bg service per app kills me; I'm ok with the system handling it as long as functionality and UX are not hindered, which AFAIK is what will happen with the current proposal.

    
    

    
  
We need a full summary of the work required to complete this bug before setting blocking-basecamp+. 6mb is a great memory win, but if this requires any major architectural work we may not implement for v1.

    
    

    
  
(In reply to Alex Keybl [:akeybl] from comment #73)
> We need a full summary of the work required to complete this bug before
> setting blocking-basecamp+. 6mb is a great memory win, but if this requires
> any major architectural work we may not implement for v1.

Alex the architectural discussion about 'Notifications' already affects the Dialer/Calendar/Email and will affect all web apps that are installed and use notifications. So this should happen and is not related *only* to this bug. The decision to use system messages has been made a while ago so I don't think it should be discussed here.

This particular bug is about removing the background service and save us 6mb of RAM. It will also expose the notifications of the SMS app to the same issue that exists for other apps. This issue should be solved globally and blocking this bug won't resolve the issue for the other apps :)

    
    

    
  
For being consistent with our way of working we cant merge code which break any other functionality. I know the issue about saving 6Mb, but as we agree in https://bugzilla.mozilla.org/show_bug.cgi?id=801351#c57 We dont want to loose features that are currently working!  I think that this bug is blocked by the 'Notification' issue, so I would like to listen how we could fix it with our Gecko Team ;).

    
    

    
  
(In reply to Etienne Segonzac (:etienne) from comment #66)
> Fabrice do you think bug 778668 can be resurected?

I see what you did here... but yes, let me try.

    
    

    
  
(In reply to Fabrice Desré [:fabrice] from comment #76)
> (In reply to Etienne Segonzac (:etienne) from comment #66)
> > Fabrice do you think bug 778668 can be resurected?
> 
> I see what you did here... but yes, let me try.

If we can have this feature combined with bug 779895 it would be doable to create an hack to go to a specific panel.

    
    

    
  
(In reply to Salvador de la Puente González [:salva] from comment #65)
> The problem is not the callback is collected by the GC, the problem is that
> the memory of the process has been completely wiped out. Doesn't it?
>

Yep.
 
> Now, returning to the discussion. Suppose we leave the background process,
> but with the registration of the system message handler instead of listening
> through the DOM API. Why this approach, which does not even open the
> application (so, it does not load any resource neither) can occupy 6MiB? It
> can not be the closure in background.js Is it the background iframe
> overhead? I hope no but I don't know.

This is a whole process, so this is expensive.

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) from comment #74)
> This particular bug is about removing the background service and save us 6mb
> of RAM. It will also expose the notifications of the SMS app to the same
> issue that exists for other apps. This issue should be solved globally and
> blocking this bug won't resolve the issue for the other apps :)

OK thanks for the explanation - what bug is blocking this work currently? I see bug 805655 marked as a blocker, is that everything we need to do for SMS before removing the comms bg service?

    
    

    
  
I noticed that cost control also eagerly starts a process during startup. Can we make that at least lazy, and ideally not start at all except when invoked?

    
    

    
  
It's also permanently set as "foreground" so its memory will be unreclaimable in practice.  The iframe needs proper setVisible() calls like all the other app iframes.

Is there already a bug filed on that?

    
    

    
  
Hey [:cjones], [:gal],

First, check this:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/cost_control.js

Note the bindings:
  window.addEventListener('utilitytrayshow', _showWidget);
  window.addEventListener('utilitytrayhide', _hideWidget);

That set visibility.

Anyway, if you think there is still a problem, can we move comments #80 and #81 to another thread? I think this is not related with removing the SMS background but with another file such as "Mark CostControl widget iframe as foreground/background" and they can introduce noise.
Flags: needinfo?(etienne) → needinfo?(jones.chris.g)

    
    

    
  
Filed bug 813262 for problems with foregrounding/backgrounding.
Flags: needinfo?(jones.chris.g)
blocking-basecamp: ? → +

        Attachment #681456 -
        Flags: review+ → review-

    
    

    
  
As this code breaks notifications for incoming SMS and it's a 'have to/must' (check https://bugzilla.mozilla.org/show_bug.cgi?id=801351#c62), Im gonna r- until having a fix for this issue. As I agree with [:vingtetun] (https://bugzilla.mozilla.org/show_bug.cgi?id=801351#c57) we need the support of our platform folks for getting a solution for this issue, and we cant add features breaking others.

    
    

    
  
(In reply to Borja Salguero [:borjasalguero] from comment #84)
> As this code breaks notifications for incoming SMS and it's a 'have to/must'
> (check https://bugzilla.mozilla.org/show_bug.cgi?id=801351#c62), Im gonna r-
> until having a fix for this issue. As I agree with [:vingtetun]
> (https://bugzilla.mozilla.org/show_bug.cgi?id=801351#c57) we need the
> support of our platform folks for getting a solution for this issue, and we
> cant add features breaking others.

Actually the SMS notifications are already broken (see bug 813917) and the same bug for the missed call notification (bug 796493) was minused...

I really want to fix this notification issue, and Vivien may have a plan. But in the meantime if those issue are vital we should probably have blocking basecamp + bugs for them both, otherwise we should land this patch.

    
    

    
  
[:etienne] As next week we are going to work together, what about to deal with this issue (and the others related) and create a small 'roadmap' in order to have more clear who and when is gonna fix these bugs? As you know the code for me it's ok, but I would like to have clear when we are going to have all features working! (now we have in SMS less features than one week and a half ago... and trust me that Im really worried about it).

    
    

    
  
(In reply to Borja Salguero [:borjasalguero] from comment #86)
> [:etienne] As next week we are going to work together, what about to deal
> with this issue (and the others related) and create a small 'roadmap' in
> order to have more clear who and when is gonna fix these bugs? As you know
> the code for me it's ok, but I would like to have clear when we are going to
> have all features working! (now we have in SMS less features than one week
> and a half ago... and trust me that Im really worried about it).

Sounds like a plan :)

    
    

    
  
Count with me, please! :)

    
  
Blocks: 815384

    
    

    
  
(In reply to Borja Salguero [:borjasalguero] from comment #84)
> As this code breaks notifications for incoming SMS and it's a 'have to/must'
> (check https://bugzilla.mozilla.org/show_bug.cgi?id=801351#c62), Im gonna r-
> until having a fix for this issue. As I agree with [:vingtetun]
> (https://bugzilla.mozilla.org/show_bug.cgi?id=801351#c57) we need the
> support of our platform folks for getting a solution for this issue, and we
> cant add features breaking others.

This will be fixed by the underlying platform work we are doing with Fabrice. this patch should not wait any longer since it already blocks 4 other blockers and landing it won't regress anything since the notification is already broken because of activity.

    
    

    
  


  
Okay here it is!

What's new:
This PR contains the work needed to fix the notification support (currently not working).

It will be fully functional once the bug 778668 and 814074 land.

In the meantime it fixes memory issues for the SMS app and various notifications-related blockers.

Asking :vingtetun for review since the changes from the last PR are only system/notifications related.

        Attachment #681456 -
        Attachment is obsolete: true

        Attachment #685658 -
        Flags: review?(21)

    
    

    
  
Comment on attachment 685658 [details]
Pointer to gaia pull request

Sounds good to me. Let's fix all those C2 issues that will be fixed by this patch and that broke smoke tests...

The notification issue (which does not depends on this bug) will be fixed by the underlying platform work - which will also fix the dialer/others notifications!

        Attachment #685658 -
        Flags: review?(21) → review+

    
  
Whiteboard: QARegressExclude

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: