784040 - [Native Fennec] Expire background tabs on low memory

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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]]

Sidenote: this is perhaps a good place to put a hook for low memory database pruning, too.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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:[e-mail])]

Attachment: (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:[e-mail])]

Attachment: (2/4) Add some missing unregistrations to destroy()

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: (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:[e-mail])]

Attachment: (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?)]

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?)]

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:[e-mail])]

(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?)]

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?)]

(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?)]

(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 :)

Comment 14

[Kartikaya Gupta (email:[e-mail])]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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