Last Comment Bug 784040 - [Native Fennec] Expire background tabs on low memory
: [Native Fennec] Expire background tabs on low memory
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 16 Branch
: ARM Android
: -- normal (vote)
: Firefox 18
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
: 733557 (view as bug list)
Depends on:
Blocks: 816381 256meg
  Show dependency treegraph
 
Reported: 2012-08-20 08:20 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2012-11-28 21:07 PST (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(1/4) Port MemoryObserver to native fennec (10.36 KB, patch)
2012-09-26 12:21 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Review
(2/4) Add some missing unregistrations to destroy() (1.87 KB, patch)
2012-09-26 15:06 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Review
(3/4) Add some code to exercise the low-memory handlers on demand (2.84 KB, patch)
2012-09-26 15:07 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Review
(4/4) Zombify background tabs on low memory (4.59 KB, patch)
2012-09-26 15:08 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2012-08-20 08:20:19 PDT
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.
Comment 1 Richard Newman [:rnewman] 2012-08-20 08:52:56 PDT
Sidenote: this is perhaps a good place to put a hook for low memory database pruning, too.
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-18 12:02:19 PDT
*** Bug 733557 has been marked as a duplicate of this bug. ***
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-20 09:37:14 PDT
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.
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-26 12:21:22 PDT
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().
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-26 15:06:22 PDT
Created attachment 665144 [details] [diff] [review]
(2/4) Add some missing unregistrations to destroy()
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-26 15:07:27 PDT
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.
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-26 15:08:00 PDT
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
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2012-09-27 08:40:31 PDT
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?
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2012-09-27 08:51:55 PDT
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.
Comment 10 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-27 08:52:09 PDT
(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.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2012-09-27 08:53:24 PDT
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.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-09-27 08:54:53 PDT
(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.
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2012-09-27 08:56:39 PDT
(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 :)

Note You need to log in before you can comment on or make changes to this bug.