838577 - [SessionStore] Cut SaveState in asynchronous chunks

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

As far as I understand, most of the Session Restore jank is caused by method |SaveState|. Since |SaveState| is asynchronous, we can subdivide it into asynchronous chunks that yield CPU back to the VM as needed.

This should be much simpler than other refactorings.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Slicing and dicing SaveState (early preview)

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 710658 [details] [diff] [review]
Slicing and dicing SaveState (early preview)

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

This looks nice. Can we maybe split this into a couple of smaller/tiny patches so that this is easier to review? TaskUtils should be moved out of the file into another JSM. Can this be merged with the other Task.js stuff we have in the code base? Also, this pattern seems quite common in the patch:

> let runner;
> if (aAsyncOptions) {
>   runner = TaskUtils.spawnCollaborative.bind(TaskUtils);
> } else {
>   runner = TaskUtils.executeImmediately;
> }
> return runner(function task() {
>   yield ...
> });

I think we should be able to remove all this boilerplate code.

As I already said on IRC I don't think we should collect the state at least twice to check for consistency. Also, I anticipate this doesn't work for some page out there in the whole web. I think we should rather listen e.g. for DOMSessionStorage changes and if that happens while we were collecting state, set a dirty flag and re-collect that information until nothing has changed and we have a consistent state. This too would be easier to tackle if we had smaller patches that convert one data collection method at a time.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> Comment on attachment 710658 [details] [diff] [review]
> Slicing and dicing SaveState (early preview)
> 
> Review of attachment 710658 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks nice. Can we maybe split this into a couple of smaller/tiny
> patches so that this is easier to review? TaskUtils should be moved out of
> the file into another JSM. Can this be merged with the other Task.js stuff
> we have in the code base?

TaskUtils and JSONUtils can definitely be moved.

> Also, this pattern seems quite common in the patch:
> 
> > let runner;
> > if (aAsyncOptions) {
> >   runner = TaskUtils.spawnCollaborative.bind(TaskUtils);
> > } else {
> >   runner = TaskUtils.executeImmediately;
> > }
> > return runner(function task() {
> >   yield ...
> > });
> 
> I think we should be able to remove all this boilerplate code.

For the moment, I wouldn't want to put this in TaskUtils.jsm (or Task.jsm) until this has matured a little. But yes, it could definitely be factored out somewhere in this module.

> As I already said on IRC I don't think we should collect the state at least
> twice to check for consistency. Also, I anticipate this doesn't work for
> some page out there in the whole web. I think we should rather listen e.g.
> for DOMSessionStorage changes and if that happens while we were collecting
> state, set a dirty flag and re-collect that information until nothing has
> changed and we have a consistent state.

Fair enough, but how do we test that?

> This too would be easier to tackle
> if we had smaller patches that convert one data collection method at a time.

I can certainly do that. Note, however, that conversion of data collections is actually the smallest part of this patch.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> > As I already said on IRC I don't think we should collect the state at least
> > twice to check for consistency. Also, I anticipate this doesn't work for
> > some page out there in the whole web. I think we should rather listen e.g.
> > for DOMSessionStorage changes and if that happens while we were collecting
> > state, set a dirty flag and re-collect that information until nothing has
> > changed and we have a consistent state.
> 
> Fair enough, but how do we test that?

I think there's also the question what data is actually invalidated by changes to DOMSessionStorage data. SessionStorage.serialize() is synchronous at the moment and collects data for all history entries (for the hosts of all entries). Only the current entry's host data can change but not while we're serializing it synchronously.

If we'd however split SessionStorage.serialize() in to chunks that allow processing the event loop in between, there is a chance that SessionStorage data might change and writing a test should be relatively straight forward, no?

> > This too would be easier to tackle
> > if we had smaller patches that convert one data collection method at a time.
> 
> I can certainly do that. Note, however, that conversion of data collections
> is actually the smallest part of this patch.

I know, I'm just trying to somehow cut this into smaller pieces :)

Comment 5

[David Teller [:Yoric] - still alive but not very active]

From the top of my head, changes to any of the following could cause the session store to become invalid:
- List of windows (How do we detect that?)
- List of tabs in a window (How do we detect that?)
- Document in a tab (How do we detect that?)
- List of iframe documents in a document (How do we detect that?)
- History of an iframe (How do we detect that?)
- History of a tab (How do we detect that?)
- SessionStorage of a document (How do we detect that?)
- Form fields of a document (How do we detect that?)
- Additional extData (How do we detect that?)

Right now, I can't think of any simple way to detect that we should restart an ongoing collection because some of its information has become invalid. Any idea?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> - List of windows (How do we detect that?)
nsIWindowWatcher (see SS.onLoad, onClose)

> - List of tabs in a window (How do we detect that?)
TabOpen/Close/Move/Show/Hide events (see SS.handleEvent etc.)

> - Document in a tab (How do we detect that?)
load events, Web/TabProgressListeners, nsISHistoryListener?

> - List of iframe documents in a document (How do we detect that?)
nsISHistoryListener.OnNewEntry (I think)

> - History of an iframe (How do we detect that?)
nsISHistoryListener.OnNewEntry/Purge/etc.

> - History of a tab (How do we detect that?)
nsISHistoryListener.OnNewEntry/Purge/etc.

> - SessionStorage of a document (How do we detect that?)
MozStorageChanged (bug 727446)

> - Form fields of a document (How do we detect that?)
Events: change/input/onDOMAutoComplete (see SS.handleEvent)

> - Additional extData (How do we detect that?)
We'd need to check this when a client calls setTabState/setWindowState

(I copied those points over from an Etherpad, sorry for the brevity.)

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Slicing and dicing SaveState

Attaching a new version.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

As discussed offline with tim, I will try and cut the patch into smaller chunks.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Utilities

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Utilities, v2

First chunk: utilities for refactoring code.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Invalidating the session when necessary

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous

Third and final part of the second prototype for this bug.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous

Same prototype, with a little parasite debugging code removed.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 730672 [details] [diff] [review]
3. Making some elements of data collection asynchronous

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

This looks like a good start to me. I wonder if we should split up data collection even more than just into cuts per window. How many people use multiple windows? But maybe we should keep it small for a start and not introduce too much churn :)

I like the fact the we have a sync and an async runner that run the same code with different behavior, so there's no code duplication. Although I had quite a hard time understanding all the details of the runner code at first sight and I wonder if we could maybe simplify this a little by for example not letting them inherit just a single tiny constructor.

The invalidation code is straight forward I think though we should check that each invalidation is there for a reason and we invalidate whenever we need to.

_getCurrentState() defaults to an async runner which is good but should be called with a sync runner by .getBrowserState() as I think we shouldn't touch that API for now.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #15)
> Comment on attachment 730672 [details] [diff] [review]
> 3. Making some elements of data collection asynchronous
> 
> Review of attachment 730672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like a good start to me. I wonder if we should split up data
> collection even more than just into cuts per window. How many people use
> multiple windows? But maybe we should keep it small for a start and not
> introduce too much churn :)

That was the idea.

> I like the fact the we have a sync and an async runner that run the same
> code with different behavior, so there's no code duplication. Although I had
> quite a hard time understanding all the details of the runner code at first
> sight and I wonder if we could maybe simplify this a little by for example
> not letting them inherit just a single tiny constructor.

Ok, will do.

> The invalidation code is straight forward I think though we should check
> that each invalidation is there for a reason and we invalidate whenever we
> need to.

I don't understand that sentence.

> _getCurrentState() defaults to an async runner which is good but should be
> called with a sync runner by .getBrowserState() as I think we shouldn't
> touch that API for now.

Actually, unless I have made a mistake, it defaults to a sync runner.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

So, here's my plan for the next iteration:
- Simplify trivially the code of SyncRunner/AsyncRunner.
- Split up data collection into chunks per tab, rather than chunks per window, because doing it per window probably means that the test suite isn't testing anything meaningful.
- Understand your sentence about invalidation code.

Is that ok with you?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Utilities, v3

In that case, let's start and get ready to land this.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous, v2

Let's make this a little more useful and fragile: this version now yields some time between collection of history for successive tabs.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> - Understand your sentence about invalidation code.

Haha, sorry. I meant to say we should check we didn't forget to invalidate when data has changed so that we cover all possible situations. I checked the patch and thought that it looks good but didn't take the time to do exactly that :)

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 739723 [details] [diff] [review]
1. Utilities, v3

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

::: browser/components/sessionstore/src/Makefile.in
@@ +22,5 @@
>    DocumentUtils.jsm \
>    SessionStorage.jsm \
>    XPathGenerator.jsm \
>    _SessionFile.jsm \
> +  _AsyncUtils.jsm \

Nit: Let's make that in alphabetical order.

::: browser/components/sessionstore/src/_AsyncUtils.jsm
@@ +1,4 @@
> +/**
> + * Utilities designed to simplify refactoring SessionStore into
> + * something more asynchronous. Some of these utilities might,
> + * at a later stage, become public.

We'll need a license header here as well.

@@ +10,5 @@
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;

We don't use Cr.

@@ +43,5 @@
> +   * @param {Generator} task A (synchronous) generator. Its iterator
> +   * will be called repeatedly until one of the following things happens:
> +   * - it reaches the end of the iterator (i.e. it hits StopIteration
> +   *    or Task.Result), in which case it returns the corresponding result;
> +   * - it receives an error, in which case the error is propagated.

A list of all possible options would be great.

@@ +47,5 @@
> +   * - it receives an error, in which case the error is propagated.
> +   *
> +   * If this generator returns a promise, this is most likely an error.
> +   */
> +  run: function run(task, options = {}) {

I was recently reminded that we no longer need to assign function names in order to have informative stack traces. We can just assign anonymous functions to property names.

@@ +53,5 @@
> +    // Accept functions that do not return anything.
> +    if (!iteration) {
> +      return;
> +    }
> +    let value = undefined;

let value;

@@ +61,5 @@
> +        value = iteration.send(value);
> +        if (ensureNoPromise) {
> +          if (typeof value === "object" && "then" in value
> +              && typeof value.then === "function") {
> +            debug("Warning: synchronous iterator has returned a promise " + new Error().stack);

I think 'ensurePromise' is a little misleading, it doesn't actually ensure anything, it just prints a warning. What is that used for?

@@ +64,5 @@
> +              && typeof value.then === "function") {
> +            debug("Warning: synchronous iterator has returned a promise " + new Error().stack);
> +          }
> +        }
> +      } catch (ex if ex == StopIteration) {

} catch (ex if ex instanceof StopIteration) {

@@ +77,5 @@
> +   * An implementation of |wait| that does not perform any waiting.
> +   *
> +   * @param {*} data Any value.
> +   */
> +  wait: function wait(data) {

wait() is IMO not a good function name for this as it doesn't convey the function's purpose. yield() would be a good name but that's already taken, so... any other ideas?

@@ +96,5 @@
> + * @param {object} scheduling A set of arguments specifying how the task
> + * should be scheduled. This object should contain the following fields:
> + * - {number} chunkDuration Determines the duration of a chunk of execution.
> + *   Method |wait| will yield time back to the system once |chunkDuration|
> + *   milliseconds are elapsed.

You should document 'delay' here as well.

@@ +131,5 @@
> +   * @type {number} Milliseconds.
> +   */
> +  this._longestOpDuration = -1;
> +};
> +AsyncRunner.prototype = {

We could define _started, _switchDate and _longestOpDuration in the prototype. No need to have them in the constructor.

@@ +139,5 @@
> +   * The computation will be interrupted during the next call to |wait|.
> +   */
> +  throw: function(error) {
> +    this._throw = error;
> +  },

Can we please place this function after the definition of wait()? This completely interrupts my reading flow.

@@ +148,5 @@
> +   * This method initializes internal data then executes |run|. By opposition
> +   * to |run|, it is not reentrant. This method is meant to be called by the
> +   * creator of the runner.
> +   */
> +  start: function start(task) {

So if the SyncRunner does not have a .start() method how will SS use those different runners? I thought they should have the same interface? The same for throw() which is also to be called by the client?

@@ +157,5 @@
> +    this._switchDate = Date.now();
> +    return this.run(task).then(
> +      (function onSuccess(result) {
> +        this._started = false;
> +      }).bind(this),

Fat arrow FTW.

@@ +168,5 @@
> +        } else {
> +          debug("Current stack");
> +          debug(new Error().stack);
> +        }
> +      }).bind(this)

F-F-F-Fat arrow.

@@ +200,5 @@
> +    // If we have been invalidated, stop computation
> +    if (this._throw) {
> +      let error = this._throw;
> +      this._throw = null;
> +      throw error;

Should we also set this._started=false here?

@@ +210,5 @@
> +        (function onSuccess() {
> +          this._switchDate = Date.now();
> +          return value;
> +        }
> +      ).bind(this));

You can use a fat arrow function here, too.

@@ +217,5 @@
> +    // Otherwise, just return
> +    return value;
> +  },
> +  toString: function toString() {
> +    return "AsyncRunner";

Where's the name? :)

@@ +227,5 @@
> +   * @type {number} milliseconds
> +   */
> +  get longestOpDuration() {
> +    return this._longestOpDuration;
> +  }

It's not a convention but I kind of like to have getters and setters at the beginning of an object.

@@ +237,5 @@
> +let TaskUtils = {
> +  /**
> +   * A shared instance of SyncRunner
> +   */
> +  sync: new SyncRunner(),

Want to give it a name? "Shared" or something?

@@ +242,5 @@
> +
> +  /**
> +   * Return a promise resolved after a given number of milliseconds
> +   */
> +  wait: function(ms) {

I actually think this could be a function in the Promise core. Like Promise.wait()?

@@ +244,5 @@
> +   * Return a promise resolved after a given number of milliseconds
> +   */
> +  wait: function(ms) {
> +    let deferred = Promise.defer();
> +    let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

The timer might never fire if the nsITimer instance is GC'ed. How about having a Set of timers that keeps references? Set.add() on initialization and Set.delete() when fired should do the trick. Alternatively you could also tack it onto the promise - if the promise is GC'ed so is the timer then and nobody cares anymore.

@@ +247,5 @@
> +    let deferred = Promise.defer();
> +    let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +    let jit = Components.utils.methodjit;// Workaround for bug 776798
> +    timer.initWithCallback(function timerCallback() {
> +      Components.utils.methodjit = jit;// Second part of workaround

Wow, um. Should we really include the workaround here? We could instead put a little pressure on this bug being fixed. We have quite some code that is called by timer callbacks that's not jitted...

Comment 22

[David Teller [:Yoric] - still alive but not very active]

>> +	   value = iteration.send(value);
>> +	   if (ensureNoPromise) {
>> +	     if (typeof value === "object" && "then" in value
>> +		 && typeof value.then === "function") {
>> +	       debug("Warning: synchronous iterator has returned a promise " +
> new Error().stack);
> 
> I think 'ensurePromise' is a little misleading, it doesn't actually ensure
> anything, it just prints a warning. What is that used for?

Ah, changed the name of the option and added a comment with some details.

> @@ +64,5 @@
>> +		 && typeof value.then === "function") {
>> +	       debug("Warning: synchronous iterator has returned a promise " +
> new Error().stack);
>> +	     }
>> +	   }
>> +	 } catch (ex if ex == StopIteration) {
> 
> } catch (ex if ex instanceof StopIteration) {

I don't think this is possible. StopIteration is a singleton.

> 
> @@ +77,5 @@
>> +   * An implementation of |wait| that does not perform any waiting.
>> +   *
>> +   * @param {*} data Any value.
>> +   */
>> +  wait: function wait(data) {
> 
> wait() is IMO not a good function name for this as it doesn't convey the
> function's purpose. yield() would be a good name but that's already taken,
> so... any other ideas?

What about |coyield|?

> @@ +131,5 @@
>> +   * @type {number} Milliseconds.
>> +   */
>> +  this._longestOpDuration = -1;
>> +};
>> +AsyncRunner.prototype = {
> 
> We could define _started, _switchDate and _longestOpDuration in the prototype.
> No need to have them in the constructor.

I personally dislike the practice of putting bogus properties in the prototype just for the sake of documentation. I realize that I may be in minority, though, so if you insist, I will yield.

> @@ +148,5 @@
>> +   * This method initializes internal data then executes |run|. By
> opposition
>> +   * to |run|, it is not reentrant. This method is meant to be called by the
> 
>> +   * creator of the runner.
>> +   */
>> +  start: function start(task) {
> 
> So if the SyncRunner does not have a .start() method how will SS use those
> different runners? I thought they should have the same interface? The same for
> throw() which is also to be called by the client?

I have resplit the code to make this clearer.

> @@ +242,5 @@
>> +
>> +  /**
>> +   * Return a promise resolved after a given number of milliseconds
>> +   */
>> +  wait: function(ms) {
> 
> I actually think this could be a function in the Promise core. Like
> Promise.wait()?

It's definitely more generic than this module. However, let's stabilize Promise first and add features later :)

> @@ +244,5 @@
>> +   * Return a promise resolved after a given number of milliseconds
>> +   */
>> +  wait: function(ms) {
>> +    let deferred = Promise.defer();
>> +    let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> 
> The timer might never fire if the nsITimer instance is GC'ed. How about having
> a Set of timers that keeps references? Set.add() on initialization and
> Set.delete() when fired should do the trick. Alternatively you could also tack
> it onto the promise - if the promise is GC'ed so is the timer then and nobody
> cares anymore.

Storing it in the promise is fragile, but you're right, I should put it in a |Set|.

> @@ +247,5 @@
>> +    let deferred = Promise.defer();
>> +    let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>> +    let jit = Components.utils.methodjit;// Workaround for bug 776798
>> +    timer.initWithCallback(function timerCallback() {
>> +	 Components.utils.methodjit = jit;// Second part of workaround
> 
> Wow, um. Should we really include the workaround here? We could instead put a
> little pressure on this bug being fixed. We have quite some code that is called
> by timer callbacks that's not jitted...

Applying pressure at the moment :)

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Utilities, v4

New version of the Utilities:
- split |AsyncRunner| in two classes - |AsyncRunner|/|AsyncController|;
- renamed |wait| into |coyield| and made that method smarter, too;
- removed method |start|;
- added a test suite;
- applied feedback.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous, v3

Propagating _AsyncUtils API changes to the main patch.

Comment 25

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

(In reply to David Rajchenbach Teller [:Yoric] from comment #22)
> >> +	 } catch (ex if ex == StopIteration) {
> > 
> > } catch (ex if ex instanceof StopIteration) {
> 
> I don't think this is possible. StopIteration is a singleton.

Well, the test certainly passes in the needed cases:
http://mxr.mozilla.org/mozilla-central/search?string=instanceof%20StopIteration

Seems like your version is closer to what was originally intended, though.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #25)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #22)
> > >> +	 } catch (ex if ex == StopIteration) {
> > > 
> > > } catch (ex if ex instanceof StopIteration) {
> > 
> > I don't think this is possible. StopIteration is a singleton.
> 
> Well, the test certainly passes in the needed cases:
> http://mxr.mozilla.org/mozilla-central/
> search?string=instanceof%20StopIteration
> 
> Seems like your version is closer to what was originally intended, though.

FWIW, I see lots of 'instanceof StopIteration' in browser/ and toolkit/. The only ones using '==' are OS.File files :) I don't really care that much about it, though.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous, v4

I just fixed a few oddities while investigating aggressive caching (beyond the scope of this bug).

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 742934 [details] [diff] [review]
1. Utilities, v4

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

Yay, tests!

::: browser/components/sessionstore/src/_AsyncUtils.jsm
@@ +51,5 @@
> +   * - it receives an error, in which case the error is propagated.
> +   *
> +   * @param {object*} options Optionally, an object with a subset of
> +   * the following fields:
> +   * - {bool} warningValueIsNotAPromise (default: |false|). If |true|,

Nit: maybe warnIfValueIsNotAPromise? warnIfValueNotPromise?

@@ +164,5 @@
> + * - {number} chunkDuration Determines the duration of a chunk of execution.
> + *   Method |coyield| will yield time back to the system once |chunkDuration|
> + *   milliseconds are elapsed.
> + * - {number} delay Determines how many milliseconds will be yielded back to the
> + *  system after |chunkDuration| are elapsed.

That documentation should be moved to the controller.

@@ +172,5 @@
> + */
> +function AsyncRunner(controller) {
> +  this._controller = controller;
> +}
> +AsyncRunner.prototype = {

Should this have a toString() printing AsyncRunner and the controller's name?

@@ +277,5 @@
> +
> +this._AsyncUtils = {
> +  TaskUtils: TaskUtils,
> +  AsyncController: AsyncController,
> +  SyncRunner: SyncRunner

Do we need to export the SyncRunner? Doesn't seem to be used anywhere. Maybe we could assign the shared instance to it and access that instead of TaskUtils.sync in SessionStore.jsm?

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 743264 [details] [diff] [review]
3. Making some elements of data collection asynchronous, v4

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +581,5 @@
>     */
>    _uninit: function ssi_uninit() {
>      // save all data for session resuming
>      if (this._sessionInitialized)
> +      this.saveState(true, true);

Huh. Let's not extend the boolean trap. Can you please re-write saveState to be:

saveState({updateAll: true, forceSync: true});

@@ +1906,5 @@
>      var tabs = tabbrowser.tabs;
>      var tabsData = this._windows[aWindow.__SSi].tabs = [];
>  
> +    return aRunner.run((function task() {
> +      for (var i = 0; i < tabs.length; i++) {

for (let tab of tabs) {

@@ +2891,5 @@
>        this._windows[aWindow.__SSi]._closedTabs = winData._closedTabs || [];
>      }
>  
>      this.restoreHistoryPrecursor(aWindow, tabs, winData.tabs,
> +      (aOverwriteTabs ? (parseInt("selected" in winData?winData.selected:1) || 1) : 0), 0, 0);

Why not parseInt(winData.selected || 1)?

@@ +3743,5 @@
> +  _saveStateAsynchronously: true,
> +
> +  // Number of runs of save state completed so far
> +  // (used for debugging purposes)
> +  _saveStateRunId: 0,

So... how about having a separate object like 'StateSaver' (or a better name) that contains the saveState() and _saveStateObject() functionality? Basically, the three steps are collecting the data (now async), cleaning it up a little and then writing it to the disk. I assume this could be done much nicer without having a *huge* method if we move it out of SessionStoreInternal.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #29)
> @@ +3743,5 @@
> > +  _saveStateAsynchronously: true,
> > +
> > +  // Number of runs of save state completed so far
> > +  // (used for debugging purposes)
> > +  _saveStateRunId: 0,
> 
> So... how about having a separate object like 'StateSaver' (or a better
> name) that contains the saveState() and _saveStateObject() functionality?
> Basically, the three steps are collecting the data (now async), cleaning it
> up a little and then writing it to the disk. I assume this could be done
> much nicer without having a *huge* method if we move it out of
> SessionStoreInternal.

Generally, I like the idea. However, this looks like a large refactoring: we're certainly going to want to put all data collection methods and state in that StateSaver object, and I would rather not do this right now. Do you mind keeping this for a followup bug?

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Utilities, v5

Applied your feedback.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous, v5

Applied most of your feedback - I haven't moved saving to its own object.

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Attachment: 4. Backing up on first use

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Invalidating the session when necessary, v2

Comment 35

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous, v6

Considerable refactoring of the code to make it easier to test.

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Attachment: 5. Progressively adapting the test suite (WIP)

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous, v7

Fixed small stuff.

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Attachment: 5. Adapted a number of tests

I have adapted 13 tests to the async API. I hope this is sufficient for the moment.

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Attachment: 5. Adapted a number of tests v2

A few fixes.

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Attachment: 4. Backing up on first use v2

Testsuite fix

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous, v8

Fixed a race condition when two calls to |saveState| happened concurrently. Also cleaned up the code meant to handle this situation.

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 746456 [details] [diff] [review]
2. Invalidating the session when necessary, v2

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

r=me with all issues addressed.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +691,1 @@
>      }

I don't think we need this ugly try-catch stmt here. Exceptions should be reported anyway if uncaught.

@@ +756,5 @@
>        case "TabUnpinned":
>          this.saveStateDelayed(win);
>          break;
> +      case "MozStorageChanged":
> +        this.saveStateDelayed();

You're missing the aWindow parameter here. No DOMSessionStorage data will be collected if the window isn't marked as dirty.

@@ +4699,5 @@
> +  },
> +  OnHistoryPurge: function(aNumEntries) {
> +    AsyncInvalidation.fire();
> +    return true;
> +  },

We don't need to touch any of the above (except OnHistoryPurge) because all of them will emit 'load' or 'pageshow' events which lead to saveStateDelayed() calls.

@@ +4704,2 @@
>    OnHistoryReload: function(aReloadURI, aReloadFlags) {
> +    AsyncInvalidation.fire();

This will also fire 'load' when the tab has been restored. No need to invalidate here.

@@ +4762,5 @@
> +  /**
> +   * Inform all observers that a change has taken pace, which may require
> +   * re-collecting session state.
> +   *
> +   * @param {*=} event, Optionally an event, to be sent to all listeners.

Isn't this more a 'reason' than an 'event'?

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 744784 [details] [diff] [review]
1. Utilities, v5

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

That looks like the wrong patch to me.

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Utilities, v6

Oops, sorry about that.

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 752157 [details] [diff] [review]
1. Utilities, v6

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

Thanks!

::: browser/components/sessionstore/src/_AsyncUtils.jsm
@@ +264,5 @@
> + * Return a promise resolved after a given number of milliseconds
> + *
> + * @return {promise}
> + */
> +let wait = function(ms) {

Nit: function wait(ms) {

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 746456 [details] [diff] [review]
2. Invalidating the session when necessary, v2

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3706,4 @@
>      if (aWindow) {
>        this._dirtyWindows[aWindow.__SSi] = true;
>      }
> +    AsyncInvalidation._invalidate();

Didn't see this. Should be .fire().

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 751378 [details] [diff] [review]
5. Adapted a number of tests v2

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

Can we move this patch to a follow-up bug? It doesn't seem necessary to land the improvements here, does it?

Comment 48

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #47)
> Comment on attachment 751378 [details] [diff] [review]
> 5. Adapted a number of tests v2
> 
> Review of attachment 751378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we move this patch to a follow-up bug? It doesn't seem necessary to land
> the improvements here, does it?

Well, my goal with these tests was to ensure that we hit as much as possible of the async codepath. But yes, I guess that we can just land the two new tests and keep the rest for a followup bug.

Comment 49

[David Teller [:Yoric] - still alive but not very active]

Attachment: 5. A few tests

Just the two tests directly relevant to this bug. If you have ideas of additional tests, I'm all ears.

Comment 50

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 751379 [details] [diff] [review]
4. Backing up on first use v2

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

I don't really like collecting return values and temporarily exposing '_promiseSecondaryBackupComplete' just for testing. Couldn't the test just start and check if any of sessionstore.{js,bak} exists and then waits until the secondary backup file exists? We could also check if the pref exists, if so we can assume the file must be there, if not we'll need to wait for it. That way we wouldn't have to expose anything but still know whether we need to wait at all for the secondary backup to complete.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +482,5 @@
>      // A Lazy getter for the sessionstore.js backup promise.
> +    if (BACKUP_BEFORE_FIRST_USE_OF_ASYNC_SESSION_RESTORE) {
> +      XPCOMUtils.defineLazyGetter(this, "_backupSessionFileOnce", function () {
> +        return this._promiseSecondaryBackupComplete.promise.then(
> +          _ => _SessionFile.createBackupCopy()

Nit: |() => _SessionFile.createBackupCopy()|. Underscore is not a pattern wildcard but will represent an argument.

@@ +513,5 @@
> +        let backupCreated = false;
> +        try {
> +          if (mustBackupForMigration) {
> +            backupCreated = yield _SessionFile.createSecondaryBackupCopy(".ff23-migration");
> +            this._prefBranch.setBoolPref("sessionstore.async.ff23.backup-required", false);

"sessionstore.async.ff24.backup-required", my bad.

::: browser/components/sessionstore/test/browser_838577_backup.js
@@ +26,5 @@
> +      is(pref, false, "The preference has been set");
> +    } finally {
> +      finish();
> +    }
> +  }.bind(this));

.bind(this) seems unnecessary.

Comment 51

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous, v9

Just merged some code.

Comment 52

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #50)
> Comment on attachment 751379 [details] [diff] [review]
> 4. Backing up on first use v2
> 
> Review of attachment 751379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really like collecting return values and temporarily exposing
> '_promiseSecondaryBackupComplete' just for testing. Couldn't the test just
> start and check if any of sessionstore.{js,bak} exists and then waits until
> the secondary backup file exists? We could also check if the pref exists, if
> so we can assume the file must be there, if not we'll need to wait for it.
> That way we wouldn't have to expose anything but still know whether we need
> to wait at all for the secondary backup to complete.

Actually, I'm not happy at all with this test. I haven't been able to create sessionstore.js before initialization of SessionStore, so I can't check for existence of a backup. Do you have any idea?

Comment 53

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #42)
> Comment on attachment 746456 [details] [diff] [review]
> 2. Invalidating the session when necessary, v2
> 
> Review of attachment 746456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with all issues addressed.
> 
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +691,1 @@
> >      }
> 
> I don't think we need this ugly try-catch stmt here. Exceptions should be
> reported anyway if uncaught.


If my memory serves, the problem was that exceptions were not reported. Or perhaps just their stack, I'm not sure.

Comment 54

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 752298 [details] [diff] [review]
5. A few tests

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

::: browser/components/sessionstore/test/browser_838577_async_save.js
@@ +12,5 @@
> +const TOPIC_INVALIDATION = "sessionstore-async_save-invalidation";
> +
> +function test() {
> +  Services.prefs.setBoolPref("browser.sessionstore.async.notify-progress", true);
> +  Services.prefs.setBoolPref("browser.sessionstore.restore_on_demand", true);

You should register a cleanup function that calls clearUserPref() for both of those.

@@ +17,5 @@
> +  waitForExplicitFinish();
> +  registerCleanupFunction(function() {
> +    info("Cleanup starting");
> +    try {
> +      ss.setBrowserState(originalState);

This should all be done by the test suite. We don't need to do that in every test anymore.

@@ +31,5 @@
> +}
> +
> +let tests = [];
> +
> +let add_task = function(task) {

function add_task(task) {

@@ +77,5 @@
> +add_task(function test_1() {
> +  info("Testing that data collection eventually succeeds");
> +  let deferreds = {
> +    success: Promise.defer()
> +  };

What's the point of having a 'deferreds' map? Looks like we always just have a single 'success' deferred.

@@ +78,5 @@
> +  info("Testing that data collection eventually succeeds");
> +  let deferreds = {
> +    success: Promise.defer()
> +  };
> +  let observe = function(subject, topic, data) {

function observe(subject, topic, data) {

@@ +81,5 @@
> +  };
> +  let observe = function(subject, topic, data) {
> +    SimpleTest.info("Observing " + topic + ": " + data);
> +    if (data == "saveState") {
> +      SimpleTest.info("progress: success");

Nit: Just info() should be sufficient.

@@ +83,5 @@
> +    SimpleTest.info("Observing " + topic + ": " + data);
> +    if (data == "saveState") {
> +      SimpleTest.info("progress: success");
> +      deferreds.success.resolve();
> +      return;

Nit: we don't need that return.

@@ +95,5 @@
> +  win.gBrowser.addTab("about:robots");
> +  win.gBrowser.addTab("about:config");
> +  info("Tabs opened");
> +  yield deferreds.success.promise;
> +  Services.obs.removeObserver(observe, TOPIC_PROGRESS);

We should make sure the observer is removed even if we time out and never receive the notification.

@@ +103,5 @@
> +add_task(function test_2() {
> +  // Ensure that we always wait
> +  Services.prefs.setIntPref("browser.sessionstore.async.chunk-duration", 0);
> +  // Ensure that we have time to open a window/tab during the interval
> +  Services.prefs.setIntPref("browser.sessionstore.async.reschedule-after", 300);

Needs clearUserPref() in a cleanup function. I know it's at the end of the function but if something goes wrong we're stuck with those values.

@@ +114,5 @@
> +  };
> +  let causedInvalidation = false;
> +  let receivedMatchingInvalidation = false;
> +  let win;
> +  let observeInvalidation = function(subject, topic, data) {

function observeInvalidation(subject, topic, data) {

@@ +127,5 @@
> +    dump("test2: INVALIDATION: success\n");
> +    receivedMatchingInvalidation = true;
> +    deferreds.invalidation.resolve();
> +  }.bind(this);
> +  let observeProgress = function(subject, topic, data) {

function observeProgress(subject, topic, data) {

@@ +149,5 @@
> +  }.bind(this);
> +  win = yield testOnWindow(false);
> +  info("test2: Window opened");
> +  Services.obs.addObserver(observeProgress, TOPIC_PROGRESS, false);
> +  Services.obs.addObserver(observeInvalidation, TOPIC_INVALIDATION, false);

As above, we should register a cleanup function that ensures those are removed when timing out.

@@ +171,5 @@
> +
> +});
> +*/
> +
> +function testOnWindow(aIsPrivate) {

This needs to wait for 'browser-delayed-startup-finished' on the newly created window. Otherwise we'll create all sorts of intermittent failures.

@@ +177,5 @@
> +  let win = OpenBrowserWindow({private: aIsPrivate});
> +  win.addEventListener("load", function onLoad() {
> +    win.removeEventListener("load", onLoad, false);
> +    executeSoon(function() {
> +        deferred.resolve(win);

Nit: indentation is off.

@@ +185,5 @@
> +}
> +
> +function nextSaveState() {
> +  let deferred = Promise.defer();
> +  waitForSaveState(_ => deferred.resolve());

Nit: waitForSaveState(() => deferred.resolve());

::: browser/components/sessionstore/test/head.js
@@ +11,5 @@
>  
> +Cu.import("resource://gre/modules/Task.jsm", tmp);
> +let Task = tmp.Task;
> +
> +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", tmp);

Promise.jsm?

@@ +88,5 @@
>          win.gBrowser.tabContainer.removeEventListener("SSTabRestored", onSSTabRestored, true);
>        });
>        listening = false;
> +      executeSoon(function() {
> +        deferred.resolve();

Hm. What's the point of the deferred here? Did you forget to return it?

@@ +153,5 @@
>    function onSSTabRestored() {
>      aTab.removeEventListener("SSTabRestored", onSSTabRestored, false);
>      listening = false;
> +    executeSoon(function() {
> +      deferred.resolve();

Should we pass aTab to resolve() like we do in the when*Loaded() methods?

@@ +209,5 @@
>    observing = true;
>    Services.obs.addObserver(observer, topic, false);
>  };
>  
> +function whenTabLoaded(aTab, aCallback = null) {

This isn't used anywhere, is it?

@@ +221,5 @@
> +  }, true);
> +  return deferred.promise;
> +}
> +
> +function whenTabClosed(aTab, aCallback = null) {

This isn't used anywhere.

@@ +235,5 @@
> +  }, true);
> +  return deferred.promise;
> +}
> +
> +function whenTabRestored(aTab, aCallback = null) {

This isn't used anywhere, either.

@@ +277,5 @@
>    }, false);
> +  return deferred.promise;
> +}
> +
> +function nextTick() {

This seems unused.

@@ +370,5 @@
>      aCallback(win);
>    }, false);
>  }
>  
> +function waitForTabLoad(aWin, aURL, aCallback) {

I don't see this function used anywhere.

Comment 56

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 751379 [details] [diff] [review]
4. Backing up on first use v2

This patch is replaced by bug 876168.

Comment 57

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 752304 [details] [diff] [review]
3. Making some elements of data collection asynchronous, v9

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

::: browser/app/profile/firefox.js
@@ +780,5 @@
>  // Don't show the about:rights notification in debug builds.
>  pref("browser.rights.override", true);
>  #endif
>  
> +pref("browser.sessionstore.log", false);

We can now use .debug for that.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +133,2 @@
>    aMsg = ("SessionStore: " + aMsg).replace(/\S{80}/g, "$&\n");
>    Services.console.logStringMessage(aMsg);

Let's do:

if (LOG) {
  aMsg = ...
}

@@ +189,5 @@
>    getBrowserState: function ss_getBrowserState() {
>      return SessionStoreInternal.getBrowserState();
>    },
>  
> +  getBrowserStateAsync: function ss_getBrowserStateAsync() {

I think I'd rather not add async APIs for now just out of fear people might rely on those before we decide this is actually stable. I know we use it in the tests (part 5) but I don't think we really need that as we're going to collect state asynchronously often enough while running the test suite.

@@ +207,5 @@
>    getWindowState: function ss_getWindowState(aWindow) {
>      return SessionStoreInternal.getWindowState(aWindow);
>    },
>  
> +  getWindowStateAsync: function ss_getWindowStateAsync(aWindow) {

(see above)

@@ +225,5 @@
>    getTabState: function ss_getTabState(aTab) {
>      return SessionStoreInternal.getTabState(aTab);
>    },
>  
> +  getTabStateAsync: function ss_getTabStateAsync(aTab) {

(see above)

@@ -278,5 @@
>    // whether a setBrowserState call is in progress
>    _browserSetState: false,
>  
> -  // time in milliseconds (Date.now()) when the session was last written to file
> -  _lastSaveTime: 0,

This is still used in SessionStoreInternal, e.g. by saveStateDelayed().

@@ +406,5 @@
>    // previous session is not always restored when
>    // "sessionstore.resume_from_crash" is true.
>    _resume_session_once_on_shutdown: null,
>  
> +  _activeWindowSSiCache: null,

It would be great to have a short description what this does. I have no clue :|

@@ +468,2 @@
>      gSessionStartup.onceInitialized.then(
> +      _ => this.initSession()

() => this.initSession()

@@ +1589,5 @@
>      // restore to the given state
>      this.restoreWindow(window, state, true);
>    },
>  
> +  getWindowState: function ssi_getWindowState(aWindow, aRunner = new _AsyncUtils.SyncRunner("getWindowState")) {

We're actually violation the interface if we also allow AsyncRunners here. The .run() method for AsyncRunners will return a promise so that wouldn't be a string anymore.

@@ +1591,5 @@
>    },
>  
> +  getWindowState: function ssi_getWindowState(aWindow, aRunner = new _AsyncUtils.SyncRunner("getWindowState")) {
> +    return aRunner.run(function task() {
> +      if (!aWindow.__SSi && !aWindow.__SS_dyingCache)

DyingCache has changed in the meantime, this will need an update.

@@ +1608,5 @@
>  
>      this.restoreWindow(aWindow, aState, aOverwrite);
>    },
>  
> +  getTabState: function ssi_getTabState(aTab, aRunner = new _AsyncUtils.SyncRunner("getTabState")) {

Same thing about violating the IDL spec.

@@ +2660,5 @@
> +          if (aUpdateAll || this._dirtyWindows[currentWindow.__SSi] || currentWindow === activeWindow) {
> +            yield aRunner.coyield(this._collectWindowData(currentWindow, aRunner));
> +          }
> +          else { // always update the window features (whose change alone never triggers a save operation)
> +            yield aRunner.coyield(this._updateWindowFeatures(currentWindow, aRunner));

_updateWindowFeatures() does not actually support a runner being passed as the second argument.

@@ +2776,5 @@
> +
> +      var winData = this._windows[aWindow.__SSi];
> +      let windows = {};
> +      windows[aWindow.__SSi] = winData;
> +      yield aRunner.coyield(this._updateCookies(windows, aRunner));

_updateCookies() does not take a runner argument.

@@ +2791,5 @@
> +      // update the internal state data for this window
> +      yield aRunner.coyield(this._saveWindowHistory(aWindow, aRunner));
> +      this._updateTextAndScrollData(aWindow);
> +      this._updateCookieHosts(aWindow);
> +      this._updateWindowFeatures(aWindow);

Should we coyield() for all those steps here, too?

@@ +3152,5 @@
>  
> +      if ("attributes" in tabData) {
> +        for (let name in tabData.attributes)
> +          this.xulAttributes[name] = true;
> +      }

That change should probably be obsolete by now as I remember touching that in some bug related to image attributes.

@@ +3282,5 @@
>        tab.removeAttribute(name);
> +    if ("attributes" in tabData) {
> +      for (let name in tabData.attributes)
> +        tab.setAttribute(name, tabData.attributes[name]);
> +    }

(see above)

@@ +3889,5 @@
> +      debug(ex);
> +      debug(ex.stack);
> +      debug(new Error().stack);
> +      throw ex;
> +    }

That looks like leftover debug code we can get rid of.

@@ +4742,5 @@
> +// corresponding preference is set.
> +const TOPIC_NOTIFY_ASYNC_SAVE_PROGRESS = "sessionstore-async_save-progress";
> +// Notification topic, sent whenever async save data collection has
> +// been invalidated.
> +const TOPIC_NOTIFY_ASYNC_SAVE_INVALIDATION = "sessionstore-async_save-invalidation";

Let's move those consts to the top.

@@ +4744,5 @@
> +// Notification topic, sent whenever async save data collection has
> +// been invalidated.
> +const TOPIC_NOTIFY_ASYNC_SAVE_INVALIDATION = "sessionstore-async_save-invalidation";
> +
> +let AsyncSaver = {

Apart from a couple prefs, the loadState, lastSessionState and lastSaveTime there is not much that the AsyncSaver needs to work other than _getCurrentState(). We don't need to do that now but I think this really needs to be in its own module, if only for readability.

@@ +4749,5 @@
> +   /**
> +   * Initialize the async saver
> +   */
> +  init: function() {
> +    let initPref = function(prefKey, getter, property) {

function initPref(...) {

@@ +4765,5 @@
> +      ["browser.sessionstore.async.notify-progress", "getBoolPref", "_notifySaveProgress"],
> +      ["browser.sessionstore.async.max-attempts", "getIntPref", "_maxAsyncCollectionAttempts"],
> +      ["browser.sessionstore.async.wait-between-attempts", "getIntPref", "_asyncSaveWaitBetweenAttemptsMS"],
> +      ["browser.sessionstore.async.chunk-duration", "getIntPref", "_asyncSaveChunkDurationMS"],
> +      ["browser.sessionstore.async.reschedule-after", "getIntPref", "_asyncSaveRescheduleAfterMS"],

They all share the 'browser.sessionstore.async.' prefix so maybe we can put that into a const or variable.

@@ +4782,5 @@
> +   * - bool forceSync (default: false)
> +   */
> +  saveState: function(options) {
> +    let updateAll = options ? !!options.updateAll : false;
> +    let forceSync = options ? !!options.forceSync : false;

let updateAll = options && options.updateAll;
let forceSync = options && ptions.forceSync;

It's way more common to do that. We don't really care if those are booleans anyway.

@@ +4799,5 @@
> +        debug("Current attempt: " + (updateAll?"update all":"do not update all"));
> +        return Task.spawn(function task() {
> +          try {
> +            debug("(queue) Waiting until the previous attempt is complete");
> +            yield progress.promise;

Hmm. I wonder if we shouldn't just invalidate the current state? Why wait for the previous saveState() operation to complete when we need to handle invalidation anyway?

@@ +4815,5 @@
> +    }
> +
> +    // If crash recovery is disabled, we only want to resume with
> +    // pinned tabs if we crash.
> +    let pinnedOnly = SessionStoreInternal._loadState == STATE_RUNNING && !SessionStoreInternal._resume_from_crash;

Nit: please wrap that line.

@@ +4909,5 @@
> +  /**
> +   * Determine whether observers should be notified of any progress of async
> +   * collection/saving. Used for testing purposes.
> +   */
> +  _notifySaveProgress: false,

Can we move all those flags/fields to the top of the object, please? I really don't expect to find them in the middle.

@@ +4971,5 @@
> +    * write a state object to disk
> +    */
> +  _saveStateObject: function(aStateObj) {
> +    debug("_saveStateObject()");
> +    debug(JSON.stringify(aStateObj));

That's probably quite expensive if we always stringify the state twice.

@@ +4995,5 @@
> +      // Note that we do not have race conditions here as _SessionFile
> +      // guarantees that any I/O operation is completed before proceeding to
> +      // the next I/O operation.
> +      // Note backup happens only once, on initial save.
> +      promise = SessionStoreInternal._backupSessionFileOnce;

As mentioned on IRC, we can probably move that to the worker.

@@ +5010,5 @@
> +
> +    // Once the session file is successfully updated, save the time stamp of the
> +    // last save and notify the observers.
> +    promise = promise.then(() => {
> +      this._lastSaveTime = Date.now();

We need to update SessionStoreInternal._lastSaveTime as well, don't we?

@@ +5034,5 @@
> +  AsyncInvalidationWrapper: function(name) {
> +    // Sanity check
> +    if (this == AsyncSaver) {
> +      throw new Error("AsyncInvalidationWrapper is meant as a constructor");
> +    }

Hmm. This seems a little unnecessary. I know this is supposed to be namespaced but I think we can just make this be its own object and be used by the AsyncSaver. It's not exported from the JSM so I don't think we need the extra namespacing here.

@@ +5056,5 @@
> +    let runner = this.controller.runner;
> +    let controller = this.controller;
> +    return runner.run(function task() {
> +      let listener = function(event) {
> +        controller.throw(new Invalidation(event, this._id));

new Invalidation(this._id, event)

@@ +5062,5 @@
> +      AsyncInvalidation.addListener(listener);
> +      try {
> +        for (let attempt = 0;
> +             attempt < AsyncSaver._maxAsyncCollectionAttempts;
> +             ++ attempt) {

Can we maybe use a variable to keep that on one line?

@@ +5075,5 @@
> +              Services.obs.notifyObservers(null, TOPIC_NOTIFY_ASYNC_SAVE_INVALIDATION, this.name);
> +            }
> +            for (let i = 0;
> +                 i < AsyncSaver._maxAsyncCollectionAttempts;
> +                 ++ i) {

We could reuse that maxAttempts (or the like) variable here.

@@ +5083,5 @@
> +              } catch (ex if ex instanceof Invalidation && ex.event != "quitting") {
> +                // While we're waiting, ignore invalidations
> +              }
> +            }
> +            continue;

Do we need that? We'll continue here anyway, no?

@@ +5094,5 @@
> +      this.fallback = true;
> +      throw new Invalidation(this._id);
> +    }.bind(this));
> +  },
> +  runWithFallback: function(aRunnable, aFallback) {

We could remove that for now without the new async APIs.

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Utilities, v7

Unbitrotten, applied feedback, propagated some changes to Components.utils.methodjit.

Comment 59

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #57)
> > +  getBrowserStateAsync: function ss_getBrowserStateAsync() {
> 
> I think I'd rather not add async APIs for now just out of fear people might
> rely on those before we decide this is actually stable. I know we use it in
> the tests (part 5) but I don't think we really need that as we're going to
> collect state asynchronously often enough while running the test suite.

As discussed over IRC, I will hide these async APIs in SessionStoreInternal, protected by the preference that allows backstage access.

> @@ -278,5 @@
> >    // whether a setBrowserState call is in progress
> >    _browserSetState: false,
> >  
> > -  // time in milliseconds (Date.now()) when the session was last written to file
> > -  _lastSaveTime: 0,
> 
> This is still used in SessionStoreInternal, e.g. by saveStateDelayed().

Scary that this didn't prevent tests from passing...

> 
> @@ +406,5 @@
> >    // previous session is not always restored when
> >    // "sessionstore.resume_from_crash" is true.
> >    _resume_session_once_on_shutdown: null,
> >  
> > +  _activeWindowSSiCache: null,
> 
> It would be great to have a short description what this does. I have no clue
> :|

Yeah, neither do I. I just wanted to avoid using |undefined| fields.
But at a glance, I suspect that it's used for two unrelated things...


> > +  getWindowState: function ssi_getWindowState(aWindow, aRunner = new _AsyncUtils.SyncRunner("getWindowState")) {
> 
> We're actually violation the interface if we also allow AsyncRunners here.
> The .run() method for AsyncRunners will return a promise so that wouldn't be
> a string anymore.

Well, yes, but both the IDL and |SessionStore.getWindowState| only expose one argument, so I believe this can't happen.

> @@ +2791,5 @@
> > +      // update the internal state data for this window
> > +      yield aRunner.coyield(this._saveWindowHistory(aWindow, aRunner));
> > +      this._updateTextAndScrollData(aWindow);
> > +      this._updateCookieHosts(aWindow);
> > +      this._updateWindowFeatures(aWindow);
> 
> Should we coyield() for all those steps here, too?

Sure, why not.

> @@ +4744,5 @@
> > +// Notification topic, sent whenever async save data collection has
> > +// been invalidated.
> > +const TOPIC_NOTIFY_ASYNC_SAVE_INVALIDATION = "sessionstore-async_save-invalidation";
> > +
> > +let AsyncSaver = {
> 
> Apart from a couple prefs, the loadState, lastSessionState and lastSaveTime
> there is not much that the AsyncSaver needs to work other than
> _getCurrentState(). We don't need to do that now but I think this really
> needs to be in its own module, if only for readability.

Yes, that was my objective, too. I didn't want to take all the measures necessary to move AsyncSaver to its own .jsm yet, but I believe that sounds like a good followup bug.

> @@ +4799,5 @@
> > +        debug("Current attempt: " + (updateAll?"update all":"do not update all"));
> > +        return Task.spawn(function task() {
> > +          try {
> > +            debug("(queue) Waiting until the previous attempt is complete");
> > +            yield progress.promise;
> 
> Hmm. I wonder if we shouldn't just invalidate the current state? Why wait
> for the previous saveState() operation to complete when we need to handle
> invalidation anyway?

Why would we need to handle invalidation here?

> @@ +4971,5 @@
> > +    * write a state object to disk
> > +    */
> > +  _saveStateObject: function(aStateObj) {
> > +    debug("_saveStateObject()");
> > +    debug(JSON.stringify(aStateObj));
> 
> That's probably quite expensive if we always stringify the state twice.

Good catch.

> @@ +4995,5 @@
> > +      // Note that we do not have race conditions here as _SessionFile
> > +      // guarantees that any I/O operation is completed before proceeding to
> > +      // the next I/O operation.
> > +      // Note backup happens only once, on initial save.
> > +      promise = SessionStoreInternal._backupSessionFileOnce;
> 
> As mentioned on IRC, we can probably move that to the worker.

Do you want me to do something in this patch, or is that for a followup bug?

> 
> @@ +5010,5 @@
> > +
> > +    // Once the session file is successfully updated, save the time stamp of the
> > +    // last save and notify the observers.
> > +    promise = promise.then(() => {
> > +      this._lastSaveTime = Date.now();
> 
> We need to update SessionStoreInternal._lastSaveTime as well, don't we?

I merged them back.

> @@ +5034,5 @@
> > +  AsyncInvalidationWrapper: function(name) {
> > +    // Sanity check
> > +    if (this == AsyncSaver) {
> > +      throw new Error("AsyncInvalidationWrapper is meant as a constructor");
> > +    }
> 
> Hmm. This seems a little unnecessary. I know this is supposed to be
> namespaced but I think we can just make this be its own object and be used
> by the AsyncSaver. It's not exported from the JSM so I don't think we need
> the extra namespacing here.

ok

> @@ +5094,5 @@
> > +      this.fallback = true;
> > +      throw new Invalidation(this._id);
> > +    }.bind(this));
> > +  },
> > +  runWithFallback: function(aRunnable, aFallback) {
> 
> We could remove that for now without the new async APIs.

Keeping since the async APIs are still somewhere.

Comment 60

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Making some elements of data collection asynchronous, v10

Main patch, plus feedback and unbitrot.
Surprisingly, it seems to pass tests:
https://tbpl.mozilla.org/?tree=Try&rev=5ca07f5f8b02

Comment 61

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #42)
> Comment on attachment 746456 [details] [diff] [review]
> 2. Invalidating the session when necessary, v2
> 
> Review of attachment 746456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with all issues addressed.
> 
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +691,1 @@
> >      }
> 
> I don't think we need this ugly try-catch stmt here. Exceptions should be
> reported anyway if uncaught.

Unfortunately, they are not, so I believe we need to report them somewhere.
Would you prefer if I turn this into a Cu.reportError?

Comment 62

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 746456 [details] [diff] [review]
2. Invalidating the session when necessary, v2

This patch makes a call to AsyncInvalidation._invalidate(), which doesn't seem to be defined anywhere. What's that for?

Comment 63

[David Teller [:Yoric] - still alive but not very active]

Bill: As noticed by ttaubert in comment 46, this should have been |fire|.

Comment 64

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Invalidating the session when necessary, v3

Comment 65

[David Teller [:Yoric] - still alive but not very active]

As discussed over Vidyo, we need to consider an alternative strategy, in which we rewrite the session restore à la e10s, with content scripts and an asynchronous driver. To avoid making code tooo messy, we will probably want to completely fork the code.

Comment 66

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 775341 [details] [diff] [review]
3. Making some elements of data collection asynchronous, v10

Cancelling review. We're not going to land this as is but will steal parts and ideas instead.

Comment 67

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

As I understand it, this is WONTFIX in favor of bug 894595.