Jose Manuel Cantera Reporter
	Description • 12 years ago

This is follow-up of bug 849186 and should address the issue defined in the title and discussed in bug 849186.

	Michael Vines [:m1] [:evilmachines]
	Comment 1 • 12 years ago

(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)

	Daniel Coloma:dcoloma
	Comment 2 • 12 years ago

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.

	Dave Hylands [:dhylands]
	Comment 3 • 12 years ago

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.

	Jose Manuel Cantera Reporter
	Comment 4 • 12 years ago

(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.

	Dave Hylands [:dhylands]
	Comment 5 • 12 years ago

(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.

	Jose Manuel Cantera Reporter
	Comment 6 • 12 years ago

if we go for that change, we will increase Contacts App, start up time. I don't believe we should follow that path.

	Dave Hylands [:dhylands]
	Comment 7 • 12 years ago

(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.

	Jose Manuel Cantera Reporter
	Comment 8 • 12 years ago

because at start-up we would need to retrieve some persistent value from indexedDB, bla, bla.

	Jose Manuel Cantera Reporter
	Comment 9 • 12 years ago

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.

	Jose Manuel Cantera Reporter
	Comment 10 • 12 years ago

oops wrong bug number. sorry

	Justin Lebar (not reading bugmail)
	Comment 11 • 12 years ago

(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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 12 • 12 years ago

Along the lines of mvines' comment 1 we'll track instead of blocking on this, renom if a more widespread use case manifests.

	alberto.pastor
	Comment 13 • 12 years ago

What's the status of this bug?

	Justin Lebar (not reading bugmail)
	Comment 14 • 12 years ago

This isn't a blocker, and that's probably why it's not being worked on.

	David Flanagan [:djf]
	Comment 15 • 11 years ago

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.

	David Flanagan [:djf]
	Comment 16 • 11 years ago

This discusson on dev-gaia may be relevant:

On 2/5/14 8:14 AM, Fabrice Desré wrote:
> On 02/05/2014 01:00 AM, Tim Chien wrote:
>> CPU lock simply stop the phone from suspending JavaScript execution
>> cycles when the screen is asleep.
>
> That's not true. We use that to set the process priority, see
> https://mxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#902
>
> Note that there is also the "High Priority" wake lock that could be
> useful there. You can set it with
> navigator.requestWakeLock('high-priority');


Daniel: does requesting 'high-priority' fix the contacts importing issue you describe in comment #2?

	Daniel Coloma:dcoloma
	Comment 17 • 11 years ago

(In reply to David Flanagan [:djf] from comment #16)
> This discusson on dev-gaia may be relevant:
> 
> On 2/5/14 8:14 AM, Fabrice Desré wrote:
> > On 02/05/2014 01:00 AM, Tim Chien wrote:
> >> CPU lock simply stop the phone from suspending JavaScript execution
> >> cycles when the screen is asleep.
> >
> > That's not true. We use that to set the process priority, see
> > https://mxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#902
> >
> > Note that there is also the "High Priority" wake lock that could be
> > useful there. You can set it with
> > navigator.requestWakeLock('high-priority');
> 
> 
> Daniel: does requesting 'high-priority' fix the contacts importing issue you
> describe in comment #2?

Not sure if 'high-priority' was applied or not to contacts, anyhow, the current behaviour is quite similar:

 - Open Contacts
 - Select import from Facebook, a list of 200 contacts is shown
 - Select All and import
 - While the contacts are being imported, I open the browser go to a bookmark.
 - Go back to contacts, the application is started and only 10 contacts are available
 - However, when I go back to settings, the app shows me that only 10 of FB contacts have been imported and click on the FB toggle to continue importing the rest.

I hope this helps

	Gabriele Svelto [:gsvelto] Assignee
	Comment 18 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 20 • 10 years ago

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).

	Michael Vines [:m1] [:evilmachines]
	Comment 21 • 10 years ago

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.

	Mike Lee [:mlee]
	Comment 22 • 10 years ago

Gabriele: What's still left to be done to land this patch in 2.2?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 23 • 10 years ago

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.

	Mike Lee [:mlee]
	Comment 24 • 10 years ago

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?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 25 • 10 years ago

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.

	Mike Lee [:mlee]
	Comment 26 • 10 years ago

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.

	Michael Vines [:m1] [:evilmachines]
	Comment 27 • 10 years ago

March 12th for the final fix uplifted to v2.2 sounds fine.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 28 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 30 • 10 years ago

I had pushed the wrong queue, this is the correct try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8ecc39481a5

	Gabriele Svelto [:gsvelto] Assignee
	Comment 31 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d34f33fae4e8

	Michael Vines [:m1] [:evilmachines]
	Comment 33 • 10 years ago

Looks like attachment 8574701 [details] [diff] [review] doesn't apply to v2.2 cleanly, could you please attach a v2.2 patch as well?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 34 • 10 years ago

[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

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/d34f33fae4e8

	Michael Vines [:m1] [:evilmachines]
	Comment 36 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 37 • 10 years ago

(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.

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/63800b4e9c4a