835563 - When Gaia does setVisible(false), send the relevant process into the background immediately

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

cjones and I think this is necessary to prevent edge cases where we receive a call and then kill the phone app due to low memory.  In that sense, this blocks bug 834059, and I'm going to assume that patches here do not need additional approval to land.

I have a patch which I'm testing now.

Comment 1

[Justin Lebar (not reading bugmail)]

I think the membuster testcase [1] is killing the homescreen because the browser apparently does not setVisibile(false) tabs until it finishes taking a screenshot.  At least, that's how I understand the code.

[1] http://people.mozilla.com/~cjones/membuster.html

Comment 2

[Justin Lebar (not reading bugmail)]

Separately, Gaia sends /every window into the background when it's first created/.  :(

In window_manager.js::appendFrame()

>    // A frame should start with visible false
>    if ('setVisible' in iframe)
>      iframe.setVisible(false);

So the suggested patch here kills the phone when it starts up, with current Gaia.

Comment 3

[Justin Lebar (not reading bugmail)]

> So the suggested patch here kills the phone when it starts up, with current Gaia.

I should say, when we're under memory pressure.

Comment 4

[Justin Lebar (not reading bugmail)]

I filed bug 835795 on the Gaia issue.  It of course blocks any Gecko patch here.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Switching the ProcessPriorityManager to using the docshell's visibility was necessary because the document's visibility gets updated asynchronously.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 707647 [details] [diff] [review]
Patch, v1

I'm having a lot of difficulty fixing the requisite issues in Gaia, so I believe less in this approach now.

We've designed our APIs so that Gaia does not know, at the time it launches the communications app, whether a phone call is pending.

So it's therefore difficult for Gaia to take the right action in this case.  It would be wrong for Gaia to launch all apps receiving a system message as fg, because then we're right back where we started (the phone app has to compete for memory with these apps).

There is a simple solution here, which is to give the phone app higher priority when it's in the fg.

Comment 7

[Justin Lebar (not reading bugmail)]

> There is a simple solution here, which is to give the phone app higher priority when it's 
> in the fg.

Unfortunately it's not so simple.  :(

Gaia sends the communications app into the background (bug 835795).  We currently don't act on that for 1s, but that timeout isn't long enough.  When I receive a call while we're busting processes, the 1s expires before the comm app is brought into the fg by Gaia, and we kill it.

We could increase this timeout, but then that decreases the efficacy of the window manager.  So I think we may be back to where we started (i.e., fixing bug 835795).

The good news is that I have the FOREGROUND_COMMUNICATIONS patch all written.  That's the easy part.  :)

Comment 8

[Andrew Overholt [:overholt]]

tef? since jlebar thinks it blocks bug 836199 which is tef+.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking as per comment 8

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

What are the next steps here?  The deadline for landing blocker bugs is rapidly approaching (2/15) and getting this landed sooner rather than later to shake out any possible regressions would be ideal.

Comment 11

[Justin Lebar (not reading bugmail)]

Ultimately we need to fix bug 836199, bug 836638, and bug 837927.  These all seem to be related.

It doesn't really matter how we fix these three bugs (I'll call these three the "true blockers").  I have a bunch of bugs (this one included) that indicate my current thinking about the best way to fix the true blockers.  These subordinate bugs are blocking+ only because of the curious way we've set up our approval system; if I didn't have these bugs marked as blocking, I'd have to wait days to land fixes, and I wouldn't have been able to get anyone to assist me on the Gaia side.

I could have just put all of this work into the true blockers themselves and avoided creating false blockers, but that's confusing from a development point of view.

I can post a patch for review here that does what the bug summary asks, but if it doesn't improve the situation with the true blockers, we're just churning code for no reason.  I'm trying to evaluate whether the changes I've made here are an improvement, but it's hard to evaluate any one patch's impact on the true blockers.

Realistically, I don't think we are going to fix the true blockers by the end of next week.  This is a complex issue, and in addition to my own slow progress, we've been making slow progress on the Gaia side, which greatly slows down most of the platform work, because I can't evaluate my platform changes without the prerequisite Gaia changes.

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v2

This seems to fix bug 837927 for me, although I've never had good luck reproducing it.

Comment 13

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1651dcb62397

Comment 14

[Ed Morley [:emorley]]

Push & followups backed out for mochitest-2 and mochitest-other failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a74f08ec0e46

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=f48618e815d1

Comment 15

[Justin Lebar (not reading bugmail)]

I think the test failures may have been the result of not clobbering (strangely enough), so I've re-pushed with a clobber.

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/15587a04ec3d
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c24117eec445

Comment 18

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 19

[Justin Lebar (not reading bugmail)]

Are you able to use adb?

Comment 20

[Cody Roesch [:croesch]]

sure am.

Comment 21

[Justin Lebar (not reading bugmail)]

Like the vast majority of bugs, I don't think this is worth verifying; the code change is obvious here.  But since you asked so nicely:

* $ watch -n.1 adb shell b2g-procrank --oom
* Open an app.  Notice its oom_adj value.
* Press the home button

Its oom_adj value should immediately change.  Before this patch, the oom_adj value would change after 1s.

Comment 22

[amiller]

No need to create a TC in Moztrap for this issue.