451670 - Discard tab data on when low on memory

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Stuart Parmenter]

We need to be able to throw away the <browser> of a tab when we run low on memory and restore its data when we go back to it.

We probably want to hook up a global listener for the nsIObserver notification "memory-pressure" with subject "low-memory" and then discard the least recently used tabs checking !nsIMemory.isLowMemory() after each thing we free.

Comment 1

[Stuart Parmenter]

finkle points out to me that some of this is already in the tree...

As we don't send multiple low-memory notifications (perhaps we should..), might want to just add the code to loop over and keep discarding until !isLowMemory.

Comment 2

[Neil Deakin]

(In reply to comment #1)
> As we don't send multiple low-memory notifications (perhaps we should..), might
> want to just add the code to loop over and keep discarding until !isLowMemory.

Should we just add this or is there more we want to do here?

Comment 3

[Stuart Parmenter]

We should get this hooked back up now that the tab stuff has changed a substantial amount.

Comment 4

[Stuart Parmenter]

Attachment: v0.5

I realized that using nsIMemory.isLowMemory isn't going to do much unless we force a GC to happen after we release stuff.

anyways, this should hook back up the previous functionality.  need to do some testing on device where we actually run low on memory.

Comment 5

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

Comment on attachment 352362 [details] [diff] [review]
v0.5

earliestBrowser == earliestTab ?

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Patch for cleaning up as many tabs as possible on low memory

I don't have access to a device yet, but you can free up memory from the earliest used tabs by pressing CTRL-SHIFT-F.  When you go back to that tab, it should reload the page, fill in forms, and take you back to your zoom level and scroll coords.

The code that runs when memory is low still needs to be tested.  A debug message occurs when this happens.

This is my first patch; please review a little more extensively than usual.

Comment 7

[Benjamin Stover (:stechz)]

Attachment: Added forced garbage collection and a utility dumpf

Comment 8

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

Comment on attachment 395699 [details] [diff] [review]
Added forced garbage collection and a utility dumpf

> ScrollwheelModule.prototype = {
>   handleEvent: function handleEvent(evInfo) {
>     if (evInfo.event.type == "DOMMouseScroll") {
>+      Browser.selectedTab.lockZoom();

Why do we need to do this here? I don't like "lockZoom". Isn't there a way we can handle the "zoomToPage" issue from the state itself?

>+    os.addObserver(function(subject, topic, data) function() {
>+      dump("i'm in ur browser, clearing your memories\n");
>+      let memory = Cc["@mozilla.org/xpcom/memory-service;1"].getService(Ci.nsIMemory);
>+      while (memory.isLowMemory && self._sacrificeSomeTab()) {
>+        nsIDOMWindowUtils.garbageCollect();
>+      }
>+    }, "memory-pressure", false);

Let's make a helper object, like SoftKeyboardObserver to handle the "observe" and remember to removeObserver too.

>     if (!tab || this._selectedTab == tab)
>       return;
> 
>+    tab._lastSelectedTime = Date.now();

rename to tab.touched (no "_" since it's public) but I'd really like this to be automatic. Maybe we could set it whenever Tab.browser is accessed.

>+    let [restoreX, restoreY] = Browser._getContentScrollCoords(tab);

Can we move Browser._getContentScrollCoords to Tab.getPosition ?

>+  _sacrificeSomeTab: function() {

No "_" for public methods

>+    let self = this;
>+
>+    let tabToClear = self._tabs.reduce(function(prevTab, currentTab) {

Yank the blank line

>+      }
>+      else {
>+        return (prevTab && prevTab._lastSelectedTime <= currentTab._lastSelectedTime)
>+          ? prevTab : currentTab;

80 is not a hard limit in mobile JS. 

>+      }
>+    }, null);
>+
>+    if (tabToClear) {
>+      tabToClear._saveState();
>+      tabToClear._destroyBrowser();

Perhaps Tab.saveState (no "_") could destroy the browser too?

>+    }
>+    else {

Cuddle your "else" blocks like this "} else {"  (you get used to it)

>+  _getContentScrollCoords: function(tab) {
>+    // XXX these should probably be computed less hackily so they don't
>+    //   potentially break if we change something in browser.xul
>+    // XXX bug: should not add offY for when the contentscrollbox version of the urlbar 
>+    // is active
>+    let offY = Math.round(document.getElementById("toolbar-container").getBoundingClientRect().height);
>+    let restoreX = Math.max(0, tab.browserViewportState.visibleX);
>+    let restoreY = Math.max(0, tab.browserViewportState.visibleY) + offY;
>+    return [restoreX, restoreY];
>+  },

Please move to Tab.getPosition and restoreX/Y -> scrollX/Y

>       doubleClick: function doubleClick(cX1, cY1, cX2, cY2) {
>+        Browser.selectedTab.lockZoom();

Still want to kill the explicit call. We could use the Tab_state to figure this out right?

Also, favor " for wrapping strings, not ' (just a consistency thing)

Comment 9

[Benjamin Stover (:stechz)]

> Why do we need to do this here? I don't like "lockZoom". Isn't there a way we
> can handle the "zoomToPage" issue from the state itself?

I can't really think of a way that doesn't require some larger refactoring.  Do you have any ideas?

Comment 10

[Benjamin Stover (:stechz)]

Attachment: Code review changes

Patch is now less confused about scrolling offsets and does the right thing when switching tabs and restoring state.  Tab switching now saves the last scroll offset so that there aren't issues around awesome bar being partially visible.  Addresses most of the code review suggestions.

Comment 11

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

Comment on attachment 396347 [details] [diff] [review]
Code review changes

Don't you need to restore the browserViewportState?

Comment 12

[Benjamin Stover (:stechz)]

Actually, the viewport state isn't disturbed when the browser object is destroyed so there is no need to save it in the first place.

Comment 13

[Benjamin Stover (:stechz)]

Attachment: Removal of lockZoom

Totally unscalable lockZoom method removed in favor of a zoomChanged flag that setZoomLevel sets.  When loading, we now only zoomToPage if a page isn't being restored and the user hasn't started zooming about.

Comment 14

[Benjamin Stover (:stechz)]

Attachment: isMemoryLow is a function call

Now patch has been tested after commenting out the entire function here:
http://mxr.mozilla.org/mozilla-central/source/gfx/src/thebes/nsThebesDeviceContext.cpp#258
(see bug #509244)

A bug in font compacting caused the browser to crash before I even had a chance to catch the event.  This patch does not remove any tabs if something else managed to clear enough memory.

Comment 15

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

From IRC: Ben discovered that Browser._browsers is not updated when removing/restoring tabs. Stuart suggested we remove Browser._browsers

I agree, it is getting difficult to manage the _browser cache. However, we are keeping the property, Browser.browsers, so we need to reimplement that to always grab the live browsers.

Alternate ideas include firing events when a browser is destroyed/restored so other code can keep track. Extensions might want to monitor the DOM of the web content. When a browser is destroyed/recreated, the code could react accordingly.

Comment 16

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

Comment on attachment 397186 [details] [diff] [review]
isMemoryLow is a function call


>   dumpLn: function dumpLn() {
>-    for (var i = 0; i < arguments.length; i++) { dump(arguments[i] + ' '); }
>-    dump("\n");
>+    dump(Array.prototype.join.call(arguments, ' '));
>+    dump('\n');

Favor " for strings

>+  case f:
>+    var result = Browser.sacrificeSomeTab();
>+    if (result)
>+      dump('Freed a tab\n');
>+    else
>+      dump('There are no tabs left to free\n');
>+    break;

Here too

>+    if (this._selectedTab) {
>+      this._selectedTab.scrollOffset = this.getScrollboxPosition(this.contentScrollboxScroller);
>+    }

I wish we weren't storing the X and Y scroll offsets as a nameless array, but it works for now

>+    tab.lastSelectedTime = Date.now();

I still want tab.touched

>+    if (tab.scrollOffset) {
>+      let [scrollX, scrollY] = tab.scrollOffset;
>+      Util.dumpf('Switch tab scrolls to: %s, %s\n', scrollX, scrollY);
>+      Browser.contentScrollboxScroller.scrollTo(scrollX, scrollY);

Remove or comment out the dump

>+   * Returns true iff a tab's browser has been destroyed to free up memory.
>+   */
>+  sacrificeSomeTab: function() {

rename to sacrificeTab; sacrificeSomeTab makes it seem random.


>+var MemoryObserver = {
>+  observe: function() {
>+    dump("i'm in ur browser, clearing your memories\n");

Remove or comment out dump

>   this._loading = false;
>   this._chromeTab = null;
>+  // Set to 0 since new tabs that have not been viewed yet are good tabs to
>+  // toss if app needs more memory.
>+  this.lastSelectedTime = 0;

Add a blank line before comment

>   _resizeAndPaint: function() {
>     let bv = Browser._browserView;
> 
>     if (this == Browser.selectedTab) {
>+
>       // !!! --- RESIZE HACK BEGIN -----
>       bv.simulateMozAfterSizeChange();
>       // !!! --- RESIZE HACK END -----

No need for the blank line here

>     // in order to ensure we commit our current batch,
>     // we need to run this function here
>     this._resizeAndPaint();
>+    // if this tab was sacrificed previously, restore its state
>+    this.restoreState();

Add blank line before comment

>+  ensureBrowserExists: function() {
>+    if (!this.browser) {
>+      this._createBrowser();

Will this._browser work here? It seems like we want to test that directly


>+    state._url = doc.location.href;
>+    state._scroll = BrowserView.Util.getContentScrollValues(this.browser);
>     if (doc instanceof HTMLDocument) {

Same complaint about arrays versus named variables, but it works for now

r+ with the nits fixed

Comment 17

[Benjamin Stover (:stechz)]

Attachment: Code review / removal of _browsers

Comment 18

[Stuart Parmenter]

Comment on attachment 398487 [details] [diff] [review]
Code review / removal of _browsers

change:
+    while (memory.isLowMemory() && Browser.sacrificeSomeTab()) {
+      nsIDOMWindowUtils.garbageCollect();
+    }

to:
+    do {
+      nsIDOMWindowUtils.garbageCollect();
+    } while (memory.isLowMemory() && Browser.sacrificeSomeTab())


otherwise, this is great.

Comment 19

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

(In reply to comment #18)
> (From update of attachment 398487 [details] [diff] [review])
> change:
> +    while (memory.isLowMemory() && Browser.sacrificeSomeTab()) {
> +      nsIDOMWindowUtils.garbageCollect();
> +    }
> 
> to:
> +    do {
> +      nsIDOMWindowUtils.garbageCollect();
> +    } while (memory.isLowMemory() && Browser.sacrificeSomeTab())
> 
> 
> otherwise, this is great.

I can change that when I land. I'll also comment out a dump, change sacrificeSomeTab to sacrificeTab and change tab.lastSelectedTime to tab.lastSelected (as discussed on IRC)

Comment 20

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

pushed with changes:
https://hg.mozilla.org/mobile-browser/rev/d1841c66f06f