Closed Bug 782488 Opened 12 years ago Closed 12 years ago

Use system messages to notify incoming telephony 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

Attachments

(3 files)

Full data
1.56 KB, text/plain
Details
Patch: use system message to notify incoming calls
1.20 KB, patch
philikon
: review+
Details | Diff | Splinter Review
Patch: gaia test (test only, won't be submitted)
1.25 KB, patch
Details | Diff | Splinter Review 
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 telephony 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 call came (we should not launch/bring the dialer app to the foreground in this case).

Etienne, do you aware of this change?
Depends on: 781127
No I wasn't, thanks for pointing it out.

It will have a pretty big impact on the dialer and the emergency call support:
Currently all calls (incoming, outgoing, emergency) go through the background service, allowing us to use the same call screen for all of them.

I'll probably start a gaia branch for this as soon as a working patch comes.
This shouldn't affect the dialer UI; the goal here is to move what the background service is doing into gecko itself.

Otherwise, we need to add a lot new hacks to deal with the service and dialer crashing.
As I said, the dialer background service is not handling *only* incoming calls. And the goal is to get rid of it. Thus the gaia dialer app will have to change quite a bit, the emergency call support too (currently uses the dialer background service).

FTR I'm all for it, just pointing out that the work on the gaia side should start early because it won't be trivial.
I'm not 100% sure how we can make this work. The System Messages API is *not* a broadcasting API, so we need to know in the first place which app to send the message to. Maybe using an activity would work, but that does not prevent other apps from registering for these events.
(In reply to Fabrice Desré [:fabrice] from comment #5)
> I'm not 100% sure how we can make this work. The System Messages API is
> *not* a broadcasting API, so we need to know in the first place which app to
> send the message to.

That was how I imagined this working.
Assignee: nobody → htsai
Depends on: 776832
I don't think that depends on bug 776832 which is about the low-level message manager, not the system messages API.
(In reply to Fabrice Desré [:fabrice] from comment #7)
> I don't think that depends on bug 776832 which is about the low-level
> message manager, not the system messages API.
Hm, I might misunderstand the scope of Bug 776832. Thanks for pointing this out. 

Per my understanding, system messages API is designed to send messages from chrome process, right? So, what I thought was we need that kind of helper to check app privilege in chrome process first before sending system messages to them.
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: --- → +
See comment 2 and comment 3.
blocking-basecamp: - → ?
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 telephony 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.
> 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.

Zero; it's already done.  :)  Bug 766437.
We should probably also tweak the telephony app's oom_adj so that it is preferentially saved from being killed on OOM.  Right now, all background apps have the same oom_adj.  If someone files a bug on that, I can handle it.
(In reply to Jonas Sicking (:sicking) from comment #11)
> 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
> telephony app running in the background at all times?
> 

Based on dhylands's data, it's ~20MB.  This is ~20% of all memory available to apps.

But note, this isn't the "telephony/sms service", which I don't understand very well.  The overhead of that is up to 10MB based on my measurements.

(In reply to Justin Lebar [:jlebar] from comment #13)
> We should probably also tweak the telephony app's oom_adj so that it is
> preferentially saved from being killed on OOM.  Right now, all background
> apps have the same oom_adj.  If someone files a bug on that, I can handle it.

I don't think that's a good idea.  Users want the app that they've put in the foreground to stay alive if at all possible.  Much of the motivation for messages/activities is to let apps die (or not load them) and be able to bring them back on demand.
> Users want the app that they've put in the foreground to stay alive if at all possible.

Well, there's no reason that we couldn't kill telephony after all background processes but before any foreground processes.

But I'm not sure that, I want my foreground app to stay alive even if that means that I can't receive calls.

If you can unload the telephony app and still receive calls, that's great.  But then why wait for OOM to kill it -- why not eagerly unload it?
(In reply to Justin Lebar [:jlebar] from comment #15)
> > Users want the app that they've put in the foreground to stay alive if at all possible.
> 
> Well, there's no reason that we couldn't kill telephony after all background
> processes but before any foreground processes.
> 

I don't think we have any evidence that users would prefer the dialer app to stay open in preference to $SOCIAL_MEDIA_APP or $SHINY_GAME.

> But I'm not sure that, I want my foreground app to stay alive even if that
> means that I can't receive calls.
> 

Well, that's a false dichotomy.

> If you can unload the telephony app and still receive calls, that's great. 
> But then why wait for OOM to kill it -- why not eagerly unload it?

Free memory is wasted memory.  Keeping the dialer in memory means we don't have to load it again if the user switches back to it.
Jonas pinged me asking if we could restart the phone app when it crashes.  I presumed that was because we really wanted it to be there.

I didn't mean for this to turn confrontational; I'm really sorry.  Please let me know if you'd like my input about anything else here.
(In reply to Justin Lebar [:jlebar] from comment #17)
> Jonas pinged me asking if we could restart the phone app when it crashes.  I
> presumed that was because we really wanted it to be there.
> 

We do want to be able to do that! :)

> I didn't mean for this to turn confrontational; I'm really sorry.  Please
> let me know if you'd like my input about anything else here.

I didn't read it as confrontational at all.  We're trying to settle on a technical plan.  Sorry if my response made it seem that way (again :( ).
Sorry, I must be really confused about what the current state of affairs is, and what it is that you want.

Currently, some background service that takes up 20mb of memory must be open in order for me to receive calls, right?  So long as that is the case, we agree that we should give this service preferential treatment so it doesn't get killed on OOM?

But what you're saying is that, in a world where the background service does not need to be running in order to receive calls (that's the false dichotomy in comment 16), we could kill the dialer app just like any other background app?  That's fine, of course.

But then I don't understand the story about pre-loading.  We want to keep the dialer app open so long as there is sufficient available memory?  If the dialer app crashes due to OOM (or whatever), you want to re-load it as soon as there is sufficient memory available (because "free memory is wasted memory")?  It's not clear to me why specifically the dialer app should get this treatment.
(In reply to Justin Lebar [:jlebar] from comment #19)
> Sorry, I must be really confused about what the current state of affairs is,
> and what it is that you want.
> 

I was fairly confused about this myself.  Did some investigation.

> Currently, some background service that takes up 20mb of memory must be open
> in order for me to receive calls, right?  So long as that is the case, we
> agree that we should give this service preferential treatment so it doesn't
> get killed on OOM?
> 

The way things work currently is
 - apps can have a "background_page" property specified in their manifest
 - if that exists, the gaia system app launches the service as <app remote=false>, *within* the b2g master process.  As root.
 - the dialer service registers to listen for incoming telephony events.  Upon receiving one, it window.open()s the in-call UI.

So on an incoming telephony event, we never launch the dialer at all.

When the dialer is launched, we load a second instance of <app src="dialer"> as remote=true.  I don't know how gecko is going to handle having <app src="dialer> loaded *both* in the master process, *and* in a content process.  I hope that doesn't affect metering etc.

Here are the problems with this setup
 - we can't run arbitrary services in the master process as root.  This means background-service must be a certified permission.  I'm somewhat ambivalent about this for v1, but it would prevent writing a third-party email app that's competitive with gaia's.
 - services running in the b2g master process can't be killed.  The memory they consume is unavailable to apps.
 - the amount of memory consumed by the dialer service is unknown, but up to 10MB.

Now, if we run the dialer service in its own app (which we would need to do for arbitrary services), the dialer service can be killed on memory pressure.  We have no plans at all for handling killed services atm, AFAIK.

My understanding previously had been that the dialer service was purely working around Gecko not being able to launch the dialer on its own.  That is, it provided no additional benefit.

However, as I look through the code more, I see that the dialer service seems to be very tightly integrated into the system app :(.

> But what you're saying is that, in a world where the background service does
> not need to be running in order to receive calls (that's the false dichotomy
> in comment 16), we could kill the dialer app just like any other background
> app?  That's fine, of course.
> 

Except for the tight integration into system UI, that's correct.

> But then I don't understand the story about pre-loading.  We want to keep
> the dialer app open so long as there is sufficient available memory?  If the
> dialer app crashes due to OOM (or whatever), you want to re-load it as soon
> as there is sufficient memory available (because "free memory is wasted
> memory")?  It's not clear to me why specifically the dialer app should get
> this treatment.

I'm saying it shouldn't be treated differently.  If it's already loaded, there's no reason to specifically protect it or unload it differently than other apps.  If it's not loaded, there's no reason to preload it, differently than we would for other apps.
Thanks a lot for clarifying.  I agree about not treating the dialer app specially, so long as the dialer app is not necessary for receiving calls...

>  - if that exists, the gaia system app launches the service as <app remote=false>, 
> *within* the b2g master process.  As root.

That's probably just a bug.  We should load it as <app remote={app is remote}>, otherwise an oop app can't talk to its background window, which kind of defeats the purpose in the general case.

If the telephony app's background window needs to be in the system process, then we can make an exception specifically for that case.
(In reply to Justin Lebar [:jlebar] from comment #21)
> Thanks a lot for clarifying.  I agree about not treating the dialer app
> specially, so long as the dialer app is not necessary for receiving calls...
> 
> >  - if that exists, the gaia system app launches the service as <app remote=false>, 
> > *within* the b2g master process.  As root.
> 
> That's probably just a bug.  We should load it as <app remote={app is
> remote}>, otherwise an oop app can't talk to its background window, which
> kind of defeats the purpose in the general case.
> 

It's NYI at the moment, but we can cross-process postMessage() pretty easily.  However I agree with out.

> If the telephony app's background window needs to be in the system process,
> then we can make an exception specifically for that case.

You mean the window.open popup for incoming call dialog?  How would that work?
> If the telephony app's background window needs to be in the system process,
> then we can make an exception specifically for that case.
>
> You mean the window.open popup for incoming call dialog?  How would that work?

I mean, we could make all background windows <app remote={app is remote}>, except we could, as an exception, keep the telephony background window <app remote=false> even if the dialer app is remote=true.
Yes, and indeed that's what happens now.  Are you sure that wouldn't cause problems ... somewhere?  I honestly don't know.

What we of course trade by doing that is locking away up to 10MB of memory in b2g process, for no particular good reason.
> Yes, and indeed that's what happens now.  Are you sure that wouldn't cause problems ... 
> somewhere?  I honestly don't know.

Neither do I.

> What we of course trade by doing that is locking away up to 10MB of memory in b2g 
> process, for no particular good reason.

But...don't we need this 10mb locked up somewhere in order to receive phone calls?
(In reply to Justin Lebar [:jlebar] from comment #25)
> > What we of course trade by doing that is locking away up to 10MB of memory in b2g 
> > process, for no particular good reason.
> 
> But...don't we need this 10mb locked up somewhere in order to receive phone
> calls?

No, that's the point here --- the telephony service is doing something that gecko is 100% capable of doing on its own with system messages.  That's one of the major motivations for creating system messages in the first place.
Ok, so we have 3 choices here:

* Leave things as-is, which means leaving 10MB of memory dedicated to the telephony background service at all times (20MB with the SMS service?). As well as a potential security risk if there is code in the background service which listens to postMessages and does dangerous tasks with it which the telephony app wouldn't otherwise be able to do?

* Switch the telephony API to using system messages for incoming phone calls and switch the telephony app to use this new API. This means enumerating this system message in the app manifest as well as adding code to prevent apps which doesn't have access to the telephony privilege from registering for the telephony system messages.

* Leave things as-is but make the telephony background page run OOP and restart it as needed if it crashes. This still uses the 10/20MB of memory, but fixes the problem of the telephony service crashing and leaving the phone unable to receive phone calls.

I agree that using 10/20% of all system memory because of a lack of this bug and bug 782489 sounds expensive.
That's accurate, except I don't think we have an idea yet of how to implement (3).

The two remaining bits of information we need are
 - accurate estimate of memory overhead of these services.  I've been saying "up to 10MB" very carefully, as this is based on the memory usage of b2g between two builds very far apart.  It should be very easy to disable these services and measure Pss before/after.

 - to what degree the current telephony service is integrated into the system UI, and how.  Etienne can you help us understand that?
Attached file Full data — 
(In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
>  - accurate estimate of memory overhead of these services.  I've been saying
> "up to 10MB" very carefully, as this is based on the memory usage of b2g
> between two builds very far apart.  It should be very easy to disable these
> services and measure Pss before/after.

Summary is that the services use 3-4.5MB of unreclaimable memory.  So this is up to 5% of memory available to apps.  This is a lot for code that doesn't add functionality, but I wouldn't tackle this work at this point for the memory savings.

There are two remaining problems we need to solve though

 (1) Either the background-service permission needs to be certified, which means third-party apps can't compete with a gaia email app

or

 (2) We need a plan for running services OOP and possibly special casing telephony/sms.  This also requires a plan for dealing with service crashes.
This would only affect the Telephony and SMS apps, and those APIs are certified-only in v1 anyway. So I don't think (1) is an issue.

I do believe that we use the background_service feature for other apps too, but fixing this bug won't affect those apps.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
>  - to what degree the current telephony service is integrated into the
> system UI, and how.  Etienne can you help us understand that?

Let's recap.
The dialer _background service_ is currently handling all (1) emergency calls, (2) outgoings calls and (3) incoming calls.

(1) The emergency call support creates a debatable relationship between the gaia system app and the dialer background service.
The lockscreen (part of the system app) dials an emergency call which is then handled by the dialer background service. The dialer background service will invoke the call screen.
In order to remove this we need to duplicate part of the dialer code (the call screen mainly) into the system app.

(2) The dialer app dials a call then handled by the dialer background service. If we get rid of the dependency in (1) this is easy peasy: we just need the dialer _app_ to invoke the call screen itself.

(3) The dialer background service gets notified of an incoming call, invokes the call screen.
This is where the system message comes in. Ideally we want the system message handler to be triggered without bringing the dialer app to the foreground, but this is an issue shared with the alarm app anyway.

Those changes on the gaia side are each a matter of days, not weeks.

Now, when I say "invokes the call screen" I mean "calls a special window.open that puts an iframe on top of everything else". This I don't think we want to change. And I will gladly discuss it elsewhere since it's not related to the removal of the background service.
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 previous comments, it sounds like we need to block here.  We can always un-+ if we come to a better conclusion.
blocking-basecamp: ? → +
Depends on: 787064
No longer depends on: 776832
After discussing with Fabrice, we are planning to add "broadcastMessage()" in nsISystemMessageInternal. In this way, RadioInterfaceLayer can use that to tell system message service which message type should be sent. The service will open registered Apps and send the message to them.
Comment on attachment 657207 [details] [diff] [review]
Patch: use system message to notify incoming calls

The patch bases on Bug 787175, which got 'nsISystemMessagesInternal' service already.
Attachment #657207 - Flags: review?(philipp)
Attachment #657207 - Flags: review?(philipp) → review+
Target Milestone: --- → mozilla17
Target Milestone: mozilla17 → mozilla18
https://hg.mozilla.org/mozilla-central/rev/b9b2cfebacf0
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 788125
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: