Follow-up Bug 849186 OOM_ADJ should not change for the topmost foreground task if the screen is switched off

RESOLVED FIXED in Firefox 39, Firefox OS v2.2

Status

Firefox OS
Performance
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Jose Manuel Cantera, Assigned: gsvelto)

Tracking

unspecified
2.2 S8 (20mar)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g18+ wontfix, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [caf priority: p1][CR 786369])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
This is follow-up of bug 849186 and should address the issue defined in the title and discussed in bug 849186.
(Reporter)

Updated

5 years ago
blocking-b2g: --- → tef?
(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.
(Reporter)

Comment 4

5 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.
(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.
(Reporter)

Comment 6

5 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.
(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.
(Reporter)

Comment 8

5 years ago
because at start-up we would need to retrieve some persistent value from indexedDB, bla, bla.
(Reporter)

Comment 9

5 years ago
Created attachment 727724 [details]
Pointer to GH PR

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.
Attachment #727724 - Flags: review?(crdlc)
Attachment #727724 - Flags: review?(alberto.pastor)
(Reporter)

Comment 10

5 years ago
oops wrong bug number. sorry
(Reporter)

Updated

5 years ago
Attachment #727724 - Flags: review?(crdlc)
Attachment #727724 - Flags: review?(alberto.pastor)
(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.
blocking-b2g: leo? → -
status-b2g18: --- → affected
tracking-b2g18: --- → +

Comment 13

5 years ago
What's the status of this bug?
Flags: needinfo?(jmcf)
This isn't a blocker, and that's probably why it's not being worked on.
(Reporter)

Updated

5 years ago
Flags: needinfo?(jmcf)
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.
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?
Flags: needinfo?(dcoloma)
(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
Flags: needinfo?(dcoloma)
(Assignee)

Updated

4 years ago
Attachment #727724 - Attachment is obsolete: true
(Assignee)

Comment 18

4 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.
(Assignee)

Updated

4 years ago
Component: Gaia::Contacts → Performance
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
(Assignee)

Updated

4 years ago
Blocks: 994518
(Assignee)

Comment 19

4 years ago
Created attachment 8537720 [details] [diff] [review]
[PATCH WIP] Prevent the foreground application from being killed when we turn off the phone
(Assignee)

Comment 20

4 years ago
Created attachment 8539267 [details] [diff] [review]
[PATCH WIP] Prevent the foreground application from being killed when we turn off the phone

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
Flags: needinfo?(mvines)
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+
Flags: needinfo?(mvines)
blocking-b2g: - → 2.2?

Updated

3 years ago
blocking-b2g: 2.2? → 2.2+

Updated

3 years ago
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED

Updated

3 years ago
Whiteboard: [CR 786369]

Updated

3 years ago
Whiteboard: [CR 786369] → [caf priority: p2][CR 786369]

Comment 22

3 years ago
Gabriele: What's still left to be done to land this patch in 2.2?
Flags: needinfo?(gsvelto)
(Assignee)

Comment 23

3 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.
Flags: needinfo?(gsvelto)

Comment 24

3 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?
Flags: needinfo?(gsvelto)
(Assignee)

Comment 25

3 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.
Flags: needinfo?(gsvelto)

Updated

3 years ago
Whiteboard: [caf priority: p2][CR 786369] → [caf priority: p1][CR 786369]

Comment 26

3 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.
Flags: needinfo?(mvines)
March 12th for the final fix uplifted to v2.2 sounds fine.
Flags: needinfo?(mvines)
(Assignee)

Comment 28

3 years ago
Created attachment 8574701 [details] [diff] [review]
[PATCH] Freeze priority changes when the screen is turned off

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 #8539267 - Attachment is obsolete: true
Attachment #8574701 - Flags: review?(khuey)
Comment hidden (obsolete)
(Assignee)

Comment 30

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

Updated

3 years ago
Blocks: 1142220

Updated

3 years ago
No longer blocks: 1142220
(Assignee)

Comment 31

3 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.
Keywords: checkin-needed
Looks like attachment 8574701 [details] [diff] [review] doesn't apply to v2.2 cleanly, could you please attach a v2.2 patch as well?
Flags: needinfo?(gsvelto)
(Assignee)

Comment 34

3 years ago
Created attachment 8576934 [details] [diff] [review]
[PATCH mozilla-b2g37_v2_2] Freeze priority changes when the screen is turned off

[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
Flags: needinfo?(gsvelto)
Attachment #8576934 - Flags: approval-mozilla-b2g37?
https://hg.mozilla.org/mozilla-central/rev/d34f33fae4e8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
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.
(Assignee)

Comment 37

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

Updated

3 years ago
Attachment #8576934 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
(Assignee)

Updated

3 years ago
Depends on: 892371
(Assignee)

Updated

3 years ago
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/63800b4e9c4a
status-b2g18: affected → wontfix
status-b2g-v2.2: affected → fixed
status-firefox37: --- → wontfix
status-firefox38: --- → wontfix
Target Milestone: --- → 2.2 S8 (20mar)
(Assignee)

Updated

3 years ago
Depends on: 1144132
You need to log in before you can comment on or make changes to this bug.