Closed Bug 821440 Opened 7 years ago Closed 7 years ago

Make app-launcher perceivable (kill all other apps before homescreen)

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: cjones, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: UX-P1)

Attachments

(2 files, 1 obsolete file)

jlebar and I had a difference of opinion over this heuristic, but my experience running the memory usage tests has convinced me that this is a good idea.

What I've found is that my mental model of launching apps is kind of like "back/forward" navigation in a web browser.  Launching the app is "forward", returning to the homescreen is "back".

In this model, my mind expects the homescreen to stay running behind the foreground app and going "back" takes me to the state I was previously in.  When the homescreen is killed early, this model is violated and it's rather jarring.

This is fudging the definition of a "perceivable" app a bit, but I think it's justified.

I'd prefer to do this in the platform with heuristics like apps with app-launcher permissions get protected as perceivable when going to the background.
Doing this bug as written would violate the memory acceptance criteria as written.

> MANDATORY: No perceivable application is killed due to memory pressure while a 
> background application is alive. 

Since we agree that the homescreen isn't actually perceivable, making it perceivable as a hack makes it impossible to meet this criterion.

Instead, I'd suggest killing the homescreen after any true bg apps, but before any true perceivable apps.

This further increases the apparent proliferation of memory classes, but since apps don't explicitly request to be put into a specific class, I don't think this is a big deal.
I don't disagree a priori.  What do you propose for this new class?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> I don't disagree a priori.  What do you propose for this new class?

You mean, what name?  "Homescreen"?  "HighPriorityNonPerceivable"?  :shrug:
Component: DOM → General
Product: Core → Boot2Gecko
Version: 19 Branch → unspecified
basecamp- The exprience is not so bad that we'll block on it for v1.
blocking-basecamp: ? → -
I tend to disagree.  This bug causes the homescreen to perma-crash when any new app is loaded.  So the user can get into situations where every app->homescreen transition relaunches the homescreen from scratch.  Follow the steps at [1] to see what I mean.

I would like UX to chime in.

https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW0:_Every_app_is_successfully_launched_into_the_foreground
Flags: needinfo?(jcarpenter)
I'll just fix this; we don't need to waste our time arguing about the blocking status.
Assignee: nobody → justin.lebar+bug
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> I tend to disagree.  This bug causes the homescreen to perma-crash when any
> new app is loaded.  So the user can get into situations where every
> app->homescreen transition relaunches the homescreen from scratch.  Follow
> the steps at [1] to see what I mean.
> 
> I would like UX to chime in.
> 
> https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW0:
> _Every_app_is_successfully_launched_into_the_foreground

You nailed it in your first comment. As the hub of the system, the home screen will be the most frequently-accessed component in FFOS, and as such we should focus our efforts on making it load instantly. I cannot speak to the implementation proposals discussed above, but from a product quality standpoint it would be tremendous if we can make the home app load instantly, reliably. For that reason, I strongly support blocking+ on this, and am also marking this UX-P1.

Thanks guys :)
Flags: needinfo?(jcarpenter)
Whiteboard: UX-P? → UX-P1
blocking-basecamp: - → ?
Triage team still feels that this isn't a blocker.
blocking-basecamp: ? → -
Attached patch Patch, v1 (obsolete) — Splinter Review
This seems to work.

cjones, the main thing I'm unsure of here is whether we're SendSetAppType()'ing at the right time.

I'm also unsure that I've chosen good settings for the oom killer, but I figure we can adjust these later.
Attachment #693160 - Flags: review?(jones.chris.g)
Comment on attachment 693160 [details] [diff] [review]
Patch, v1

We don't have SR in this component (I'll file a bug on that).  Jonas, can you please SR the following API change?

I'm adding an attribute "mozapptype" to iframes.  It's ignored unless the iframe corresponds to an app.  If the iframe corresponds to an app and mozapptype is "homescreen", we will be less likely to kill that iframe's process when we encounter low memory.
Attachment #693160 - Flags: review?(jonas)
Attached file Gaia pull request
The Gaia change can land before or after the Gecko change.
Attachment #693164 - Flags: review?(timdream+bugs)
Comment on attachment 693160 [details] [diff] [review]
Patch, v1

>diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js

>+pref("hal.processPriorityManager.gonk.backgroundHomescreenOomScoreAdjust", 200);

Are you sure this value puts us on an integer oom_adj value?

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp

> TabParent::Show(const nsIntSize& size)
> {

>+      nsAutoString appType;
>+      GetAppType(appType);
>+      if (!appType.IsEmpty()) {
>+        unused << SendSetAppType(appType);

We should send this along with the PBrowser constructor.  We can pass
it down from nsFrameConstructor and keep the IContent* hackery
centralized.

> void
>+TabParent::GetAppType(nsAString& aAppType)

We wouldn't need this if we send the info from FrameConstructor.

This patch makes me vom in my mouth a bit, but otherwise looks OK ;).
Attachment #693160 - Flags: review?(jones.chris.g)
> We should send this along with the PBrowser constructor.

I tend to dislike adding things to that because it's unnecessarily complicated in the window.open case.  We end up writing all the code twice (because it has to work parent --> child and child --> parent).

Is there some other init method I can use?
Also, I guess you mean s/nsFrameConstructor/nsFrameLoader/?
Yes, I meant FrameLoader.

There's not any other init method.  If you can't send it though the constructor at least only send it once.
Comment on attachment 693160 [details] [diff] [review]
Patch, v1

Review of attachment 693160 [details] [diff] [review]:
-----------------------------------------------------------------

sr=me
Attachment #693160 - Flags: review?(jonas) → review+
> Are you sure this value puts us on an integer oom_adj value?

Unless the kernel is doing something weird, it turns out that none of our oom_score_adj's correspond to integer oom_adj's.

  $ adb shell
  # echo 3 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj
  176
  # echo 4 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj
  235

So 200 is not an integer oom_adj.  But neither is 400, which we currently use for bg processes:

  # echo 6 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj     
  352
  # echo 7 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj     
  411

and neither is 67, which we use for fg processes:

  # echo 1 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj     
  58
  # echo 2 > /proc/1766/oom_adj; cat /proc/1766/oom_score_adj     
  117

I'll use 411 in this patch and file a separate bug to fix the rest of these.  I think we need to fix GonkHal.cpp::OomAdjOfOomScoreAdj, too.

> Yes, I meant FrameLoader.

Doing it in nsFrameLoader is /way/ simpler.  Thanks for the suggestion.
> I'll use 411 in this patch and file a separate bug to fix the rest of these.

erm, I mean 176.
Attached patch Patch, v2Splinter Review
Attachment #693478 - Flags: review?(jones.chris.g)
Attachment #693160 - Attachment is obsolete: true
> I'll use [176] in this patch and file a separate bug to fix the rest of these. 

Filed bug 822709.
Blocks: 822709
> I'll use [176] in this patch and file a separate bug to fix the rest of these. 

Turns out the kernel is doing something weird, and our code (which it turns out is based on the kernel code) is right.

It turns out that the oom_adj --> oom_score_adj and oom_score_adj --> oom_adj functions are not inverses of each other.  The first one multiplies by 15 and divides by 1000, while the second multiplies by 1000 and divides by 17.

The formulas are

  oom_score_adj = (oom_adj * 1000) / 17
  oom_adj = (oom_score_adj * 15) / 1000

so an oom_score_adj of 200 corresponds perfectly to an oom_adj of 3.  Just not the other way around.  I'll fix the patch locally.
Attachment #693478 - Flags: review?(jones.chris.g) → review+
BTW, you can test this patch by running https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW0:_Every_app_is_successfully_launched_into_the_foreground .  If by around step 18 you don't want to throw the phone out the window, your patches have helped ;).
Comment on attachment 693478 [details] [diff] [review]
Patch, v2

[Approval Request Comment]
Affects only B2G, no string changes.
Attachment #693478 - Flags: approval-mozilla-b2g18?
Attachment #693478 - Flags: approval-mozilla-aurora?
> If by around step 18 you don't want to throw the phone out the window, your patches have helped ;).

Homescreen didn't crash once.
Comment on attachment 693164 [details] [review]
Gaia pull request

Tim, even though this isn't a blocker it's a UX-P1 (and a simple fix).  If you're too busy to look at this because it's not a blocker, please let us know and we can find another reviewer.  Thanks!
Given that we're actively working on this bug and it is highly desired I think we should remove any roadblocks to finishing this work. I'm flipping the blocking flag to basecamp+. We can renom, if necessary, if this bug takes significantly more time to land or has a bad bounce.
blocking-basecamp: - → +
Attachment #693478 - Flags: approval-mozilla-b2g18?
Attachment #693478 - Flags: approval-mozilla-aurora?
Target Milestone: --- → B2G C3 (12dec-1jan)
https://hg.mozilla.org/mozilla-central/rev/7da46060c2d9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #693164 - Flags: review?(timdream+bugs) → review+
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.