9.41 KB, patch
|Details | Diff | Splinter Review|
9.53 KB, patch
|Details | Diff | Splinter Review|
This is follow-up of bug 849186 and should address the issue defined in the title and discussed in bug 849186.
(moving to leo? because while the potential for this bug to produce really terrible UX is high, I'm not sure we've heard many reports of the foreground app mysteriously closing when the display turns off beyond bug 849186. suggest we consider an uplift to v1.0.1 if a patch becomes available but not block until we see another manifestation of this bug)
blocking-b2g: tef? → leo?
Not sure if this is an scenario we should be targeting to fix with this bug: I start importing contacts from Facebook (have around 500 friends), during the process I am tired to see the progress screen and go to browser, and then to check my e-mail. When I go back to contacts, I see the contact list (not the FB import progress) and I see only some of the contacts imported without any further information.
I would actually expect the results in comment 2 if memory is low. I guess the contacts app (or any app for that matter) needs to keep some state information so that it pick up where it left off if it got OOM'd in the background.
(In reply to Dave Hylands [:dhylands] from comment #3) > I would actually expect the results in comment 2 if memory is low. I guess > the contacts app (or any app for that matter) needs to keep some state > information so that it pick up where it left off if it got OOM'd in the > background. yes, the user can continue later importing more contacts by clicking on 'Update Contacts' later.
(In reply to Jose M. Cantera from comment #4) > (In reply to Dave Hylands [:dhylands] from comment #3) > > I would actually expect the results in comment 2 if memory is low. I guess > > the contacts app (or any app for that matter) needs to keep some state > > information so that it pick up where it left off if it got OOM'd in the > > background. > > yes, the user can continue later importing more contacts by clicking on > 'Update Contacts' later. I meant that the contacts app should probably remember that it was importing contacts and didn't finish, so that it can resume or offer to resume the next time it launches.
if we go for that change, we will increase Contacts App, start up time. I don't believe we should follow that path.
(In reply to Jose M. Cantera from comment #6) > if we go for that change, we will increase Contacts App, start up time. I > don't believe we should follow that path. Huh - I don't follow how you came to that conclusion. I'm not suggesting that it should do a sync each time it starts. I'm suggesting that it should remember that it didn't finish doing the previous one. I don't understand how that can have any significant impact on the startup time.
because at start-up we would need to retrieve some persistent value from indexedDB, bla, bla.
Requesting also Crdlc's review as the PR modifies image loader component. Please note that this PR in practice disable search of FB contacts by organization . In later PRs we can address that issue or keep it as it is but as this is a blocker I would prefer not to be distracted by that minor issue and address the rest later by think it more carefully.
oops wrong bug number. sorry
(In reply to Jose M. Cantera from comment #8) > because at start-up we would need to retrieve some persistent value from > indexedDB, bla, bla. I thought IndexedDB was asynchronous. dhylands' suggestion is exactly what we should do, IMO. It's a premature optimization to say that we can't do it because it would be too slow.
Along the lines of mvines' comment 1 we'll track instead of blocking on this, renom if a more widespread use case manifests.
What's the status of this bug?
This isn't a blocker, and that's probably why it's not being worked on.
Okay, so here's another use case that is affected by this bug. If the user has an sdcard with a lot of photos on it and inserts it into their phone, then launches the gallery app, the gallery app will scan the sdcard and create thumbnails for all of the photos it finds. This takes a lot of memory and, if there are a lot of photos, it takes a long time, too. If the screen blanks while the scan is happening, the gallery app dies with an OOM. I noticed this while investigating bug 963917, but that bug is about something else. I can work around the issue in gallery by having it hold a wake lock on the screen while scanning photos, but that seems wrong. We could fix the bug as described in the summary. Or we could try some sort of compromise. Maybe we should treat apps that have a wake lock on the CPU specially so that if the screen blanks, they remain in the foreground and are not targeted when memory is tight. That way, apps that really need to keep running for some reason (like scanning) have a way to keep themselves alive. But ordinary apps don't get special status when the user let's their screen go idle.
Attachment #727724 - Attachment is obsolete: true
I've been trying to fix this bug in the past as part of bug 989713 and I've got a WIP patch there that solves the root cause of this. I'll move the patch here so that I can pick it up later as soon as I have some time for it.
Component: Gaia::Contacts → Performance
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Rebased patch that applies on top of master. Michael I haven't had time to re-test this but it used to work; you might want to give it a spin to see if it solves the issue of the foreground app being killed when it becomes hidden (but doesn't go into the background).
Attachment #8537720 - Attachment is obsolete: true
Comment on attachment 8539267 [details] [diff] [review] [PATCH WIP] Prevent the foreground application from being killed when we turn off the phone Review of attachment 8539267 [details] [diff] [review]: ----------------------------------------------------------------- This definitely improves the basic card view use case. Consider this UI flow though: Contacts -> New Contact -> Select Picture -> Camera While the Camera activity is in the foreground, both Camera and Contacts apps have OOM_ADJ=2. With this patch, when the card view is entered the Camera remains at 2 but Contacts (Communications) is still bumped down to OOM_ADJ=10 and is likely to get killed.
Attachment #8539267 - Flags: feedback+
Gabriele: What's still left to be done to land this patch in 2.2?
There's two things to address: first of all the behavior is not completely solid as described in comment 21, second all the associated tests need to be adjusted. However those tests are currently quite complicated and part of them is going to be removed in bug 1119277. So I'd like to finish that first so that I don't do any duplicate work there.
Thanks Gabriel. Do you have a sense of how long each of the two parts you called out will take and a general timeframe for when you'd be able to deliver these changes?
Bug 1119277 is about to land and I've got a half-done patch for bug 892371 which I intend to finish by the end of this week, early next week at most. Then comes this bug so I'd say two weeks max to land everything. Does that fit with our deadlines? The reason why I want to fix bug 892371 first is that I'm adding LRU tracking of foreground processes which will help with the work needed here.
Whiteboard: [caf priority: p2][CR 786369] → [caf priority: p1][CR 786369]
Thanks Gabriele. mvines: Can you respond to Gabriele's comment 25 question re: his delivery date and CAF's schedule? 2 weeks from the date he commented will be Thursday, March 12th which is between FL and FC.
March 12th for the final fix uplifted to v2.2 sounds fine.
This patch addresses the problem in a rather simple way: by freezing all priority changes while the screen is off. This ensures that foreground processes are not demoted because their visibility changed. I feel silly of not having thought about this solution earlier :-/ This shouldn't cause regressions when launching an application with the screen off (for example in response to a message or call) because we invariably grab a wakelock to turn the screen on and in that case any pending priority changes are immediately applied. BTW I've developed this on top of the patch for bug 892371 because they both touch the same code and we want both in 2.2 but there's no logical dependency between the two so if we want to land this one first I can rebase it on top of a clean mozilla-central tree.
Attachment #8574701 - Flags: review?(khuey) → review+
I had pushed the wrong queue, this is the correct try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8ecc39481a5
Here's a more complete try run, disregarding the MacOS 10.6 harness bustage everything is green though I had to re-trigger a couple of tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bb63e8457b5 Note to the sheriffs, this needs to land after bug 892371 as the patch here applies on top of that one.
Looks like attachment 8574701 [details] [diff] [review] doesn't apply to v2.2 cleanly, could you please attach a v2.2 patch as well?
[Approval Request Comment] Bug caused by (feature/regressing bug #): Long standing bug User impact if declined: On low memory devices the current foreground app can be killed when turning the screen off even though there should be enough memory to keep it around Testing completed: Tested on a device, a try run is here https://treeherder.mozilla.org/#/jobs?repo=try&revision=7257705b1f95 Risk to taking this patch (and alternatives if risky): This prevents changes in the priorities of processes when the screen is off, if this is buggy or does not work as advertised after turning the screen back on some priorities might be wrong. The patch is very simple though so the risk should be very low. String or UUID changes made by this patch: None
Attachment #8576934 - Flags: approval-mozilla-b2g37?
Based on comment 20 and comment 21, I was expecting that the final patch here would also preserve the OOM_ADJ score of the (previously) foreground application when the card view is entered. It does however fix the issue described in the bug title, so probably my bad for assuming. I'll create a new bug for the card view issue.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #36) > Based on comment 20 and comment 21, I was expecting that the final patch > here would also preserve the OOM_ADJ score of the (previously) foreground > application when the card view is entered. It does however fix the issue > described in the bug title, so probably my bad for assuming. I'll create a > new bug for the card view issue. Ah yes, good point. I went for the simpler solution because my original approach was trickier to get right than I had anticipated and I didn't want to delay the fix. Fixing the card view situation without doing some ugly hacks requires some cooperation from the system app which is better addressed in another bug so thanks for filing it.
Attachment #8576934 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.