478470 - Cleanup SeaMonkey sessionstore code a bit.

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Misak Khachatryan]

Zeniko suggested some things to do for SM sessionstore:

Two nits for a follow-up:

>+    //this is firefox part of code, keeping it for reference only. Will be removed in final patch
>+    //misak if (winData._closedTabs && (root._firstTabs || aOverwriteTabs)) {

Wasn't removed.

>+    // restore text data saved by SeaMonkey

/SeaMonkey/Firefox 2.0\/3.0/ (that's our own legacy format which you might not
even need to support).

(From update of attachment 361771 [details] [diff] [review])
Comparing what you've checked into comm-central with what we've got in
mozilla-central, I have the following suggestions for a follow-up bug:

>+    var maxTabsUndo = this._prefBranch.getIntPref("browser.sessionstore.max_tabs_undo");

s/browser\.// (your pref branch already includes the "browser." prefix.

>+    let closedTabs = aWindow.getBrowser().savedBrowsers.map(function(e) { return e.tabData; });

This line appears a dozen time throughout your nsSessionStore.js. Worth its own
function?

>+    // parentheses are for backwards compatibility with older sessionstore files
>+    stateString.data = "(" + this._toJSONString(aStateObj) + ")";

Do you expect users to actually exchange SeaMonkey and Firefox sessionstore.js
files? If not, please do yourself a favor and drop the parens here, replace
every occurrence of evalInSandbox with JSON.parse and rename sessionstore.js to
sessionstore.json.

Comment 1

[Misak Khachatryan]

Attachment: patch

suggested code cleanup.

Comment 2

[Misak Khachatryan]

Attachment: v 1.1

Oops, missed some eval's.

Comment 3

[Simon Bünzli]

Comment on attachment 362394 [details] [diff] [review]
v 1.1

>-      // duplicate of quit-application request. Seamonkey doesn't have these yet.

Does SeaMonkey have quit-application-granted after all?

>   _safeEval: function sss_safeEval(aStr) {
>+    return JSON.parse(aStr);
>   },

_safeEval no longer does what it claims to. As it stands, it'd be best to just replace all _safeEval calls with JSON.parse.

>+  _getSavedBrowsers: function sss_getSavedBrowsers(aWindow) {

_getClosedTabData ? You don't return references to <xul:browser>s...

Comment 4

[Misak Khachatryan]

Attachment: v 1.2

(In reply to comment #3)
> Does SeaMonkey have quit-application-granted after all?
> 

It's can be used, if you observe it, because it's generated by GECKO. But code here is up to Neil to decide.

> _safeEval no longer does what it claims to. As it stands, it'd be best to just
> replace all _safeEval calls with JSON.parse.
> 

Fixed.
> _getClosedTabData ? You don't return references to <xul:browser>s...

Fixed.


This patch also fixes leaks (Bug 478506), i don't know how.

Comment 5

[Michael Kraft [:morac]]

I noticed the following error after restarting SeaMonkey, I don't know if this is a cleanup or a separate issue.

Error: this.windowToFocus.content is null
Source File: file:///C:/Program%20Files/SeaMonkey/components/nsSessionStore.js
Line: 2125

Comment 6

[Michael Kraft [:morac]]

I'm also see the following error a lot when non-browser windows are open:

Error: aWindow.getBrowser().savedBrowsers is undefined
Source File: file:///C:/Program%20Files/SeaMonkey/components/nsSessionStore.js
Line: 1505

Comment 7

[Ian Neal]

Comment on attachment 362407 [details] [diff] [review]
v 1.2

>+  _getClosedTabData: function sss_getClosedTabData(aWindow) {
>+    return aWindow.getBrowser().savedBrowsers.map(function(e) { return e.tabData; });
>+  },
>+
You've changed all the callers but not the name of the function itself.

Comment 8

[Michael Kraft [:morac]]

That could explain why calling the getClosedTabData() function returns a bunch of empty arrays.  I closed 3 tabs and then called getClosedTabData and this is what was returned:

(new String("[{},{},{}]"))

Comment 9

[Michael Kraft [:morac]]

Actually it turns out getClosedTabData() is hopelessly broken because SeaMonkey doesn't store any tabData in gBrowser.saveBrowsers.  This is why they keep coming back as an empty array.  

I filed Bug 478579 to document this.

Comment 10

[Misak Khachatryan]

Attachment: v 1.3

Fixed confusing naming and all callers.

Comment 11

[Misak Khachatryan]

(In reply to comment #5)
> I noticed the following error after restarting SeaMonkey, I don't know if this
> is a cleanup or a separate issue.
> 
> Error: this.windowToFocus.content is null
> Source File: file:///C:/Program%20Files/SeaMonkey/components/nsSessionStore.js
> Line: 2125

I didn't see that error on my console (i'm on Linux), but v 1.2 patch was completely broken. Can you retest? Sorry for bad patch.

BTW line 2125 is pointing to empty line on my local nsSessionStrore.js

Comment 13

[Simon Bünzli]

Comment on attachment 362454 [details] [diff] [review]
v 1.3

>+    total._closedTabs = this._getClosedTabs(this._windows[aWindow.__SSi]);

This shouldn't work, as this._windows is a collection of window data, not DOMWindows. Unfortunately, we don't seem to have a test for detecting this, so far...

>+    if (winData._closedTabs && (root._firstTabs || aOverwriteTabs)) {
>+      this._windows[aWindow.__SSi]._closedTabs = winData._closedTabs;
>+    }

Have you already filed a bug about the bits right after this one not working yet (i.e. the list of recently closed tabs not being restored)?

Comment 14

[Michael Kraft [:morac]]

Quick question, will this go in before SeaMonkey 2.0a3?  The reason I ask is that since this patch changes Seamonkey to use JSON.parse() to parse session while the version currently in the branch uses evalInSandbox their parameters are not compatible.  Any session data passed to nsSessionStore while the patch is applied cannot be enclosed in parenthesis, while it must be enclosed in parenthesis without the patch applied.

Basically I'm making some changes to SessionManager to better work with SeaMonkey and it can either with this patch or without it, but not both ways.


It would be kind of nice if the API functions could still take session data enclosed in parenthesis and then strip them off before calling JSON.parse(), but I can understand if that is not desired.

BTW, my development version of Session Manager that works with this patch applied is at http://downloads.mozdev.org/sessionmanager/session_manager_dev.xpi

Comment 15

[Simon Bünzli]

(In reply to comment #14)
> while it must be enclosed in parenthesis without the patch applied.

The SessionStore API asks for JSON data, so you should never (have to) enclose anything in parentheses for neither SeaMonkey nor Firefox. Firefox happens to be lenient and also accepts uneval'd objects (which are enclosed in parens) whereas SeaMonkey won't.

Can't you just change your own callers to use native JSON where available (and fall back to uneval/.toSource where not - which will be Firefox 2 and 3.0)?

Comment 16

[Robert Kaiser]

We should try to get this in for 2.0a3, yes, esp. as it fixes the leak and as it removes some eval(InSandbox) ugliness when there's a better way to do it. Also, it fixes a pref problem and changes the store format slightly if I see this correctly, so we surely should get this in before pushing it to the public.

Comment 17

[Michael Kraft [:morac]]

(In reply to comment #15)
> Can't you just change your own callers to use native JSON where available (and
> fall back to uneval/.toSource where not - which will be Firefox 2 and 3.0)?

I had actually already changed my code to check for the availability of JSON and use it, but didn't put two and two together that Firefox 3.1 and up would take pure JSON strings.  That actually makes things really simple on my end as I don't need to check to see whether it's SeaMonkey or Firefox, I can simply check for JSON.  

I'll still need to wait for this patch to drop though before officially enabling SeaMonkey support in Session Manager

Comment 18

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/347c778f306e

Comment 19

[Michael Kraft [:morac]]

What about the issue mentioned in comment #13?  

Namely that there is no way to modify the closedTabs for a window, mainly because they aren't technically maintained by SessionStore.  For example, the code I have in Session Manager to delete specific closed tabs from the Closed Tab list doesn't work even though no error is thrown.

Show a new bug be filed for that?

Comment 20

[Robert Kaiser]

Michael, one bug per issue, please. And we don't intend to switch to a different solution for undo closed tabs at this time. You may need to implement dealing with those in a different way, if possible.

Comment 21

[Misak Khachatryan]

(In reply to comment #13)
> This shouldn't work, as this._windows is a collection of window data, not
> DOMWindows. Unfortunately, we don't seem to have a test for detecting this, so
> far...
> 
> 
> Have you already filed a bug about the bits right after this one not working
> yet (i.e. the list of recently closed tabs not being restored)?

I think best i can do to get rid of this is to move restoreTab method to sessionstore and maintain _closedTabs instead of savedBrowsers, but use Seamonkey way of undo close tab. I need some investigation.  Bug 478707 filed.

Thanks Simon for your help, is it ok to put you in CC list of Seamonkey specific sessionstore bugs for while? Your comments are very valuable to me.

Comment 22

[Simon Bünzli]

(In reply to comment #21)
> > This shouldn't work, as this._windows is a collection of window data, not
> > DOMWindows.

This means that your getWindowState API is still broken, which should start annoying Micheal any time soon... ;-)

> Thanks Simon for your help, is it ok to put you in CC list of Seamonkey
> specific sessionstore bugs for while? Your comments are very valuable to me.

Sure. Some of your bugs might be mine as well, anyway.

Comment 23

[Michael Kraft [:morac]]

(In reply to comment #22)

> This means that your getWindowState API is still broken, which should start
> annoying Micheal any time soon... ;-)

And that would be today. :)

Calling getWindowState tends to throw an exception.  I filed bug 478768 documenting this problem