899213 - Introduce Session Restore API for global data

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[Infocatcher]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

We currently have special handling for Scratchpad's windows, but don't have any similar API for extensions.
In other words, we currently can save some tab- and browser window-related data, but what about some "global" data?


Actual results:

No easy way for now to save some global state like state of third-party windows.


Expected results:

We should have something like nsISessionStore.setGlobalValue()/nsISessionStore.getGlobalValue() (and rewrite Scratchpad-related things using this public API).

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: Patch - Add an API for working with global session data, and switch Scratchpad over to it.

First draft patch which adds the API and switches scratchpad over. Currently the way scratchpad is using the API is pretty hacky, and its still integrated with Session Store.

It would be much nicer if we could move the scratchpad related code completely out of the session store, and have it only call the API. Notifications exist that would allow us to detect when data is being restored, and restore scratchpads then, but updating the global data only when state is being collected is another story.

If the scratchpad updates the state every time it is invalidated, we would be updating for every keystroke in the scratchpad window.

Tim and I discussed adding a notification for when state is being collected, to allow updating the global data before it is written, but decided we'd like to avoid adding any more notifications here.

Another option is to have the scratchpad maintain its own timer, and only update the global state on some interval.

I'd love to here any other ideas about how to properly switch the scratchpad over, and remove the code for it from sessions store completely.

try: https://tbpl.mozilla.org/?tree=Try&rev=89255834e8c5

Comment 3

[tabmix.onemen]

(In reply to Steven MacLeod [:smacleod] from comment #2)
>
> It would be much nicer if we could move the scratchpad related code
> completely out of the session store, and have it only call the API.
> Notifications exist that would allow us to detect when data is being
> restored, and restore scratchpads then, but updating the global data only
> when state is being collected is another story.
> 
> If the scratchpad updates the state every time it is invalidated, we would
> be updating for every keystroke in the scratchpad window.
> 
> Tim and I discussed adding a notification for when state is being collected,
> to allow updating the global data before it is written, but decided we'd
> like to avoid adding any more notifications here.
> 
> Another option is to have the scratchpad maintain its own timer, and only
> update the global state on some interval.
> 
> I'd love to here any other ideas about how to properly switch the scratchpad
> over, and remove the code for it from sessions store completely.
> 
In bug 886447 the notification "sessionstore-state-write" will no longer be used to allow users to change the state object. However maybe we can use it allow users to observe this and call setGlobalValue. Then _saveStateObject can collect only this._global once more.
>
>  _saveStateObject: function ssi_saveStateObject(aStateObj) {
>    Services.obs.notifyObservers(null, "sessionstore-state-write", "");
>+   aStateObj.global = this._global;
>

even better is to add notification to the start of saveState and remove the one from _saveStateObject.

>  saveState: function ssi_saveState(aUpdateAll) {
>    // If crash recovery is disabled, we only want to resume with pinned tabs
>    // if we crash.
>    TelemetryStopwatch.start("FX_SESSION_RESTORE_COLLECT_DATA_MS");
>    TelemetryStopwatch.start("FX_SESSION_RESTORE_COLLECT_DATA_LONGEST_OP_MS");
>
>+   Services.obs.notifyObservers(null, "sessionstore-before-saveState", aUpdateAll);
>+
>    var oState = this._getCurrentState(aUpdateAll);
>    if (!oState) {

Comment 4

[tabmix.onemen]

(In reply to Steven MacLeod [:smacleod] from comment #2)
> Created attachment 790836 [details] [diff] [review]
> Patch - Add an API for working with global session data, and switch
> Scratchpad over to it.

>    // get open Scratchpad window states too
>    this.setGlobalValue('scratchpads', JSON.stringify(ScratchpadManager.getSessionState()));
>
>

i don't this you need to stringify it here, the stateObj will be stringify later by _saveStateObject

Comment 5

[Steven MacLeod [:smacleod]]

(In reply to onemen.one from comment #4)
> i don't this you need to stringify it here, the stateObj will be stringify
> later by _saveStateObject

like setTabValue(), this API is meant to take strings, which is why I stringify here.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to onemen.one from comment #3)
> In bug 886447 the notification "sessionstore-state-write" will no longer be
> used to allow users to change the state object. However maybe we can use it
> allow users to observe this and call setGlobalValue. Then _saveStateObject
> can collect only this._global once more.
> 
> even better is to add notification to the start of saveState and remove the
> one from _saveStateObject.

It is our goal to not add any new notifications. We'd rather remove existing ones because they just make it harder, like in bug 886447, to improve sessionstore because they expose so much of how it currently works.

This API should work like get/setTabValue() and get/setWindowValue() without any notifications. All those values are ready to be read at the same time, after a session has been restored.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 790836 [details] [diff] [review]
Patch - Add an API for working with global session data, and switch Scratchpad over to it.

Review of attachment 790836 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, I don't see any problems here. Now that set{Tab|Window|Global}Value() are exposed via SessionStore.jsm I wonder if we should have guards that check for at least the value to be a string. People could do crazy stuff without the IDL type checks, like storing an object and keeping a reference, and afterwards modifying the object without us noticing.

This should have tests as well.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +322,5 @@
>  
>    // states for all recently closed windows
>    _closedWindows: [],
>  
> +  // state saved globaly for a session

Nit: globally

@@ +323,5 @@
>    // states for all recently closed windows
>    _closedWindows: [],
>  
> +  // state saved globaly for a session
> +  _global: {},

Maybe... _globalValues? _globalStore? _global itself isn't that expressive.

@@ +1716,5 @@
> +    this.saveStateDelayed();
> +  },
> +
> +  deleteGlobalValue: function ssi_deleteGlobalValue(aKey) {
> +    delete global[aKey];

this._global

@@ +2576,5 @@
>        windows: total,
>        selectedWindow: ix + 1,
>        _closedWindows: lastClosedWindowsCopy,
>        session: session,
> +      global: global

Nit: I think you can just pass this._global here.

::: browser/metro/components/SessionStore.idl
@@ +74,5 @@
> +   * @param aKey is the value's name.
> +   *
> +   * @returns A string value or an empty string if none is set.
> +   */
> +  AString getGlobalValue( in AString aKey);

Nit: space before in.

Comment 8

[Florian Bender]

May this system be used to store stateful data like app.update.lastUpdateTime.* (instead of storing this in prefs)?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

No, this data is saved per session. If you restore a session from two weeks ago this will surely have the wrong value. If the session is lost or never restored, so is the value.

Comment 10

[Steven MacLeod [:smacleod]]

Attachment: Patch 1 - Add an API for working with global session data, and switch Scratchpad over to it

Comment 11

[Steven MacLeod [:smacleod]]

Attachment: Patch 2 - Fix Broken Tests

Comment 12

[Steven MacLeod [:smacleod]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=c68840d0f26d

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 794225 [details] [diff] [review]
Patch 1 - Add an API for working with global session data, and switch Scratchpad over to it

Review of attachment 794225 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1827,5 @@
> +    if (lastSessionState.global) {
> +      this._globalValues = lastSessionState.global;
> +    }
> +
> +    let scratchpads = JSON.parse(this.getGlobalValue('scratchpads') || "[]");

Hmm. This should probably be wrapped in a try/catch stmt? If JSON.parse() fails or getGlobalValue() returns a falsy value we just do nothing.

@@ +2831,5 @@
> +    if (aState.global) {
> +      this._globalValues = aState.global;
> +    }
> +
> +    let scratchpads = JSON.parse(this.getGlobalValue('scratchpads') || "[]");

This should be wrapped in a try/catch stmt as well. Also, this looks a lot like the code from above, can we move this into an _updateGlobalValues() method or the like that we pass a session state? That would reduce duplication a little.

::: browser/components/sessionstore/test/Makefile.in
@@ +142,5 @@
>  	browser_739531_sample.html \
>  	browser_739805.js \
>  	browser_819510_perwindowpb.js \
>  	browser_833286_atomic_backup.js \
> +	browser_899213_global_store.js \

Let's name this browser_global_store.js. That way it's easier to find as this really covers a core feature.

::: browser/components/sessionstore/test/browser_899213_global_store.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests the API for saving global session data.
> +function test() {

It would be great if this test would use the TestRunner. It restores the initial state automatically after finishing and eases writing async tests. This is the early version of tasks before Task.jsm existed but we should be able to switch to Task.jsm easily in the future.

function test() {
  TestRunner.run();
}

function runTests() {

@@ +38,5 @@
> +    ss.deleteGlobalValue(key2);
> +    is(ss.getGlobalValue(key2), "", "global value was deleted");
> +  }
> +
> +  ss.setBrowserState(JSON.stringify(testState));

This could then be:

yield waitForBrowserState(testState, next);

Comment 14

[Steven MacLeod [:smacleod]]

Attachment: Patch 1 - Add an API for working with global session data, and switch Scratchpad over to it v2

Updated based on Tim's review.

Try: https://tbpl.mozilla.org/?tree=Try&rev=9457170b0b93

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 795551 [details] [diff] [review]
Patch 1 - Add an API for working with global session data, and switch Scratchpad over to it v2

Review of attachment 795551 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1845,5 @@
> +
> +  _restoreScratchPads: function ssi_restoreScratchPads() {
> +    let scratchpads;
> +    try {
> +      scratchpads = JSON.parse(this.getGlobalValue('scratchpads') || "[]");

You can probably omit the "[]" fallback here.

@@ +1847,5 @@
> +    let scratchpads;
> +    try {
> +      scratchpads = JSON.parse(this.getGlobalValue('scratchpads') || "[]");
> +    } catch (ex) {
> +      // Ignore any errors when attempting to decode the scratchpads.

Doesn't hurt to add a debug message here.

::: browser/components/sessionstore/test/Makefile.in
@@ +142,5 @@
>  	browser_739531_sample.html \
>  	browser_739805.js \
>  	browser_819510_perwindowpb.js \
>  	browser_833286_atomic_backup.js \
> +	browser_global_store.js \

Please move this file to the top to all the other feature tests.

Comment 16

[Steven MacLeod [:smacleod]]

Attachment: Patch - Add an API for working with global session data, and switch Scratchpad over to it

Folded both patches into a single commit for checkin

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9d9e289013de

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d9e289013de

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Hmm, now that this landed I see a couple of:

TEST-INFO | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_394759_perwindowpb.js | Console message: [JavaScript Error: "JSON.parse: unexpected end of data" {file: "resource:///modules/sessionstore/SessionStore.jsm" line: 1838}]

which comes from _restoreScratchPads(). We should maybe not try to parse the value if it's falsy.

Comment 20

[tabmix.onemen]

When restoring session with more then one window Scratchpad open multiple times

step to reproduce:
1. open 2 tow  windows
2. open Scratchpad
3. quit or restart Firefox
4. open Firefox

Result:
1. two windows restored OK
2. two identical Scratchpad windows opened instead of 1

Comment 21

[Steven MacLeod [:smacleod]]

Re-opening this to fix the regression mentioned by [e-mail], and cut down on the parsing errors.

Comment 22

[Steven MacLeod [:smacleod]]

Attachment: Patch 2 - Fixup regressions and errors from patch 1

This patch fixes the regressions and reported errors.

Comment 23

[Steven MacLeod [:smacleod]]

Try for Patch 2: https://tbpl.mozilla.org/?tree=Try&rev=06f72a2091e6

Comment 24

[Steven MacLeod [:smacleod]]

Comment on attachment 799567 [details] [diff] [review]
Patch 2 - Fixup regressions and errors from patch 1

This patch fixes one regression, but introduces another. New patch to come.

Comment 25

[Steven MacLeod [:smacleod]]

While fixing the regressions for this patch, I've come to the conclusion it was making far too many assumptions about where and when global session data is restored. It would probably be best to just back this change out to fix the regressions for now, and a new patch can re-introduce the functionality.

The patch had assumed global data should be restored at the same point scratchpads were restored from their session data. Depending on how you read the scratchpad data this will cause it to never be restored, or restored too many times. A proper patch really should support a nice way to move the scratchpad code out, and I think we should revisit the best way for code to use this API.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out at Steven's request.

https://hg.mozilla.org/mozilla-central/rev/d5ee3f89e12b

Comment 27

[Steven MacLeod [:smacleod]]

Attachment: Patch - Add a Session Store API for global data

Currently, most state is restored inside the |restoreWindow| method in SessionStore. This method is called in a number of places, and is called by the following methods to actually restore state:
 * restoreLastSession - restoreWindow is called once for each window (is passed only the windows state)
 * onLoad - restoreWindow is called once but is passed all state
 * setBrowserState - restoreWindow is called once but restores multiple windows (is passed all state)
 * setWindowState - restoreWindow is called once but restores multiple windows (is passed all state)

This difference in calling conventions ( |restoreWindow| being used to restore many windows at once vs being called multiple times to restore many windows) is what brought about the bug in my previous patch where the scratchpads were being opened for every window being restored, instead of just once each. There is definitely a potential here for cleanup, since |restoreWindow| is acting both how you'd expect, and as a |restoreWindows|.

It also means if we'd like to add the global data API without changing how this code is called, we need to restore global data outside of |restoreWindow| (which makes sense; it is global, not window, data). Thus the global data should be restored in the previously listed methods, excluding |setWindowState|.

We should really just properly move scratchpad out of Session Store entirely, and have it use the global API externally. Instead of having it make these calls in Session Store like I previously attempted, I'll write a proper patch for this separately.

Something we need to consider here as well is deferred sessions and how global state works with them. Currently, scratchpad restoration will be part of a deferred session restoration, but cannot cause a previous session to be detected. In other words, if there were scratchpads in the previous session, but no tabs to be restored, the scratchpads will be thrown away. Should global data act like this as well, or should its presence be enough to keep a deferred session? This patch currently mimics scratchpad behavior, and doesn't special case global data.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b3853edddc3f

Comment 28

[tabmix.onemen]

>+  /**
>+   * @param aKey         is the value's name.
>+   * @param aStringValue is the value itself (use JSON.stringify/parse before setting JS objects).
>+   */
>+  void setGlobalValue(in AString aKey, in AString aStringValue);

if we set global value with stringified JS object and then all session state will stringify again in SessionSaverInternal._writeState
can we get problems like in bug 467409?

the comment form _updateTextAndScrollDataForFrame make me wory....

>        // We want to avoid saving data for about:sessionrestore as a string.
>        // Since it's stored in the form as stringified JSON, stringifying further
>        // causes an explosion of escape characters. cf. bug 467409

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Steven MacLeod [:smacleod] from comment #27)
>  * setWindowState - restoreWindow is called once but restores multiple
> windows (is passed all state)

Uh... does this mean that I can actually pass multiple window states to setWindowState() and session store will restore multiple windows? We should file a bug for that and remove that. I fear though that there are people out there abusing this already :(

> This difference in calling conventions ( |restoreWindow| being used to
> restore many windows at once vs being called multiple times to restore many
> windows) is what brought about the bug in my previous patch where the
> scratchpads were being opened for every window being restored, instead of
> just once each. There is definitely a potential here for cleanup, since
> |restoreWindow| is acting both how you'd expect, and as a |restoreWindows|.

Yeah, restoreWindow() calls _openWindowWithState() if it's passed more than a single window. It would be great to clean this up in a follow-up bug. 

> Something we need to consider here as well is deferred sessions and how
> global state works with them. Currently, scratchpad restoration will be part
> of a deferred session restoration, but cannot cause a previous session to be
> detected. In other words, if there were scratchpads in the previous session,
> but no tabs to be restored, the scratchpads will be thrown away. Should
> global data act like this as well, or should its presence be enough to keep
> a deferred session? This patch currently mimics scratchpad behavior, and
> doesn't special case global data.

For now, let's just keep the current behavior. The question you posed is a good one though and probably not easy to answer. I think it really depends on the application itself, whether the data should be restored as soon as there is a part of the session restored.

(In reply to onemen.one from comment #28)
> if we set global value with stringified JS object and then all session state
> will stringify again in SessionSaverInternal._writeState
> can we get problems like in bug 467409?

This could become a problem depending on how much data extensions and other code will shove into the global value store. If you're serializing a huge object it will be serialized and escaped a second time when writing to disk, yes. It cannot get as bad as bug 467409 though because that would require the API client to pass data that already contains lots of serialized objects.

After all, methods like set{Tab|Window/|Global}Value() are really not intended to be used as huge data stores but rather to store additional identifiers that may point to data stored elsewhere. If you're thinking about passing huge serialized objects, please think again.

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 806040 [details] [diff] [review]
Patch - Add a Session Store API for global data

Review of attachment 806040 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think the global value store warrants having its own JSM but I think we would definitely benefit from moving all this code out into its own object. Other than that, LGTM!

Comment 31

[Steven MacLeod [:smacleod]]

Attachment: Patch - Add a Session Store API for global data v2

Moved the global session data out into its own object

Comment 32

[Steven MacLeod [:smacleod]]

Attachment: Patch - Add a Session Store API for global data v3

Removed some WIP code that I had missed.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 818484 [details] [diff] [review]
Patch - Add a Session Store API for global data v3

Review of attachment 818484 [details] [diff] [review]:
-----------------------------------------------------------------

Patch needs rebasing.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +4692,5 @@
> +  /**
> +   * Clear all currently stored global state.
> +   */
> +  clear: function() {
> +    this.state = {}

nit: missing semicolon

@@ +4696,5 @@
> +    this.state = {}
> +  },
> +
> +  /**
> +   * Retrieve a globally stored value.

nit: 'globally stored' is a little misleading

@@ +4738,5 @@
> +  setFromState: function (aState) {
> +    if (aState && aState.global) {
> +      this.state = aState.global;
> +    } else {
> +      this.state = {};

Maybe:

this.state = (aState && aState.global) || {};

@@ +4742,5 @@
> +      this.state = {};
> +    }
> +  }
> +
> +}

nit: empty new line

Comment 34

[Steven MacLeod [:smacleod]]

Attachment: Patch - Add a Session Store API for global data v4

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 826831 [details] [diff] [review]
Patch - Add a Session Store API for global data v4

Review of attachment 826831 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! I know I said we don't need a separate JSM but I think I changed my mind :) Would you mind filing a follow-up for this? This could also be a good first bug to mentor. r=me

::: browser/components/sessionstore/test/browser_644409-scratchpads.js
@@ +3,5 @@
>  
> +const scratchpads = [
> +  { text: "text1", executionContext: 1 },
> +  { text: "", executionContext: 2, filename: "test.js" }
> +];

Looks like we don't need the changes to that file any longer.

Comment 36

[Steven MacLeod [:smacleod]]

Attachment: Patch - Add a Session Store API for global data v5

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d5aec406f376

Comment 38

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/d5aec406f376