Closed Bug 899213 Opened 7 years ago Closed 6 years ago

Introduce Session Restore API for global data

Categories

(Firefox :: Session Restore, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: infocatcher.bugs, Assigned: smacleod)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 10 obsolete files)

12.62 KB, patch
Details | Diff | Splinter Review
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).
Component: Untriaged → Session Restore
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
Duplicate of this bug: 592155
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
Attachment #790836 - Flags: feedback?(ttaubert)
(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) {
(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
(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.
Severity: normal → enhancement
OS: Windows 7 → All
Hardware: x86 → All
(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 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.
Attachment #790836 - Flags: feedback?(ttaubert) → feedback+
May this system be used to store stateful data like app.update.lastUpdateTime.* (instead of storing this in prefs)?
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.
Attached patch Patch 2 - Fix Broken Tests (obsolete) — Splinter Review
Attachment #794226 - Flags: review?(ttaubert)
Blocks: 908440
Attachment #794226 - Flags: review?(ttaubert) → review+
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);
Attachment #794225 - Flags: review?(ttaubert) → feedback+
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.
Attachment #795551 - Flags: review?(ttaubert) → review+
Blocks: 909789
Folded both patches into a single commit for checkin
Attachment #794226 - Attachment is obsolete: true
Attachment #795551 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9d9e289013de
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9d9e289013de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
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.
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
Re-opening this to fix the regression mentioned by onemen.one@gmail.com, and cut down on the parsing errors.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch fixes the regressions and reported errors.
Attachment #799567 - Flags: review?(dteller)
Blocks: 911992
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.
Attachment #799567 - Flags: review?(dteller)
Attachment #799567 - Attachment is obsolete: true
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.
Target Milestone: Firefox 26 → ---
Backed out at Steven's request.

https://hg.mozilla.org/mozilla-central/rev/d5ee3f89e12b
Flags: in-testsuite+
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
Attachment #796079 - Attachment is obsolete: true
Attachment #806040 - Flags: review?(ttaubert)
>+  /**
>+   * @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
(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 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!
Attachment #806040 - Flags: review?(ttaubert) → feedback+
Moved the global session data out into its own object
Attachment #806040 - Attachment is obsolete: true
Attachment #818481 - Flags: review?(ttaubert)
Removed some WIP code that I had missed.
Attachment #818481 - Attachment is obsolete: true
Attachment #818481 - Flags: review?(ttaubert)
Attachment #818484 - Flags: review?(ttaubert)
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
Attachment #818484 - Flags: review?(ttaubert) → feedback+
Attachment #818484 - Attachment is obsolete: true
Attachment #826831 - Flags: review?(ttaubert)
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.
Attachment #826831 - Flags: review?(ttaubert) → review+
Keywords: checkin-needed
Blocks: 938248
https://hg.mozilla.org/integration/fx-team/rev/d5aec406f376
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d5aec406f376
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.