Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[Native Fennec] Expire background tabs on low memory

RESOLVED FIXED in Firefox 18

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mfinkle, Assigned: kats)

Tracking

16 Branch
Firefox 18
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

XUL Fennec used a MemoryObserver class to perform GC's and drop background tabs in low memory situations. We also used a preference to disable the normal GC so we don't do it twice.

http://mxr.mozilla.org/mozilla-central/search?string=MemoryObserver
http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#388

In Native Fennec we do not yet have the MemoryObserver code, but we still disable the normal GC (that is bad). We need to port the code or drop the preference.
Sidenote: this is perhaps a good place to put a hook for low memory database pruning, too.
OS: Linux → Android
Hardware: x86_64 → ARM
Blocks: 792131
Duplicate of this bug: 733557
Note to future self: mfinkle suggested moving the dump code from bug 783230 into the MemoryObserver class once it is resurrected. That seems like a good idea.
Created attachment 665081 [details] [diff] [review]
(1/4) Port MemoryObserver to native fennec

I moved over MemoryObserver but took out the tab.resurrect() calls for now since I wasn't sure if they worked in native fennec. I'll check to see if that still works as expected and if so add a second patch that hooks that up to handleLowMemory().
Assignee: nobody → bugmail.mozilla
Attachment #665081 - Flags: review?(mark.finkle)
Attachment #665081 - Attachment description: Port MemoryObserver to native fennec → (1/4) Port MemoryObserver to native fennec
Created attachment 665144 [details] [diff] [review]
(2/4) Add some missing unregistrations to destroy()
Attachment #665144 - Flags: review?(mark.finkle)
Created attachment 665145 [details] [diff] [review]
(3/4) Add some code to exercise the low-memory handlers on demand

This one maybe doesn't need to go in, but I found it pretty useful.
Attachment #665145 - Flags: review?(mark.finkle)
Created attachment 665146 [details] [diff] [review]
(4/4) Zombify background tabs on low memory

Seems to work pretty well but might need a little more testing
Attachment #665146 - Flags: review?(mark.finkle)
Comment on attachment 665081 [details] [diff] [review]
(1/4) Port MemoryObserver to native fennec

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>+var MemoryObserver = {

>+  observe: function mo_observe(aSubject, aTopic, aData) {
>+    if (aTopic == "memory-pressure") {
>+      if (aData != "heap-minimize") {
>+        this.handleLowMemory();
>+      }
>+      // The JS engine would normally GC on this notification, but since we
>+      // disabled that in favor of this method (bug 669346), we should gc here.
>+      // See bug 784040 for when this code was ported from XUL to native Fennec.
>+      this.gc();

Holy crap. I realize the the pref "javascript.options.gc_on_memory_pressure" is still false in mobile.js which means we have not been GC on memory pressure this whole time.

>+  dumpMemoryStats: function(aLabel) {

nit: We use "let" over "var". Could you switch?
Attachment #665081 - Flags: review?(mark.finkle) → review+
(Reporter)

Updated

5 years ago
Attachment #665144 - Flags: review?(mark.finkle) → review+
(Reporter)

Updated

5 years ago
Attachment #665145 - Flags: review?(mark.finkle) → review+
Comment on attachment 665146 [details] [diff] [review]
(4/4) Zombify background tabs on low memory

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    // Make sure the previously selected panel remains selected. The selected panel of a deck is
>+    // not stable when panels are added.
>+    let selectedPanel = BrowserApp.deck.selectedPanel;
>+    BrowserApp.deck.insertBefore(this.browser, aParams.browserSibling || null);

nit: Let's just call "browserSibling" -> "sibling"

>   handleLowMemory: function() {

>+    var tabs = BrowserApp.tabs;
>+    var selected = BrowserApp.selectedTab;
>+    for (var i = 0; i < tabs.length; i++) {

"var" -> "let"


Nice patch. Looks like you have everything covered for resurrection.
Attachment #665146 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #8)
> Holy crap. I realize the the pref "javascript.options.gc_on_memory_pressure"
> is still false in mobile.js which means we have not been GC on memory
> pressure this whole time.

Heh, yeah. I thought that's what you were referring to in comment 0 when you said "that is bad".

> >+  dumpMemoryStats: function(aLabel) {
> 
> nit: We use "let" over "var". Could you switch?

I can, although I'm curious as to why we do that. "let" is only supported by our JS engine and as far as i can tell identical to "var". Why not use the standard "var"?

Also I tested these patches some more and they seem to be holding up. Specifically I did this:

1) load http://people.mozilla.org/~kgupta/tmp/oom.html (it just keeps allocating memory)
2) stop oom after it has allocated about 10000 divs
3) open a new tab with about:blank
4) dump memory stats using adb shell am -a org.mozilla.gecko.MEMORY_DUMP
5) force zombification of the oom tab using adb shell -a org.mozilla.gecko.FORCE_MEMORY_PRESSURE
6) dump memory stats again
7) compare memory stats from (4) and (6) to ensure that we freed a bunch of memory. this was in fact the case, so i conclude this code works as expected.
CC'ing Brian because he was interested in working on some "stub tab" placeholders in the Java side and this might have some overlap with that work.
(In reply to Kartikaya Gupta (:kats) (away sep28-oct08) from comment #10)

> > nit: We use "let" over "var". Could you switch?
> 
> I can, although I'm curious as to why we do that. "let" is only supported by
> our JS engine and as far as i can tell identical to "var". Why not use the
> standard "var"?

"var" hoists the scope of the variable to function-level. "let" allows the scope to stay at the if/for block level. "let" allows the scope to be more like C or Java.
(In reply to Kartikaya Gupta (:kats) (away sep28-oct08) from comment #10)
> (In reply to Mark Finkle (:mfinkle) from comment #8)
> > Holy crap. I realize the the pref "javascript.options.gc_on_memory_pressure"
> > is still false in mobile.js which means we have not been GC on memory
> > pressure this whole time.
> 
> Heh, yeah. I thought that's what you were referring to in comment 0 when you
> said "that is bad".

I guess I did realize it, but just forgot about that. I'm glad this is getting fixed :)
Ah, I didn't know that about let (or maybe I knew but forgot). Updated var -> let and browserSibling -> sibling

https://hg.mozilla.org/integration/mozilla-inbound/rev/41416282254b
https://hg.mozilla.org/integration/mozilla-inbound/rev/db369c141f93
https://hg.mozilla.org/integration/mozilla-inbound/rev/8921d8bc93a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6fd6841e7f3
https://hg.mozilla.org/mozilla-central/rev/41416282254b
https://hg.mozilla.org/mozilla-central/rev/db369c141f93
https://hg.mozilla.org/mozilla-central/rev/8921d8bc93a2
https://hg.mozilla.org/mozilla-central/rev/e6fd6841e7f3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18

Updated

5 years ago
Keywords: sec-review-needed

Updated

5 years ago
Summary: Port MemoryObserver to Native Fennec → [Native Fennec] Expire background tabs on low memory

Updated

5 years ago
Blocks: 816381

Updated

5 years ago
Keywords: sec-review-needed
You need to log in before you can comment on or make changes to this bug.