Closed Bug 810453 Opened 13 years ago Closed 12 years ago

Background services are in "foreground" OOM class

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia
Type:
defect

Tracking

(blocking-basecamp:+)

Status:
RESOLVED FIXED
Milestone:
B2G C2 (20nov-10dec)
Project Flags:
blocking-basecamp +

People

(Reporter: cjones, Assigned: cjones)

References

Details

(Whiteboard: QARegressExclude)

Attachments

(2 files, 1 obsolete file)

Move background iframes to `background` OOM category by using `setVisible(false)`
802 bytes, patch
Details | Diff | Splinter Review
Load background services in the background, v2
628 bytes, patch
cjones
: review+
justin.lebar+bug
: review+
Details | Diff | Splinter Review
$ adb shell b2g-ps Messages app_0 3527 3481 60252 20876 ffffffff 400b9330 S /system/b2g/plugin-container $ adb shell cat /proc/3527/oom_adj 1 This means the services are not killable. We only have the cost control and SMS services currently. Neither has a useful purpose so they'll die for v1. However, if something comes up at the last minute, we *must* ensure this bug is fixed.
Assignee: nobody → salva
Please, do not kill CC service lightly. I know we were removing useless services but note Cost Control is trying to maintain up to date the balance amount so it needs to periodically send a SMS and wait for the answer. The Cost Control application should not awake every time an SMS is received but only when it is waiting for an answer SMS. It is probably I could minimize the time the service is up by using ALARM API instead of timers but this could be another issue. Furthermore this is related with the bug :cjones and I chatted about on IRC. Therefore, this is the bug blocking #813262. Does it sound good to you :cjones?
No longer blocks: 813262
blocking-basecamp: --- → ?
Flags: needinfo?(jones.chris.g)
Please, do not kill CC service lightly. I know we were removing useless services but note Cost Control is trying to maintain up to date the balance amount so it needs to periodically send a SMS and wait for the answer. The Cost Control application should not awake every time an SMS is received but only when it is waiting for an answer SMS. It is probably I could minimize the time the service is up by using ALARM API instead of timers but this could be another issue. Furthermore this is related with the bug :cjones and I chatted about on IRC. Therefore, this is the bug blocking #813262. Does it sound good to you :cjones?
As we discussed in the other bug, cost control should be sending its poll SMSes off alarms. That's the only way to ensure it's woken up when needed. The cost control process can continue running in the background after that until its memory is needed again for something higher priority. When UI needs to consume the returned values, it should read the SMS DB as needed. The UI itself should also be launched when needed.
Flags: needinfo?(jones.chris.g)
> Please, do not kill CC service lightly. Given the choice between leaving the CC service alive and leaving alive an app the user might want to interact with (such as the calculator or browser app), what do you think we should leave alive?
(In reply to Justin Lebar [:jlebar] from comment #4) > > Please, do not kill CC service lightly. > > Given the choice between leaving the CC service alive and leaving alive an > app the user might want to interact with (such as the calculator or browser > app), what do you think we should leave alive? Note it is important for the user to know how much money he expends because it is vital to use the device. Of course if he is interacting with some app we should kill before `background` applications rather than `foreground` ones. That is why this bug must be solved but this does not directly imply to remove a necessary service.
blocking-basecamp: ? → +
Priority: -- → P3
Target Milestone: --- → B2G C2 (20nov-10dec)
I leave this bug cause it is a B2G issue. It is related with the side effects of setVisible(). Maybe foreground/background test should only count among the iframes marked as not `background` or not been opened with the `background` flag.
Assignee: salva → nobody
Gecko allows privileged content to setVisible(true/false) mozbrowser frames. Are you suggesting we should remove that privilege? I don't quite understand comment 6. The goal of this bug is very simple: iframes loaded for background services need to be setVisible(false). That should be a 1-line change somewhere in the system app.
Nop, if you make setVisible(false) to the iframes, for some reason, they stop to work. I tried the solution and yes, it is only needed one line to solve the problem but to break background processes.
Can you describe in more detail what stops working?
How to reproduce: Before apply the patch! 1. Send a message to your test device. Ensure you are receiving a notification or vibration/sound feedback. 2. Apply the patch to Gaia 3. Send a message to your test device. Expected: You receive a notification with vibration or sound feedback. Actual: You are not receiving any notification nor vibration nor sound feedback.
Flags: needinfo?(jones.chris.g)
Assignee: nobody → salva
Works perfectly fine for me ...
Assignee: salva → jones.chris.g
Flags: needinfo?(jones.chris.g)
Ah, but I didn't notice the patch here and wrote my own, which is different.
Are there any race conditions here we should be mindful of? This patch seems to work just fine for me. $ adb reboot # wait a bit $ adb shell cat /proc/`adb shell b2g-ps | grep Messages | awk '{print $3}'`/oom_adj 6 # unlock lock screen $ adb shell cat /proc/`adb shell b2g-ps | grep Messages | awk '{print $3}'`/oom_adj 6
Attachment #685416 - Flags: review?(justin.lebar+bug)
More specifically, what I see (if I force the screen to remain on during all of startup), $ while true; do adb shell cat /proc/`adb shell b2g-ps | grep Messages | awk '{print $3}'`/oom_adj; sleep 0.25; done /proc//oom_adj: No such file or directory /proc//oom_adj: No such file or directory /proc//oom_adj: No such file or directory /proc//oom_adj: No such file or directory /proc//oom_adj: No such file or directory /proc//oom_adj: No such file or directory /proc//oom_adj: No such file or directory /proc//oom_adj: No such file or directory 1 1 1 6 6 6 6 ... which shows the Messages subprocess starting up in the foreground and then immediately going into the background. This is what I would expect to happen.
> Are there any race conditions here we should be mindful of? Do you mean should we care whether we setVisible(false) before or after adding the iframe to the DOM? Strictly speaking calling setVisible(false) before is better. But in our current implementation, the process doesn't get niced until one second after all its windows go into the background, so it doesn't matter.
> Strictly speaking calling setVisible(false) before is better. Oh, except I don't think the mozbrowser gets initialized until after the frame is inserted into the DOM. So you have to do it your way.
Attachment #685416 - Flags: review?(justin.lebar+bug) → review+
(In reply to Justin Lebar [:jlebar] from comment #16) > > Strictly speaking calling setVisible(false) before is better. > > Oh, except I don't think the mozbrowser gets initialized until after the > frame is inserted into the DOM. So you have to do it your way. Yeah, that's the difference between Salva's patch and mine. Sorta makes sense but sucks, however sucks in a post-v1 kind of way ;). I was more worried about the setVisible(false) being "lost" before the TabChild is set up, and this code working just because I was getting lucky. Do we store that value appropriately on the parent side and on the child ask for it reliably whenever it happens to get to a point to set its visibility state?
> I was more worried about the setVisible(false) being "lost" before the TabChild is set > up, and this code working just because I was getting lucky. No; we just set the visibility on the child's docshell. But the child has a docshell immediately after it's created, so I think we're fine.
So we're relying on synchronously creating a ContentParent/TabParent when the remote iframe is inserted into the DOM. OK; that's cheap and correct in general.
(Correct because the IPC messages are queued until the child is ready to process them.)
If I kill the SMS process and then send my phone an SMS, then SMS process isn't woken back up to process the message, AFAICT. ARGHHH! Did we ever land the patches to have the sms app handle the SMS system messages?
Flags: needinfo?(etienne)
Holding off on requesting approval-gaia-master for this because it's apparently regressed functionality that was previously working by accident ...
(In reply to Chris Jones [:cjones] [:warhammer] from comment #21) > If I kill the SMS process and then send my phone an SMS, then SMS process > isn't woken back up to process the message, AFAICT. ARGHHH! > > Did we ever land the patches to have the sms app handle the SMS system > messages? Fabrice and Vivien are working hard to land bug 778668 and bug 814074, as soon as we have them I have an updated gaia patch that should make everybody happy.
Flags: needinfo?(etienne)
The tray notifications are mostly orthogonal to the problem here, but OK.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #21) > If I kill the SMS process and then send my phone an SMS, then SMS process > isn't woken back up to process the message, AFAICT. ARGHHH! https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/background_service.js#L73 Supposedly background service frame will be relaunched when it is being killed :-/ Not sure what happen here. Also, I think you might have some opinion on what the correct behavior should be ...
It doesn't really make sense to restart content services if they die in the background. These services generally need to do these things - wake up from timers - respond to requests and events In each case, the services can be re-launched on demand. If they die in the background, the QoS is reduced slightly when the service is needed, but the device still functions just fine. Any scheme that attempts to relaunch non-critical services has to have a mechanism like exponential backoff built in, otherwise attempting to relaunch the processes can get into an infinite, expensive crash loop. That kind of tricky logic just isn't worth it to us in v1. I'd recommend that we morph the current background_services.js code into what bug 815440 proposes to do. That's basically what it's doing right now anyway. But I don't care all that much. The relaunch code needs to go, though.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #26) > It doesn't really make sense to restart content services if they die in the > background. These services generally need to do these things > - wake up from timers > - respond to requests and events > > In each case, the services can be re-launched on demand. If they die in the > background, the QoS is reduced slightly when the service is needed, but the > device still functions just fine. > > Any scheme that attempts to relaunch non-critical services has to have a > mechanism like exponential backoff built in, otherwise attempting to > relaunch the processes can get into an infinite, expensive crash loop. That > kind of tricky logic just isn't worth it to us in v1. > > I'd recommend that we morph the current background_services.js code into > what bug 815440 proposes to do. That's basically what it's doing right now > anyway. But I don't care all that much. > > The relaunch code needs to go, though. Background services are dying too. The last one is beeing worked on fwiw.
First, :cjones :jlebar, thanks for clarify all this. > Oh, except I don't think the mozbrowser gets initialized until after the > frame is inserted into the DOM. So you have to do it your way. Anyway, if setVisible(false) is being lost, why does the service ceases to work? It should be missed at all. If not, if the message is being delivered by the time the child is able to process the IPC message, then something weird is happening as well.
> (...) then something > weird is happening as well. Because it should be the same as making visible after appending.
With added workaround for bug 810431 suggested by jlebar, so carrying r=jlebar.
Attachment #686950 - Flags: review+
Attachment #685416 - Attachment is obsolete: true
Comment on attachment 686950 [details] [diff] [review] Load background services in the background, v2 Hey, if it works...
Attachment #686950 - Flags: review+
(In reply to Salvador de la Puente González [:salva] from comment #29) > > (...) then something > > weird is happening as well. > > Because it should be the same as making visible after appending. In theory yes, but the real machinery behind mozbrowser is only created once the iframe is inserted into the DOM. That bug is fixable but extremely painful at the platform level, and way too risky for v1 (could regress a lot of other things).
No problem for me. Very useful, thanks :)
FWIW the process doesn't get marked as background until 1s after all its windows are of type background, so the extra setTimeout(0) shouldn't be a significant delay.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #21) > If I kill the SMS process and then send my phone an SMS, then SMS process > isn't woken back up to process the message, AFAICT. ARGHHH! > > Did we ever land the patches to have the sms app handle the SMS system > messages? SMS is waking up like a champ now. Bombs away ...
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: