644409 - Make scratchpads save their state across restarts

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Rob Campbell [:rc] (:robcee)]

Currently, when a person restarts Firefox, they lose all their Workspace state. On restart, all workspaces should restore their windows and text contents.

Comment 2

[Heather Arthur [:harth]]

Attachment: scratchpad-manager.jsm for opening and restoring scratchpads

Here's what I have working right now.

This adds scratchpad-manager.jsm, which is a single entry point for opening new scratchpad windows and a manager of scratchpad states for restoring across restarts.

Restoring is similar to Firefox's main session restore process. The states of open scratchpad windows are kept in a JSON file in the profile (scratchpads.json). When a new scratchpad is opened, an id is set on the window. Whenever the scratchpad wants to save its state, it calls a ScratchpadManager method, which saves the state in an internal object keyed by window id, and for the time being immediately writes this object to the JSON file.

Right now each scratchpad only saves its state on app shutdown.

There are two basic ways to do this afaics:
1) Have each scratchpad listen for app-shutdown and control when it saves its state.
2) Have the ScratchpadManager (or whatever central object) listen for app-shutdown and save the state of all the open scratchpad windows.

I went with 1 because it involved less querying of window objects, and also the scratchpad might want control over when it saves it state in the future (like right after saving the scratchpad to a file, maybe) in case of crashes.

So...Any feedback on the approach? Also, I'm curious about the file IO, which calls could be made asynchronous?

Comment 3

[Dave Camp (:dcamp)]

(In reply to Heather Arthur [:harth] from comment #2)
> So...Any feedback on the approach? Also, I'm curious about the file IO,
> which calls could be made asynchronous?

https://developer.mozilla.org/en/JavaScript_code_modules/NetUtil.jsm#asyncFetch%28%29 should make an _restoreSession async pretty easily.

Cc'ed sdwilsh, I bet he has thoughts about the right strategy for when to write this to disk.

Comment 4

[David Dahl :ddahl]

See http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#484

and 

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#509

This came up in the editor (Orion) security review - a scenario where a scratchpad could be made to open a specific file which is then executed by the user causing some kind of damage. I think this is highly unlikely in the approach and scope of what you are doing here.

Comment 5

[David Dahl :ddahl]

Another question: are you going to save state on Window close? in which case you listen for 'outer-window-destroyed', confirm the window is a scratchpad window, then do SaveMyScratchpads()...

One more thing, you should listen for 'quit-application-granted' instead of 'app-shutdown'

This is a highly-sought-after-feature, yay.

Comment 6

[Heather Arthur [:harth]]

(In reply to David Dahl :ddahl from comment #5)
> Another question: are you going to save state on Window close? in which case
> you listen for 'outer-window-destroyed', confirm the window is a scratchpad
> window, then do SaveMyScratchpads()...

Right now a scratchpad's unload handler forces a session save. I actually did that to handle the edge case where a single restored scratchpad is manually closed, that seems silly now though, and it would make more sense to just clear out the session file after restoring the previous session.

If we just want to handle restarts, it should be sufficient to restore the previous session, clear out the session store file, and have every scratchpad save it's state on shutdown.

Do we want to just go ahead and tackle crashes too though?

> One more thing, you should listen for 'quit-application-granted' instead of
> 'app-shutdown'
> 

My bad, shouldn't have made that look like a real event, it is technically listening for 'quit-application-granted'.

Comment 7

[David Dahl :ddahl]

(In reply to Heather Arthur [:harth] from comment #6)
> If we just want to handle restarts, it should be sufficient to restore the
> previous session, clear out the session store file, and have every
> scratchpad save it's state on shutdown.
> 
> Do we want to just go ahead and tackle crashes too though?
> 
You can file a follow up bug for that - I am being over-cautious perhaps:)

Comment 8

[Heather Arthur [:harth]]

Session store writes the session file asynchronously, and reading the file on startup could definitely be async, so let's do that.

For just handling restarts, we could make it really simple and do:

On shutdown, iterate through all open scratchpad windows and write their state to the JSON file.

On startup, read the file and restore the windows.


If the app happens to crash, the next time ff is opened the starting state of the previous session will be restored. Unless we want to not restore anything on startup after a crash? in which case we could just clear the session file after restoring.

Comment 9

[David Dahl :ddahl]

(In reply to Heather Arthur [:harth] from comment #8)
> If the app happens to crash, the next time ff is opened the starting state
> of the previous session will be restored. Unless we want to not restore
> anything on startup after a crash? in which case we could just clear the
> session file after restoring.

I would want to have something restored after a crash - if not the user perceives dataloss. In a followup bug, we should add an 'onidle save session' as well.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Nice patch and I like your approach. I agree with everything Dave and David have said. For async writing, you can see a decent pattern using asyncCopy in:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#464

Does the ScratchpadManager restore filenames in the titles of the scratchpads currently? 

It'd be handy if it did and also handy if on exit, scratchpads could save their contents to file if they had filenames and get persisted otherwise. Both of these cases could be done as follow-ups as there are some funny conditions that we might run into trying to black application shutdown to get input from the user.

Comment 11

[Heather Arthur [:harth]]

Attachment: Restore open scratchpads after restarting

This implements the strategy outlined in comment 8.

The filename, content, and execution context of any open scratchpads are saved when Firefox is shutdown, and restored when it's opened.

Handling crashes and saving scratchpads to their files will be separate bugs.

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 564601 [details] [diff] [review]
Restore open scratchpads after restarting

+/**
+ * The ScratchpadManager object opens new Scratchpad windows and manages the state
+ * of open scratchpads for session restore.
+ */
+var ScratchpadManager = { 

might want to call out that this is a singleton object in the comment. There can be only one! (highlander reference)

+  openScratchpad: function SPM_openScratchpad(aState)
+  {
+    let params = null;
+    if (aState) {
+      params = Cc["@mozilla.org/embedcomp/dialogparam;1"]
+               .createInstance(Ci.nsIDialogParamBlock);
+      params.SetNumberStrings(1);
+      params.SetString(0, JSON.stringify(aState));

hunh!

nit:

+
+  /**
+   * Iterate through open scratchpad windows and save their states
+   * to the session store.
+   */
+  saveOpenWindows: function SPM_saveOpenWindows() {
+    let session = [];
+

open brace should be on next line for function declarations.

+  _writeFile: function SPM_writeFile(aFile, aData, aCallback) {

here too.

+    let ostream = Cc["@mozilla.org/network/safe-file-output-stream;1"].
+                  createInstance(Ci.nsIFileOutputStream);
+    ostream.init(aFile, 0x02 | 0x08 | 0x20, PERM_MASK, ostream.DEFER_OPEN);

magic flags! These must be defined as constants somewhere?

Maybe in one of the Ci.nsIFile*Stream definitions?

+var ShutdownObserver = {
+  _initialized: false,
+
+  init: function SDO_init()
+  {
+    if (this._initialized) {
+      return;
+    }
+    Services.obs.addObserver(this, "quit-application-granted", false);
+    this.initialized = true;

should that be:

this._initialized = true; ?

The rest looks good.

I'm going to have to r- because this doesn't have a unittest attached and the above nits should be addressed.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 564601 [details] [diff] [review]
Restore open scratchpads after restarting

>+  openScratchpad: function SPM_openScratchpad(aState)

>+      params = Cc["@mozilla.org/embedcomp/dialogparam;1"]
>+               .createInstance(Ci.nsIDialogParamBlock);
>+      params.SetNumberStrings(1);
>+      params.SetString(0, JSON.stringify(aState));

If you use window.openDialog instead, you should be able to avoid the need for the XPCOM monstrosity that is nsIDialogParamBlock, and just pass the JSON directly.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 564601 [details] [diff] [review]
Restore open scratchpads after restarting

>+    Cu.import("resource:///modules/scratchpad-manager.jsm", this);

Other modules are in modules/devtools/...

Comment 15

[Heather Arthur [:harth]]

Attachment: Restore open scratchpads after restarting + tests

This patch addresses the review comments and adds two test files that test the ScratchpadManager's ability to open scratchpads and restore scratchpads.

Decided to go with ww.openWindow() instead of window.openDialog() because openDialog requires an existing parent window, and there isn't an obvious choice for one from the jsm.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 565102 [details] [diff] [review]
Restore open scratchpads after restarting + tests

Please package scratchpad-manager.jsm in the devtools directory.

Comment 17

[David Dahl :ddahl]

(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 565102 [details] [diff] [review] [diff] [details] [review]
> Restore open scratchpads after restarting + tests
> 
> Please package scratchpad-manager.jsm in the devtools directory.

I'm wondering if this object shouldn't just live in scratchpad.js ? 

We should try and avoid loading too many individual jsms, when it makes sense - I remember the console being 5 or 6 individual jsms when we started - we consolidated them all together to keep the i/o at a minimum.

Comment 18

[Mihai Sucan [:msucan]]

(In reply to David Dahl :ddahl from comment #17)
> (In reply to Dão Gottwald [:dao] from comment #16)
> > Comment on attachment 565102 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Restore open scratchpads after restarting + tests
> > 
> > Please package scratchpad-manager.jsm in the devtools directory.
> 
> I'm wondering if this object shouldn't just live in scratchpad.js ? 

It could turn ugly. scratchpad.js runs in the Scratchpad window. So for Heather to be able to access the Scratchpad Manager, she'd have to open the window before she has access to the scratchpad.js code (or otherwise load the scratchpad.js somehow).

Unless... scratchpad.js becomes a jsm. That would be a good idea, because it could export the Scratchpad and ScratchpadManager objects. The former would be used in the Scratchpad window, while the latter would be used for this bug's purposes (as Heather did).

> We should try and avoid loading too many individual jsms, when it makes
> sense - I remember the console being 5 or 6 individual jsms when we started
> - we consolidated them all together to keep the i/o at a minimum.

In either case, in browser.js we need to load a jsm.

Comment 19

[Mihai Sucan [:msucan]]

Shouldn't this respect the user preference to restore the previous browser session? I mean, if I choose to not restore my previous open tabs and windows it might also mean I do not want to restore the previous open Scratchpads.

Comment 20

[Rob Campbell [:rc] (:robcee)]

I don't think this needs to live in scratchpad.js.

regarding comment 19, I think that's something we should do in a follow-up bug.

Comment 21

[Rob Campbell [:rc] (:robcee)]

... aaaand, you should probably do as Dão asks and place the jsm in modules/devtools.

C.f., Makefile.in in the devtools/styleinspector directory for how to do that.

Comment 22

[Heather Arthur [:harth]]

Attachment: Restore open scratchpads after restarting + tests + jsm in modules/devtools

This patch just adds putting scratchpad-manager.jsm in modules/devtools.

Will file a separate bug for the pref checking.

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 565352 [details] [diff] [review]
Restore open scratchpads after restarting + tests + jsm in modules/devtools

When asking for review, you should pick one of us in the Requestee field.

+function asyncMap(items, iterator, callback)

I guess this is the async method you were looking for. Nice.

+        asyncMap(restoredWins, function(restoredWin, done) {
+          restoredWin.addEventListener("load", function() {
+            done(restoredWin.Scratchpad.getState());
+            restoredWin.close();
+          });
+        },
+        function(restoredStates) {

I find this indentation style a little difficult to match up, but I don't have a suggestion for improvement. It's just a test anyway.

+            
+          // Yay, we're done!
+          finish();
+        });
+      });
+    });
+  });

that's some nesting.

Looks good!

I'd like to see how these tests run under try, but r+ with a successful run there.

Comment 24

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I've talked with Heather about this in person, but I'm adding my 2 cents here. I know Heather's plan was to file a followup to make restoring respect session restore properly, but backwards compatibility came up. We both decided it wasn't a big deal to break people one time especially when restoring isn't something we already do.

So I'm wondering if it might be better to do the whole restoring a bit differently. As it stands we're adding more IO to startup (even if it's just to see if the file exists). We already do this in sessionstore, and we even have nice tight control over when we do restore (figuring out if a session is going to be restored/deferred-restored is a bit annoying for other components). And we already handle the crashed & about:sessionrestore case just fine.

So my thought was something along these lines:

* During a session, sessionstore would call some method to get current state from scratchpad. This would return a JS object (jsval or JSON.stringified nsXXXString using xpcom, or with JSM that's way easier). This would happen several times throughout the session so caching state on your end would be critical. Minor changes in a scratchpad probably aren't _so_ important that we need to write changes every time, so we'd only collect the data when we're writing the rest of sessionstore.js to disk.

* When restoring (or in theory at any point in time because of how sessionstore APIs are), we would call Scratchpad.setState(js object) and you would do whatever you want with that. (Probably just open new scratchpads & leave open scratchpads open).

That's the general idea. It would require minimal changes to nsSessionStore and most of the work in Heather's patch is still used - just no more IO & figuring out restoring stuff yourself.

(Sidebar: this is a step in the direction for an idea I had - session restore plugins that would allow something like Scratchpad to automagically get called like I propose to send & receive data during restore).

Comment 25

[Rob Campbell [:rc] (:robcee)]

hey Paul, thanks for dropping in here.

I like the proposal for a few reasons. Mainly, getting some IO optimization from SessionStore since you've already done a lot of that hard work already seems like a good thing to do. Making a somewhat generic sessionstore improvement for other consumers is also nice-to-have. I was kind of hoping there was some possibility of using session store for scratchpads early on when I asked you about it. Since it's *right there*.

Couple of questions:
1) Is the work to sessionstore something that can be done in parallel with this patch or should we wait for that to be done?

2) Can we land this and make use of the new bits in sessionstore when they're ready or do you think the IO impact is too painful?

3) Is there a bug on file to make the changes to SessionStore? Can we get one?

Comment 26

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

We chatted online & IRL, but for those of you following along at home...

The changes I'm proposing are minimal. I think there should be 2 methods on ScratchpadManager (getSessionState, setSessionState), which return and handle and object representing the state that scratchpad stores.

In sessionstore, we would call ScratchpadManager.getState when saving sessionstore.js to disk (or more exactly, in _getCurrentState() - need to land bug 665702...).

When restoring, we would call ScratchpadManager.setState from (I think) restoreWindowState (terribly poorly named, I know).

The more advanced changes to sessionstore won't happen for a while. Perhaps a SessionStore2. No bug yet as I haven't really formed up what is going to happen (just a wiki: https://wiki.mozilla.org/User:Zpao/SessionStore2)

Comment 27

[Heather Arthur [:harth]]

Attachment: Restore scratchpads using Session Restore

This patch implements the proposal in comment 26. It adds a scratchpad-manager.jsm singleton for opening scratchpads and keeping track of open scratchpads' states.

The ScratchpadManager queries for the states of all open scratchpad windows on shutdown and saves this to an internal object. Session restore asks ScratchpadManager for its state to save periodically. On startup session restore restores scratchpad windows.

This does not handle crashes, that will be a follow up bug. With this, no scratchpads are restored in the case of a crash.

Asking :zpao for review since most of the changes since the last patch are in the session restore code.

Comment 28

[Mihai Sucan [:msucan]]

Comment on attachment 568264 [details] [diff] [review]
Restore scratchpads using Session Restore

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

This is awesome! Really great work!

Drive-by comments below.

::: browser/base/content/browser.js
@@ +8714,5 @@
>  };
>  
> +XPCOMUtils.defineLazyGetter(Scratchpad, "ScratchpadManager", function() {
> +  Cu.import("resource:///modules/devtools/scratchpad-manager.jsm", Scratchpad);
> +  return Scratchpad.ScratchpadManager;

This getter is somewhat confusing. XPCOMUtils.defineLazyGetter() will deal with putting the ScratchpadManager object into the Scratchpad object.

You might want to use something like let foo = {}; Cu.import("", foo); return foo.bar;

::: browser/devtools/scratchpad/scratchpad-manager.jsm
@@ +76,5 @@
> +   *
> +   * @param function aCallback
> +   *        Optional. Function called when session has been restored
> +   */
> +  restoreSession: function SPM_restoreSession(aSession)

The jsdoc comment needs an update.

::: browser/devtools/scratchpad/scratchpad.js
@@ +718,5 @@
>      }
>  
> +    let initialText = this.strings.GetStringFromName("scratchpadIntro");
> +    if ("arguments" in window &&
> +         window.arguments[0] instanceof Components.interfaces.nsIDialogParamBlock) {

s/Components.interfaces/Ci/

::: browser/devtools/scratchpad/test/browser_scratchpad_open.js
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Cu.import("resource:///modules/devtools/scratchpad-manager.jsm");

No need to import. Scratchpad.ScratchpadManager should be available.

::: browser/devtools/scratchpad/test/browser_scratchpad_restore.js
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Cu.import("resource:///modules/devtools/scratchpad-manager.jsm");

Ditto.

Comment 29

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 568264 [details] [diff] [review]
Restore scratchpads using Session Restore

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

The session restore bits look ok, but r- given the following...

You haven't accounted for invalid states. If I call ScratchpadManager.restoreSession(anything not an array) it will throw, which will break in session restore and cause problems (it's probably mostly ok based on where the call is made, but unhandled exceptions are no good). Since we don't control state 100%, we need to handle to failure case somewhere. I think that should happen in SPM.restoreSession to keep the API usage as clean as possible, but I'm fine with that being in session restore too.

Along those lines, if I pass in an array of invalid states [4,5,6], what happens? It looks like 3 scratchpad windows will still be opened. Maybe that's fine, I just want to make sure it's considered.

And then finally, I think we should have a session restore test as well. We've got unit tests for SPM on your end (I still think you add a bad state test), but we should test integration to make sure we don't bust it with future changes to session restore.

Comment 30

[Heather Arthur [:harth]]

Attachment: Restore scratchpads using Session Restore

This adds a test that uses the SessionStore API's setBrowserState(), and checks for invalid state. In the case that the scratchpad's session state isn't an array or a scratchpad state isn't an object, no scratchpad window is opened.

Comment 31

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 569470 [details] [diff] [review]
Restore scratchpads using Session Restore

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

Thanks Heather! r+ on the sessionstore bits

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +2497,5 @@
> +      windows: total,
> +      selectedWindow: ix + 1,
> +      _closedWindows: lastClosedWindowsCopy,
> +      scratchpads: scratchpads
> +    };

I bitrotted you a little bit here by landing bug 665702 but that should be an easy fix.

::: browser/components/sessionstore/test/browser/browser_644409-scratchpads.js
@@ +51,5 @@
> +  return states.every(function(state) {
> +    return restored.some(function(restoredState) {
> +      return state.filename == restoredState.filename
> +        && state.text == restoredState.text
> +        && state.executionContext == restoredState.executionContext;

Tiny style nit if you get to it: put the && at the end of the line like so
return a == b &&
       c == d;

::: browser/devtools/scratchpad/scratchpad-manager.jsm
@@ +82,5 @@
> +   */
> +  restoreSession: function SPM_restoreSession(aSession)
> +  {
> +    if (!Array.isArray(aSession)) {
> +      return;

You should probably return [] so it's consistent (your jsdocs say you're returning an array)

Comment 32

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 569470 [details] [diff] [review]
Restore scratchpads using Session Restore

>+++ b/browser/components/sessionstore/test/browser/browser_644409-scratchpads.js
>+// only finish() when correct number of windows opened
>+var restored = [];
>+function addState(state) {
>+  restored.push(state);
>+
>+  if (restored.length == testState.scratchpads.length) {
>+    ok(statesMatch(restored, testState.scratchpads),
>+      "Two scratchpad windows restored");
>+
>+    Services.ww.unregisterNotification(windowObserver);

One more thing - you're not closing the scratchpad windows that you open before you finish().

>+    finish();
>+  }
>+}

Comment 33

[Rob Campbell [:rc] (:robcee)]

looks like we're just waiting for a minor patch update and then we can land this. Great work! Thanks for helping with this, Paul.

Comment 34

[Heather Arthur [:harth]]

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

Comment 35

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/78280a96ab4e