Closed Bug 451670 Opened 14 years ago Closed 12 years ago

Discard tab data on when low on memory

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(fennec1.0b3+)

RESOLVED FIXED
fennec1.0a2
Tracking Status
fennec 1.0b3+ ---

People

(Reporter: pavlov, Assigned: stechz)

References

Details

(Keywords: memory-footprint)

Attachments

(1 file, 6 obsolete files)

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.
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.
Flags: wanted-fennec1.0+
Target Milestone: Fennec A1 → Fennec A2
(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?
We should get this hooked back up now that the tab stuff has changed a substantial amount.
Attached patch v0.5 (obsolete) — Splinter Review
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.
Assignee: enndeakin → pavlov
Comment on attachment 352362 [details] [diff] [review]
v0.5

earliestBrowser == earliestTab ?
tracking-fennec: --- → 1.0b3+
Assignee: pavlov → webapps
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.
Attachment #352362 - Attachment is obsolete: true
Attachment #395620 - Flags: superreview+
Attachment #395620 - Flags: review+
Attachment #395620 - Flags: superreview?(pavlov)
Attachment #395620 - Flags: superreview+
Attachment #395620 - Flags: review?(combee)
Attachment #395620 - Flags: review+
Attachment #395620 - Attachment is obsolete: true
Attachment #395620 - Flags: superreview?(pavlov)
Attachment #395620 - Flags: review?(combee)
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)
> 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?
Attached patch Code review changes (obsolete) — Splinter Review
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.
Attachment #395699 - Attachment is obsolete: true
Comment on attachment 396347 [details] [diff] [review]
Code review changes

Don't you need to restore the browserViewportState?
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.
Attached patch Removal of lockZoom (obsolete) — Splinter Review
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.
Attachment #396347 - Attachment is obsolete: true
Attached patch isMemoryLow is a function call (obsolete) — Splinter Review
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.
Depends on: 509244
Depends on: 503784
No longer depends on: 509244
Attachment #396485 - Attachment is obsolete: true
Attachment #397186 - Flags: review?(mark.finkle)
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 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
Attachment #397186 - Flags: review?(mark.finkle) → review+
Attachment #397186 - Attachment is obsolete: true
Attachment #398487 - Flags: superreview?(pavlov)
Attachment #398487 - Flags: review?(mark.finkle)
Attachment #398487 - Flags: review?(mark.finkle) → review+
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.
(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)
Attachment #398487 - Flags: superreview?(pavlov)
pushed with changes:
https://hg.mozilla.org/mobile-browser/rev/d1841c66f06f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
See Also: → 865594
You need to log in before you can comment on or make changes to this bug.