393716 - Add single tab save/restore to session store API

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[Manish Singh]

I propose the following API addition to nsISessionStore:

  AString getTabState(in nsIDOMNode aTab);
  void setTabState(in nsIDOMNode aTab, in AString aState);

analogous to {get,set}{Browser,Window}State, except it operates on a single tab.

If this API is acceptable, I can cook up a patch.

Comment 1

[Simon Bünzli]

What use case(s) did you have in mind for these two methods?

And please take into consideration that cookies are currently saved/restored per window and not per tab.

Comment 2

[Manish Singh]

An extension that does a full state bookmark and restore of tabs (not just URL and title, but also sessionHistory, scroll position, i.e. all the stuff session saver saves).

This is already working, but what I've done was use getWindowState and pull out the the tab I want out of the tabs array, which I feel is an abuse of the API, not to mention the wastefulness of snapshotting the other tabs I don't care about. For restore, I cut'n'pasted code from nsSessionStore.js, so that's obviously suboptimal.

I'm aware that cookies are per window. In the current implementation I just ignore them, need to think through that interaction further anyway.

Comment 3

[Simon Bünzli]

CCing Mike Shaver for a second opinion on the usefulness of the proposal (see comment #0).

(In reply to comment #2)
You can actually restore the tab also with only three or four lines of code (see the attachment at bug 298571 comment #18 and bug 344640 comment #24).

In fact, the only difference between the proposed setTabState and the existing setWindowState is that setWindowState will create a new tab for you whereas setTabState reuses an existing one.

So the main gain would be a slightly more obvious interface and a certain performance win when getting the tab state. Might still be worth it, though.

Manish: If you want to implement this, please have a look at the attachment at bug 298571 comment #23 for how to update the data related to a single tab only (just ignore the privacy override bits!). And as long as cookies aren't per tab (bug 117222) and you don't get another clever idea for what to do about them, adding a note to the interface definition might be enough.

Comment 4

[Manish Singh]

(In reply to comment #3)
> You can actually restore the tab also with only three or four lines of code
> (see the attachment at bug 298571 comment #18 and bug 344640 comment #24).
> 
> In fact, the only difference between the proposed setTabState and the existing
> setWindowState is that setWindowState will create a new tab for you whereas
> setTabState reuses an existing one.

In the extension, I've implemented the (slightly) higher level interface to take the same semantics as openUILinkIn, so I want the option to replace a current tab.

> Manish: If you want to implement this, please have a look at the attachment at
> bug 298571 comment #23 for how to update the data related to a single tab only
> (just ignore the privacy override bits!). And as long as cookies aren't per tab
> (bug 117222) and you don't get another clever idea for what to do about them,
> adding a note to the interface definition might be enough.

Will do, once I get buy in from shaver and whoever else that the API is acceptable.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think the intent of this API is on the side of the angels.  It will let the caller express more clearly the intended scope of changes, which means that any subsequent optimizations we do to avoid writing the whole session state out on every load, etc., will be more effective.  It also aids in understanding the calling code and reducing the amount of data that needs to be considered when analyzing the behaviour.

Comment 6

[Simon Bünzli]

Attachment: getTabState/setTabState/duplicateTab

This patch adds three new methods to our API:
* getTabState returns a JSON representation of the data concerning a single tab (really just the subset of what get(Window|Browser)State returns for a tab)
* setTabState loads that state into an existing tab
* duplicateTab copies a tab while ignoring the privacy_level pref (so that form data, etc. is retained for https pages as well)

The first two would now allow to easily implement undoCloseTab like functionality (in fact, our own undoCloseTab feature gets the very same data) or to move individual tabs between sessions/computers/whatever the original use case was. The third one is needed mainly for bug 298571 but will benefit extensions as well, simplifying e.g. window-merging/-splitting.

Mike: Any concerns about these additions?

Comment 7

[Manish Singh]

Thanks for implementing this! I hadn't found the time yet to flesh out an implementation myself.

Comment 8

[Simon Bünzli]

Dietrich, Mike: Please make sure that you get to this bug before Beta 3, as the UI for this back-end change has already landed...

(In reply to comment #7)
In case you find time to actually use it (or consider whether it's fit for your use case), please provide some feedback as to whether this is what you originally requested.

Comment 9

[Manish Singh]

Yes, this is exactly what I wanted! I've ported over the extension to use it (after applying the patch in the bug), and it works great.

For reference, the extension is now on AMO: https://addons.mozilla.org/en-US/firefox/addon/5756

Obviously the version there doesn't contain the code to use the new API here, but it was trivial to plug in.

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 292624 [details] [diff] [review]
getTabState/setTabState/duplicateTab

>Index: browser/components/sessionstore/nsISessionStore.idl
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/sessionstore/nsISessionStore.idl,v
>retrieving revision 1.8
>diff -u -6 -d -p -r1.8 nsISessionStore.idl
>--- browser/components/sessionstore/nsISessionStore.idl	11 Dec 2006 09:45:24 -0000	1.8
>+++ browser/components/sessionstore/nsISessionStore.idl	11 Dec 2007 18:12:33 -0000
>@@ -44,13 +44,13 @@ interface nsIDictionary;
> /**
>  * nsISessionStore keeps track of the current browsing state - i.e.
>  * tab history, cookies, scroll state, form data, POSTDATA and window features
>  * - and allows to restore everything into one window.
>  */
> 
>-[scriptable, uuid(11852a90-20de-11db-a98b-0800200c9a66)]
>+[scriptable, uuid(58d17e12-a80f-11dc-8314-0800200c9a66)]
> interface nsISessionStore : nsISupports
> {
>   /**
>    * Initialize the service
>    */
>   void init(in nsIDOMWindow aWindow);
>@@ -82,12 +82,35 @@ interface nsISessionStore : nsISupports
>    * @param aState     is a JSON string representing a session state.
>    * @param aOverwrite boolean overwrite existing tabs
>    */
>   void setWindowState(in nsIDOMWindow aWindow, in AString aState, in boolean aOverwrite);
> 
>   /**
>+   * @param aTab is the tab whose state is to be returned.
>+   * 
>+   * @return a JSON string representing the state of the tab (note: this is no
>+   *         complete session state and is to be used only with setTabState).
>+   */

can you describe what is missing and why in the comment?

s/no/not/

actually, i don't think that the note part is really necessary.

>+
>+  setTabState: function sss_setTabState(aTab, aState) {
>+    var window = aTab.ownerDocument.defaultView;
>+    
>+    var tabState = this._safeEval("(" + aState + ")");
>+    tabState._tab = aTab;

can you add some basic validation of the state data here?

nit: could get |window| after the eval lines.

also, please include xpcshell tests for the new apis.

Comment 11

[Simon Bünzli]

Attachment: nits fixed

(In reply to comment #10)
> can you describe what is missing and why in the comment?
> actually, i don't think that the note part is really necessary.

Most importantly, cookie data is missing (which we store per window), so I've left the (clarified) comment in.

> can you add some basic validation of the state data here?
> nit: could get |window| after the eval lines.

Done and done.

> also, please include xpcshell tests for the new apis.

Added basic tests as we've already got for the rest of SessionStore. Since I currently don't have a complete build environment with the test harnesses, I couldn't test them, though...

Comment 12

[Manish Singh]

Attachment: Slightly cleaned up patch

The patch didn't apply for me, as it creates a new test_bug350525.xul which already exists. I renamed to test_bug393716.xul which is what it should have been.  I also had to stick the testbody inside CDATA. I've attached a patch with these changes.

However, but the duplicateTab test fails because calling that function results in:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "aData is undefined" {file: "file:///home/manish/ffdev/objectdir/dist/bin/components/nsSessionStore.js" line: 1178}]' when calling method: [nsISessionStore::duplicateTab]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://mochikit/content/chrome/browser/components/sessionstore/test/chrome/test_bug393716.xul :: <TOP_LEVEL> :: line 86"  data: yes]
************************************************************

I haven't looked at the code in detail enough to figure out why aData is undefined there.

Comment 13

[Simon Bünzli]

Attachment: fixed test breakage

Thanks for the clean up. Turns out that we didn't properly handle tabs which were loading with an incomplete tab state (e.g. missing tab index). Now all the added tests pass (at least in my ad hoc testing environment).

Comment 14

[Manish Singh]

Tests pass here now, but later on in the test run (outside of the sessionsaver tests) I get:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "aEntry is undefined" {file: "file:///home/manish/ffdev/objectdir/dist/bin/components/nsSessionStore.js" line: 1195}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

It looks like the same problem as above, not handling incomplete tab state, so easy fix would be to check if aEntry is defined in extractHosts and bail if it isn't.

Comment 15

[Simon Bünzli]

This looks like a different issue. aEntry in that situation should never be undefined. If it were, you should've got an error when trying to restore that defective state (because in sss_deserializeHistoryEntry we make the exact same assumption). Do you have a testcase for me to reproduce this new issue?

Comment 16

[Manish Singh]

Not really. This is spewed out during the full chrome mochitest run, but many tests after the sessionsaver tests, and it doesn't appear to result in any test failures either.

Comment 17

[Simon Bünzli]

Can't debug that myself. Could you please enclose the last line of _updateCookieHosts in a try-catch block and Cu.reportError(uneval(aWindow)); in case of an exception? This should copy such a faulty window state to the Error Console and hopefully gives us a clue about what might have gone wrong.

OTOH: Do these exceptions happen without the test to this bug added to the test suite as well? If not, you could also try to comment out the ss.setTabState(...) line and the two following ok(ss.getTabValue(...)) lines (and should that not help the duplicate-tab one as well) to make sure that it's really this test which causes the issue in the first place.

Thanks for your help!

Comment 18

[Manish Singh]

Yeah, they don't happen with the test removed.

Comment 19

[Manish Singh]

Or now even with the test added again. There's other test failures and exceptions in the current code, maybe that's interfering...

Comment 20

[Simon Bünzli]

(In reply to comment #19)
> There's other test failures and exceptions in the current code

"current code" hopefully not referring to SessionStore code... (otherwise please file new bugs)

Comment 21

[Manish Singh]

Yes, sorry I wasn't clear, the session store tests all pass still.

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 299165 [details] [diff] [review]
fixed test breakage

r=me, thanks for the corrections, and tests.

Comment 23

[Reed Loden [:reed]]

Checking in browser/components/sessionstore/nsISessionStore.idl;
/cvsroot/mozilla/browser/components/sessionstore/nsISessionStore.idl,v  <--  nsISessionStore.idl
new revision: 1.10; previous revision: 1.9
done
Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.95; previous revision: 1.94
done
Checking in browser/components/sessionstore/test/chrome/Makefile.in;
/cvsroot/mozilla/browser/components/sessionstore/test/chrome/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/browser/components/sessionstore/test/chrome/test_bug393716.xul,v
done
Checking in browser/components/sessionstore/test/chrome/test_bug393716.xul;
/cvsroot/mozilla/browser/components/sessionstore/test/chrome/test_bug393716.xul,v  <--  test_bug393716.xul
initial revision: 1.1
done

Comment 24

[Steve England [:stevee]]

It looks likely this caused bug 416115

Comment 25

[Eric Shepherd [:sheppy]]

Updated the documentation at http://developer.mozilla.org/en/docs/nsISessionStore