Closed Bug 776928 Opened 12 years ago Closed 12 years ago

When doing lazy tab restoration, set the <browser>s in question to display:none until restored

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 20
Tracking Status
firefox18 --- disabled
firefox19 --- disabled

People

(Reporter: bzbarsky, Assigned: ttaubert)

References

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: [MemShrink:P1])

Attachments

(1 file, 1 obsolete file)

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.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [MemShrink]
Version: unspecified → Trunk
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #645318 - Flags: review?(gavin.sharp)
Attachment #645318 - Flags: review?(dao)
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.
Attachment #645318 - Flags: review?(dao) → review-
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?
Whiteboard: [MemShrink] → [MemShrink:P1]
Attached patch patch v2Splinter Review
(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.
Attachment #645318 - Attachment is obsolete: true
Attachment #645318 - Flags: review?(gavin.sharp)
Attachment #645720 - Flags: review?(dao)
Attachment #645720 - Flags: review?(dao) → review+
https://hg.mozilla.org/integration/fx-team/rev/48bfee9fa89d
Whiteboard: [MemShrink:P1] → [MemShrink:P1][fixed-in-fx-team]
That simple patch didn't go so well... Backed out.

https://hg.mozilla.org/integration/fx-team/rev/52be83165f95
Whiteboard: [MemShrink:P1][fixed-in-fx-team] → [MemShrink:P1]
https://hg.mozilla.org/mozilla-central/rev/48bfee9fa89d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
(oops, backed out) https://hg.mozilla.org/mozilla-central/rev/52be83165f95
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> That simple patch didn't go so well... Backed out.

What went wrong?
(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.
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.
Though I would have thought the binding would have gotten attached when the scripted access happened anyway.  Did that not happen for some reason?
Oh, nevermind.  This isn't a cloned element, so no binding via precreate.

Let me see what I can do about it.
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.
Local testing indicates that once bug 787131 is fixed we can land this.  Pushing the combination to try now.
Blocks: 787131
No longer blocks: 787131
Depends on: 787131
Bug 787131 just landed on inbound, so we should be able to land this on inbound now; on other branches once inbound merges there.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ef9db325d8

I moved the CSS to tabbrowser.css as advised in comment 2.
Target Milestone: Firefox 17 → Firefox 18
https://hg.mozilla.org/mozilla-central/rev/e5ef9db325d8
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 789037
Depends on: 810584
Keywords: addon-compat
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is still on trunk, at least for now.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 817947
(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?
Compare about:memory output for a session as described in comment 0 before/after this patch?
Which section in about:memory should I look at?
The about:blank windows, specifically the layout memory.
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
Status: RESOLVED → VERIFIED
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?
Keywords: dev-doc-needed
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.
Target Milestone: Firefox 18 → Firefox 20
See Also: → 886496
You need to log in before you can comment on or make changes to this bug.