Last Comment Bug 776928 - When doing lazy tab restoration, set the <browser>s in question to display:none until restored
: When doing lazy tab restoration, set the <browser>s in question to display:no...
Status: VERIFIED FIXED
[MemShrink:P1]
: addon-compat, dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 20
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 787131 789037 810584 817947
Blocks: 681201
  Show dependency treegraph
 
Reported: 2012-07-24 08:00 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2014-01-24 17:01 PST (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled
disabled


Attachments
patch v1 (4.13 KB, patch)
2012-07-24 08:23 PDT, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Review
patch v2 (3.33 KB, patch)
2012-07-25 05:04 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-24 08:00:03 PDT
Kyle and I were looking for about:memory for a large session with tons of about:blank in to-be-restored tabs, and a large component of it is the memory needed for UA stylesheet stuff for all those pages.  Simply setting unrestored browsers to display:none will prevent them having to create a presentation and all that jazz, at least for now, saving a good bit of memory.
Comment 1 Tim Taubert [:ttaubert] 2012-07-24 08:23:49 PDT
Created attachment 645318 [details] [diff] [review]
patch v1
Comment 2 Dão Gottwald [:dao] 2012-07-24 09:49:03 PDT
Comment on attachment 645318 [details] [diff] [review]
patch v1

You shouldn't need the tabbrowser-browser class. You can just style 'browser' in tabbrowser.css. Also please document this with a comment along the lines of comment 0.
Comment 3 Dão Gottwald [:dao] 2012-07-24 09:50:30 PDT
Comment on attachment 645318 [details] [diff] [review]
patch v1

>@@ -4351,16 +4353,19 @@ let SessionStoreInternal = {
>     let browser = aTab.linkedBrowser;
> 
>     // Keep the tab's previous state for later in this method
>     let previousState = browser.__SS_restoreState;
> 
>     // The browser is no longer in any sort of restoring state.
>     delete browser.__SS_restoreState;
> 
>+    aTab.removeAttribute("pending");
>+    browser.removeAttribute("pending");

Is it a bug that the 'pending' attribute wasn't removed from the tab here?
Comment 4 Tim Taubert [:ttaubert] 2012-07-25 05:04:25 PDT
Created attachment 645720 [details] [diff] [review]
patch v2

(In reply to Dão Gottwald [:dao] from comment #3)
> >+    aTab.removeAttribute("pending");
> >+    browser.removeAttribute("pending");
> 
> Is it a bug that the 'pending' attribute wasn't removed from the tab here?

Yeah, kind of. When we're interrupting the restoration of a pending tab by restoring a completely different session (i.e. by overwriting the current session), then the pending attribute might not be removed.

This wasn't really a problem before but would be now. Also, there are other situations where _resetTabRestoringState() is called to clear a tab's restore state.
Comment 5 Tim Taubert [:ttaubert] 2012-07-25 11:56:46 PDT
https://hg.mozilla.org/integration/fx-team/rev/48bfee9fa89d
Comment 6 Tim Taubert [:ttaubert] 2012-07-25 13:37:59 PDT
That simple patch didn't go so well... Backed out.

https://hg.mozilla.org/integration/fx-team/rev/52be83165f95
Comment 7 Paul Rouget [:paul] 2012-07-25 15:12:10 PDT
https://hg.mozilla.org/mozilla-central/rev/48bfee9fa89d
Comment 8 Paul Rouget [:paul] 2012-07-25 15:14:25 PDT
(oops, backed out) https://hg.mozilla.org/mozilla-central/rev/52be83165f95
Comment 9 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-08-13 22:09:14 PDT
> That simple patch didn't go so well... Backed out.

What went wrong?
Comment 10 Tim Taubert [:ttaubert] 2012-08-21 15:15:11 PDT
(In reply to Nicholas Nethercote [:njn] from comment #9)
> > That simple patch didn't go so well... Backed out.
> 
> What went wrong?

There seems to be a lot of code that accesses browser.contentWindow and the like and just assumes that there's a docShell, etc.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-21 16:44:38 PDT
Hmm.  And the issue was that the XBL binding didn't get attached?

I wonder if we could just force XBL binding attachment here even though we're display:none to work around that issue.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-21 16:45:06 PDT
Though I would have thought the binding would have gotten attached when the scripted access happened anyway.  Did that not happen for some reason?
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-30 08:36:46 PDT
Oh, nevermind.  This isn't a cloned element, so no binding via precreate.

Let me see what I can do about it.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-30 09:25:13 PDT
Except we _do_ load bindings for display:none nodes.

What's really going on here is that the handling the "type" attribute for <xul:browser> lives in nsSubDocumentFrame.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-30 10:26:03 PDT
Local testing indicates that once bug 787131 is fixed we can land this.  Pushing the combination to try now.
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-04 20:02:25 PDT
Bug 787131 just landed on inbound, so we should be able to land this on inbound now; on other branches once inbound merges there.
Comment 17 Dão Gottwald [:dao] 2012-09-05 00:25:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ef9db325d8

I moved the CSS to tabbrowser.css as advised in comment 2.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-09-05 19:42:52 PDT
https://hg.mozilla.org/mozilla-central/rev/e5ef9db325d8
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-28 14:32:17 PST
This is still on trunk, at least for now.
Comment 21 Paul Silaghi, QA [:pauly] 2013-02-12 07:31:52 PST
(In reply to Boris Zbarsky (:bz) from comment #0)
> Simply setting
> unrestored browsers to display:none will prevent them having to create a
> presentation and all that jazz, at least for now, saving a good bit of
> memory.
Can you please tell me how to test this?
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-02-12 07:42:35 PST
Compare about:memory output for a session as described in comment 0 before/after this patch?
Comment 23 Paul Silaghi, QA [:pauly] 2013-02-15 04:21:38 PST
Which section in about:memory should I look at?
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-02-15 06:22:19 PST
The about:blank windows, specifically the layout memory.
Comment 25 Paul Silaghi, QA [:pauly] 2013-02-15 07:23:26 PST
Thanks Boris.
Indeed, major improvements with 1000 about:blank tabs:
nightly 2012-09-04 - 115.68 MB (95.78%) -- layout
nightly 2012-09-06 - 2.60 MB (34.09%) -- layout
19b6 (backout) - 133.24 MB (94.97%) -- layout
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-15 13:41:38 PST
We need to get this on the "Firefox 20 for add-on developers" doc at least.

Jorge, is this something that would merit a post on the add-ons blog as well?
Comment 27 Jorge Villalobos [:jorgev] 2013-02-15 13:52:54 PST
We will definitely include it in the Firefox 20 compat docs, and I might add it to a separate compat post that I have planned for next week.

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