1024382 - [Session Restore] Move closed windows information to a Data module

Status: NEW

(Firefox :: Session Restore, defect)

Description

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

As a first step towards bug 960572, let's start with a simple module holding only the information on closed windows.

Once this is done, we will be able to:
1. progressively complete the module to handle all the toplevel values (open windows, global attributes);
2. let the module handle invalidation of data;
3. let the module communicate diffs directly to the SessionWorker.

Comment 1

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

Attachment: Data.jsm

So, let's start moving things.
Nothing extraordinary here, just the first baby step of the refactoring.

Comment 2

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

Attachment: Data.jsm, v2

Better if I send it after hg qref than before.

Comment 3

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

Attachment: Data.jsm, v3

I decided to move not just the information on closed windows, but all windows-related information to Data.jsm. This API should be sufficient for ensuring that we do not need to transmit the entire sessionstore data across threads whenever we close a window. I plan to execute this in a followup bug.

Comment 4

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

Comment on attachment 8440169 [details] [diff] [review]
Data.jsm, v3

I have started applying this to actually sending diffs and it looks like the API needs a little reworking.

Comment 5

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

Attachment: Data.jsm, v4

This is an almost full rewrite of the previous patches.

The main contribution of the this patch is to introduce a module `resource:///modules/sessionstore/Data.jsm`, designed to hold the in-memory representation of sessionstore.js and at a later stage to transparently send updates to the SessionWorker. (I have a Proof of Concept on my machine that gets it to work.)

The long-term objective is to make Data.jsm the public API for interacting with Session Restore data. By opposition to SessionStore.jsm, getters do not return strings but read-only objects, and setters themselves are much more structured.

For the moment, Data holds only the data formerly contained in `SessionStore._closedWindows`.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8442339 [details] [diff] [review]
Data.jsm, v4

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

Tests are unfortunately breaking all over the place. Please make sure to run tests before submitting a new patch, thanks!

I'll submit some more general comments in a second comment.

::: browser/components/sessionstore/src/Data.jsm
@@ +41,5 @@
> +   * properties.
> +   */
> +  this._properties = new WeakMap();
> +}
> +UpdatableArray.prototype = {

Do we need that? Can't we just use a normal array and call .remove() from Utils.jsm?

@@ +255,5 @@
> + *
> + * @return A read-only value with the same structure as `obj`, with
> + * the addition of methods `{get, set, remove, has}Property`.
> + */
> +function readonly(obj, weakmap) {

What are we guarding against here? I'm not sure I understand why this needs be read-only.

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +195,5 @@
>      }
>  
>      stopWatchStart("COLLECT_DATA_MS", "COLLECT_DATA_LONGEST_OP_MS");
>      let state = SessionStore.getCurrentState(forceUpdateAllWindows);
> + 

Nit: space

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +131,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "Utils",
>    "resource:///modules/sessionstore/Utils.jsm");
>  
> +
> +Array.prototype.remove = function(condition) {

Let's not modify basic prototypes, this should be in Utils.jsm.

@@ +138,5 @@
> +      this.splice(i, 1);
> +    }
> +  }
> +};
> +Array.prototype.update = function(i, cb) {

Do we really need that simple function?

@@ +1029,5 @@
>          // random closed or even a pop-up window re-opened. To prevent that
>          // we explicitly allow saving an "empty" window state.
>          let isLastWindow =
>            Object.keys(this._windows).length == 1 &&
> +          Data.closedWindows.some(win => win.getProperty("shouldRestore"));

This reverses the condition.

@@ +1034,4 @@
>  
>          if (hasSaveableTabs || isLastWindow) {
>            // we don't want to save the busy state
>            delete winData.busy;

It would be great if we would just say |SessionData.closeWindow(aWindow.__SSi)| and the whole |hasSaveableTabs| and |isLastWindow| checking would happen internally.

@@ +1040,5 @@
> +
> +#ifndef XP_MACOSX
> +          // Until we decide otherwise elsewhere, this window is part of a series
> +          // of closing windows to quit.
> +          Data.closedWindows.get(0).setProperty("shouldRestore");

This is also something |SessionStore| doesn't need to know about and should be covered in SessionData.

@@ +1047,1 @@
>            this._capClosedWindows();

This whole thing should really just be done automatically when calling SessionData.closeWindow().

@@ +1197,5 @@
>      // remove all open & closed tabs containing a reference to the given
>      // domain in closed windows
> +    for (let ix = Data.closedWindows.length - 1; ix >= 0; ix--) {
> +
> +      // First check whether we need to update something, using

This should be in SessionData and internal only.

@@ +1443,5 @@
>    // Do not reschedule a save. This will wait for the next regular
>    // save.
>    onIdleDaily: function() {
>      // Remove old closed windows
> +    this._cleanupOldData([Data.closedWindows]);

SessionData.cleanupClosedWindows()?

@@ +1712,4 @@
>    },
>  
>    undoCloseWindow: function ssi_undoCloseWindow(aIndex) {
> +    let winData = Data.closedWindows.extract(aIndex);

let winData = SessionData.undoCloseWindow(aIndex); ?

@@ +3365,5 @@
>     * windows.
>     */
>    _clearRestoringWindows: function ssi_clearRestoringWindows() {
> +    for (let i = 0; i < Data.closedWindows.length; i++) {
> +      Data.closedWindows.get(i).removeProperty("shouldRestore");

SessionData.closePendingWindows(); ?

::: browser/components/sessionstore/src/moz.build
@@ +13,5 @@
>  JS_MODULES_PATH = 'modules/sessionstore'
>  
>  EXTRA_JS_MODULES = [
>      'ContentRestore.jsm',
> +    'Data.jsm',

Let's call it SessionData.jsm, Data.jsm is a little too generic for my taste.

::: browser/components/sessionstore/test/browser_cleaner.js
@@ +126,5 @@
>  
>    state = JSON.parse(ss.getBrowserState());
>    is(state.windows[0].closedAt || false, false, "4. Main window doesn't have closedAt");
> +
> +  info("Closed windows: " + JSON.stringify(state._closedWindows, null, "\t"));

Do we really need more output here? I'd rather not do that.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

What bothers me with the current patch is that there is all this prototype and readonly magic that seems unnecessary. The API for Data.closedWindows probably shouldn't be used that directly either. I expected a somewhat higher-level API:

--

> SessionData.importClosedWindows(windows);
Called when restoring an old session. Overrides all stored closed windows.

> SessionData.closeWindow(window);
Called when a window is closed. We should keep attaching the |_shouldRestore| property like we currently do to keep changes small. Maintains the max cap of closed windows.

> SessionData.undoCloseWindow(index);
Called when restoring a closed window. Removes the window state at the given index and returns it.

> SessionData.closePendingWindows();
Called when a user action causes closed windows with shouldRestore=true to be finally marked as closed. Remove |_shouldRestore| properties from all closed windows.

> SessionData.clearClosedWindows()
Empty the list of closed windows. Called when receiving "browser:purge-session-history".

> SessionData.clearClosedWindowTabsForDomain(domain)
Remove tabs from a given domain from the list of closed windows. Called when receiving "browser:purge-domain-data". The logic to remove those tabs should just move here.

> SessionData.cleanupClosedWindows(ttl)
Purge the list of closed windows based on the set TTL. Called when receiving "idle-daily". The logic should move here as well.

> SessionState.export(state)
Attaches a |_closedWindows| property to the given |state|.

--

We would now do the following: The main thread instance would be used by |SessionStore|. In |getCurrentState()| we can simply call |SessionState.export()| and stringify the state and return it.

The worker instance would be called by messages sent to the worker whenever the main thread instance is being called. The worker would additionally take care of "restoring" closed windows with shouldRestore=true after calling |SessionState.export()| in addition to what has been sent to |SessionFile.write()|.

I don't think we need to go nuts and rewrite everything, we could just handle the state in an array of (closed) window objects like we currently do. We want to stringify it either way.

So... how does that sound? Anything I'm missing?

Comment 8

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

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Comment on attachment 8442339 [details] [diff] [review]
> Data.jsm, v4
> 
> Review of attachment 8442339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tests are unfortunately breaking all over the place. Please make sure to run
> tests before submitting a new patch, thanks!

Sorry about that.

> Do we need that? Can't we just use a normal array and call .remove() from
> Utils.jsm?

I managed to get rid of it.
> 
> @@ +255,5 @@
> > + *
> > + * @return A read-only value with the same structure as `obj`, with
> > + * the addition of methods `{get, set, remove, has}Property`.
> > + */
> > +function readonly(obj, weakmap) {
> 
> What are we guarding against here? I'm not sure I understand why this needs
> be read-only.

Because this is the first part of a refactoring that will move the data off the main thread on the fly. If data can be changed without Data being aware of it, we have data corruption. After spending too much time debugging my first version that didn't make data readonly, I decided to be stricter :)

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +131,5 @@
> >  XPCOMUtils.defineLazyModuleGetter(this, "Utils",
> >    "resource:///modules/sessionstore/Utils.jsm");
> >  
> > +
> > +Array.prototype.remove = function(condition) {
> 
> Let's not modify basic prototypes, this should be in Utils.jsm.
> 
> @@ +138,5 @@
> > +      this.splice(i, 1);
> > +    }
> > +  }
> > +};
> > +Array.prototype.update = function(i, cb) {
> 
> Do we really need that simple function?

The sole role of both methods is to keep _cleanOldData simple. You wanted me to factor that code, so I took pains to keep it factored :)

> @@ +1034,4 @@
> >  
> >          if (hasSaveableTabs || isLastWindow) {
> >            // we don't want to save the busy state
> >            delete winData.busy;
> 
> It would be great if we would just say
> |SessionData.closeWindow(aWindow.__SSi)| and the whole |hasSaveableTabs| and
> |isLastWindow| checking would happen internally.

I'd rather keep this for a followup, if you don't mind. Despite appearances, I attempt strongly to keep the scope of this bug as small as possible :)

> @@ +1040,5 @@
> > +
> > +#ifndef XP_MACOSX
> > +          // Until we decide otherwise elsewhere, this window is part of a series
> > +          // of closing windows to quit.
> > +          Data.closedWindows.get(0).setProperty("shouldRestore");
> 
> This is also something |SessionStore| doesn't need to know about and should
> be covered in SessionData.

As above.

> @@ +1047,1 @@
> >            this._capClosedWindows();
> 
> This whole thing should really just be done automatically when calling
> SessionData.closeWindow().

Yeah, once we have `closeWindow` in the SessionData. I backtracked from a previous version that had it due to wild scope creep.

> This should be in SessionData and internal only.

Fair enough for moving it to SessionData. What do you mean "internal"?

> @@ +1443,5 @@
> >    // Do not reschedule a save. This will wait for the next regular
> >    // save.
> >    onIdleDaily: function() {
> >      // Remove old closed windows
> > +    this._cleanupOldData([Data.closedWindows]);
> 
> SessionData.cleanupClosedWindows()?

As a rule of thumb, at least for a first version, I would like to avoid making SessionData introduce any data by itself. It feels like a footgun, especially for an API that I would like to turn into the official API for add-ons that want to interact with the Session.

> 
> @@ +1712,4 @@
> >    },
> >  
> >    undoCloseWindow: function ssi_undoCloseWindow(aIndex) {
> > +    let winData = Data.closedWindows.extract(aIndex);
> 
> let winData = SessionData.undoCloseWindow(aIndex); ?

I believe that calling this `undoCloseWindow` would be misleading for users. We are only manipulating data, not [un]closing windows.

> @@ +3365,5 @@
> >     * windows.
> >     */
> >    _clearRestoringWindows: function ssi_clearRestoringWindows() {
> > +    for (let i = 0; i < Data.closedWindows.length; i++) {
> > +      Data.closedWindows.get(i).removeProperty("shouldRestore");
> 
> SessionData.closePendingWindows(); ?

Same here.

> ::: browser/components/sessionstore/src/moz.build
> @@ +13,5 @@
> >  JS_MODULES_PATH = 'modules/sessionstore'
> >  
> >  EXTRA_JS_MODULES = [
> >      'ContentRestore.jsm',
> > +    'Data.jsm',
> 
> Let's call it SessionData.jsm, Data.jsm is a little too generic for my taste.

What about just Session.jsm?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> > SessionData.cleanupClosedWindows()?
> 
> As a rule of thumb, at least for a first version, I would like to avoid
> making SessionData introduce any data by itself. It feels like a footgun,
> especially for an API that I would like to turn into the official API for
> add-ons that want to interact with the Session.

We shouldn't really design an official API for add-ons here. I don't think it makes sense to try and address two different issues at the same time. We are working towards differential updates and a journaled storage, not changing anything for add-ons.

> > Let's call it SessionData.jsm, Data.jsm is a little too generic for my taste.
> 
> What about just Session.jsm?

SessionState.jsm?

Comment 10

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

(In reply to Tim Taubert [:ttaubert] from comment #7)
> What bothers me with the current patch is that there is all this prototype
> and readonly magic that seems unnecessary. The API for Data.closedWindows
> probably shouldn't be used that directly either. I expected a somewhat
> higher-level API:

I believe that before we can think of a higher-level API, we need to make sure that 1/ we have a rock-solid low-level API; 2/ our tools are not going to footgun us at every step – getting a data structure correctly synchronized between threads is hard, and we have all sorts of "interesting" side-effects in SessionStore.jsm. 

Now, I removed the prototype and simplified `readonly`. Once we have finished the refactoring, we may wish to discuss the fate of `readonly`, as it will become less useful once we are sure that we are not accidentally changing data without breaking the invariant that both threads have matching models/patches.

> --
> 
> We would now do the following: The main thread instance would be used by
> |SessionStore|. In |getCurrentState()| we can simply call
> |SessionState.export()| and stringify the state and return it.

Given how `getCurrentState()` currently works, I'm not convinced just yet that anything can "simply" work :)

> The worker instance would be called by messages sent to the worker whenever
> the main thread instance is being called. The worker would additionally take
> care of "restoring" closed windows with shouldRestore=true after calling
> |SessionState.export()| in addition to what has been sent to
> |SessionFile.write()|.
> 
> I don't think we need to go nuts and rewrite everything, we could just
> handle the state in an array of (closed) window objects like we currently
> do. We want to stringify it either way.

If I understand correctly your approach, we would need to eventually have a complete implementation of many modules on each thread. Even if we somehow manage to keep both implementations in a single source file, this still means rewriting pretty much everything

So far, I'm not a big fan of the idea.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> I believe that before we can think of a higher-level API, we need to make
> sure that 1/ we have a rock-solid low-level API; 2/ our tools are not going
> to footgun us at every step – getting a data structure correctly
> synchronized between threads is hard, and we have all sorts of "interesting"
> side-effects in SessionStore.jsm. 

Synchronization between threads in this case is actually not hard as the worker just receives stuff and doesn't do anything else.

> If I understand correctly your approach, we would need to eventually have a
> complete implementation of many modules on each thread. Even if we somehow
> manage to keep both implementations in a single source file, this still
> means rewriting pretty much everything

I do explicitly not want to rewrite everything. That's why I am suggesting to keep the current mechanisms like |shouldRestore| etc. As discussed in the meeting we need two SessionData/State instances with the current nsISessionStore API, that's just unavoidable. Those should of course be the same modules and code, just in different threads.

Starting with the closed windows first is good, it doesn't touch everything at once and lets us start with a subset of the data.

> So far, I'm not a big fan of the idea.

Can you elaborate and point to concrete problems of the approach I suggested in comment #7?

Comment 12

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

(In reply to Tim Taubert [:ttaubert] from comment #11)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> > I believe that before we can think of a higher-level API, we need to make
> > sure that 1/ we have a rock-solid low-level API; 2/ our tools are not going
> > to footgun us at every step – getting a data structure correctly
> > synchronized between threads is hard, and we have all sorts of "interesting"
> > side-effects in SessionStore.jsm. 
> 
> Synchronization between threads in this case is actually not hard as the
> worker just receives stuff and doesn't do anything else.

Synchronization between threads is harder than it looks, as we have a nasty habit of doing side-effects on data after it has been collected. For instance, currently PrivacyFilter.filterPrivateWindowsAndTabs has side-effects on our in-memory representation of windows, and I'm almost certain that it can corrupt data – I found this out when I realized I had no way of moving it to the worker thread without changing it semantics, by the way.

> > If I understand correctly your approach, we would need to eventually have a
> > complete implementation of many modules on each thread. Even if we somehow
> > manage to keep both implementations in a single source file, this still
> > means rewriting pretty much everything
> 
> I do explicitly not want to rewrite everything. That's why I am suggesting
> to keep the current mechanisms like |shouldRestore| etc.

As a side-note, I classify the current mechanism for `shouldRestore` as a footgun. We annotate the data structure that we intend to write to the disk with stuff that we do not intend to write to the disk, then de-annotate the data prior to writing it to the disk. If some future patch (or an add-on) ever causes us to write from another path (e.g. getBrowserState), we might end up writing `shouldRestore` to the disk accidentally, then restoring it on the next run, with all sorts of interesting consequences.

> As discussed in the
> meeting we need two SessionData/State instances with the current
> nsISessionStore API, that's just unavoidable. Those should of course be the
> same modules and code, just in different threads.
>
> Starting with the closed windows first is good, it doesn't touch everything
> at once and lets us start with a subset of the data.
> > So far, I'm not a big fan of the idea.
> 
> Can you elaborate and point to concrete problems of the approach I suggested
> in comment #7?

I realize that we are working based on distinct assumptions. Both approaches will eventually lead to a result, although I unsurprisingly prefer mine.

** Die Methode TTaubert **

If I understand correctly, you intend to have the code executed twice, once on the main thread and once on the worker. You intend to convert SessionStore.jsm into an event dispatcher that turns each low-level event into two high-level method calls, once on the main thread and once on the worker. Is that correct? For this purpose, you basically need to move the code from SessionStore.jsm to another module SessionDataHighLevel.jsm, which will itself be opened on both threads. Eventually, this module will be in charge of doing nearly everything that SessionStore.jsm does at the moment except event-handling. Progressively, you need to port all modules that can affect the data to be themselves launched on both threads.

Pros: (I'll let you fill this in)
- Most messages are short;
- code is the same on both workers, which should make it easier to spot errors in the code;

Cons:
- eventually, almost every module needs to be rewritten to work on both threads;
- doesn't get us much closer to journaling;
- the communication protocol has many moving parts, which makes it harder to spot errors in the protocol.

** La méthode Yoric **

On my side, I intend to have the code executed only once, on the main thread, with dumb diffs being sent to the worker thread. For this purpose, I move the data (not the code) from SessionStore.jsm to another module SessionDataLowLevel.jsm, which only provides a dumb API to access and modify the data. Behind the scenes, this module will assemble dumb diffs and send them to the worker. Progressively, all the modules that perform side-effects to the browser state data structures will need to be ported to access data only through the API.

Pros:
- Requires few changes to the code of each module, most of them trivial;
- the communication protocol is trivial (for the moment, we only need three dif primitives `replace`, `insert` and `remove) and easy to audit;
- this is exactly what we need for journaling;
- `readonly` helps clean up footguns;
- as a side-effect, a clean data API might be a much better API than what we have now, both saner (e.g. `"__SSi" in window.__SSi` => `openedWindows.has(window) || dyingWindows.has(window)` ) and faster (e.g. `JSON.parse(getWindowState(window))` becomes `openedWindows.get(window)`). This should make it easier to refactor and review future patches. Oh, yeah, and add-ons, too :)

Cons:
- `readonly` policy forces us to rewrite many drive-by side-effects;
- messages are typically larger and may require future optimization work (e.g. coalescing);
- code is not shared, which makes it harder to spot errors in the code.


====

Now, I can't get much more specific than that without first writing the code for your method.

Now, let me suggest the following approach:
- start with my general approach and get it to work for closed windows, then reevaluate;
- I can see a few cases in which your approach will be more suited (in particular, SessionCookies), so for these cases, use your approach.

Is this ok with you?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> As a side-note, I classify the current mechanism for `shouldRestore` as a
> footgun. We annotate the data structure that we intend to write to the disk
> with stuff that we do not intend to write to the disk, then de-annotate the
> data prior to writing it to the disk. If some future patch (or an add-on)
> ever causes us to write from another path (e.g. getBrowserState), we might
> end up writing `shouldRestore` to the disk accidentally, then restoring it
> on the next run, with all sorts of interesting consequences.

I agree, this is a terrible hack. I was merely interested in not changing as much code but I'm equally happy to see that thing go!

> Cons:
> - eventually, almost every module needs to be rewritten to work on both
> threads;
> - doesn't get us much closer to journaling;

Yeah, the worker code would have to have some more code that takes care of constructing these diffs that could be sent to the storage engine. Not super elegant, maybe.

> - the communication protocol has many moving parts, which makes it harder to
> spot errors in the protocol.

Good point.

> On my side, I intend to have the code executed only once, on the main
> thread, with dumb diffs being sent to the worker thread. For this purpose, I
> move the data (not the code) from SessionStore.jsm to another module
> SessionDataLowLevel.jsm, which only provides a dumb API to access and modify
> the data. Behind the scenes, this module will assemble dumb diffs and send
> them to the worker. Progressively, all the modules that perform side-effects
> to the browser state data structures will need to be ported to access data
> only through the API.

You're right, that does sound better. Thanks for talking it through.

> Cons:
> - `readonly` policy forces us to rewrite many drive-by side-effects;

May also be a pro, not taking the amount of work into account :)

> Now, let me suggest the following approach:
> - start with my general approach and get it to work for closed windows, then
> reevaluate;

Yup.

> - I can see a few cases in which your approach will be more suited (in
> particular, SessionCookies), so for these cases, use your approach.

I didn't give it that much thought yet but I agree, let's talk about this when we get there.

Comment 14

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

Attachment: SessionState, v5

Summary of the changes:
- Data becomes SessionState;
- simplified the implementation of `readonly`, removed `{has, set, get, delete}Property` and replaced it with the less convoluted SessionState.annotations;
- replaced _getAllClosedTabs, which didn't fit in the update model with getClosedTabs and getOpenTabs, which do;
- reimplemented onPurgeDomainData on top of these new APIs.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8447114 [details] [diff] [review]
SessionState, v5

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

I still haven't completely wrapped my head around how this is supposed to work. SessionState.closedWindows almost duplicates the Array.prototype API so that we can send diffs accordingly. What would these diffs look like, do you have an idea about that yet?

Another thing confusing me is the annotations and private windows/tabs cleanup. Only the session workers wants and needs to know about the possibility of closed windows being secretly "restored" before writing to disk. The only exception is that the main thread needs to notify the worker about user actions so that annotations can be cleared. Private tabs and windows however should never make it to the worker in an ideal world.

Would it be beneficial to try and make closed windows work by sending diffs to the worker in a second patch? As a proof-of-concept?

How would we in the future implement sending a diff that moves a window from the list of open windows to the list of closed windows and vice versa? Would we try to optimize that or would send two diffs?

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +197,5 @@
>      }
>  
>      stopWatchStart("COLLECT_DATA_MS", "COLLECT_DATA_LONGEST_OP_MS");
>      let state = SessionStore.getCurrentState(forceUpdateAllWindows);
> + 

Nit: white space/newline

@@ +206,5 @@
>        state.windows = state.deferredInitialState.windows || [];
>        delete state.deferredInitialState;
>      }
>  
> +

Nit: unnecessary newline

::: browser/components/sessionstore/src/SessionState.jsm
@@ +36,5 @@
> +  /**
> +   * A mechanism for storing in-memory annotations on values.
> +   */
> +  get annotations() {
> +    return AnnotationsMap;

Great idea to use WeakMaps to get rid of that property but this doesn't really help us going forward, does it? "Restoring" windows is something that we would only do on disk writes so that would be done in the SessionWorker. The diff we would send to the session worker would basically insert another closed window and at the same time maintain the "shouldRestore" annotations. A special message would probably need to be sent to the worker to make it clear "shouldRestore" annotations on user action.

@@ +43,5 @@
> +
> +/**
> + * A map from (data, key) to values.
> + */
> +let AnnotationsMap = {

Might want to freeze that.

@@ +45,5 @@
> + * A map from (data, key) to values.
> + */
> +let AnnotationsMap = {
> +  _map: new WeakMap(),
> +  has: function(data, key) {

I don't see use using annotations.has(), please remove.

@@ +61,5 @@
> +    let store = this._map.get(data);
> +    if (!store) {
> +      return null;
> +    }
> +    return store[key] || undefined;

We should fallback to |null| too as we would with |!store|.

@@ +68,5 @@
> +    // Normalize `data` to a non-readonly representation
> +    data = readonly.toRaw.get(data) || data;
> +    let store = this._map.get(data);
> +    if (!store) {
> +      store = {};

We should probably use a Map here.

@@ +79,5 @@
> +    data = readonly.toRaw.get(data) || data;
> +    let store = this._map.get(data);
> +    if (!store) {
> +      return false;
> +    }

if (store && key in store) {
  delete store[key];
}

The return value isn't used anywhere.

@@ +180,5 @@
> +    return this._data.unshift(...values);
> +  },
> +
> +  /**
> +   * Add values at the ekend of the list

Nit: end of the list

@@ +182,5 @@
> +
> +  /**
> +   * Add values at the ekend of the list
> +   *
> +   * WARNING: Do not modify `values` after this call.

Why not just clone the values using cloneInto()?

@@ +356,5 @@
> +    return result;
> +  }
> +
> +  // Otherwise, create a new one and store weak references
> +  result = new Proxy(obj, ReadOnlyInterceptor);

Sweet use case for Proxies.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +133,5 @@
>  
> +// Transition API, for compatibility with the data structures
> +// of SessionSaver.
> +
> +Array.prototype.remove = function(condition) {

Please don't modify basic prototypes. We can put this into Utils.jsm or a helper function.

@@ +1229,5 @@
> +          } else {
> +            // some duplication from restoreHistory - make sure we get the correct title
> +            let activeIndex = (selectedTab.index || selectedTab.entries.length) - 1;
> +            if (activeIndex >= selectedTab.entries.length) {
> +              activeIndex = selectedTab.entries.length - 1; 

Nit: trailing space

@@ +1238,4 @@
>        }
> +      return true;
> +    });
> +    closedWindows.getClosedTabs().remove((tabData, k) => 

Nit: trailing space and |k| is unused.

@@ +2040,5 @@
>      this._handleClosedWindows();
>  
>      var activeWindow = this._getMostRecentBrowserWindow();
>  
> +

Nit: newline

@@ +2090,5 @@
>        }
>      }
>  
> +    // shallow copy Session.closedWindows to preserve current state
> +    let lastClosedWindowsCopy = [winData for (winData of SessionState.closedWindows)];

let lastClosedWindowsCopy = [...SessionState.closedWindows];

::: browser/components/sessionstore/src/SessionWorker.js
@@ +477,5 @@
> +   * @param browserState (object)
> +   *        The browser state for which we remove any private windows and tabs.
> +   *        The given object will be modified.
> +   */
> +  _filterPrivateWindowsAndTabs: function(browserState) {

Why is this done in the worker? Differential updates for private windows shouldn't even be sent to the worker as well as updates for private tabs.

Comment 16

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

(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #15)
> Comment on attachment 8447114 [details] [diff] [review]
> SessionState, v5
> 
> Review of attachment 8447114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still haven't completely wrapped my head around how this is supposed to
> work. SessionState.closedWindows almost duplicates the Array.prototype API
> so that we can send diffs accordingly. What would these diffs look like, do
> you have an idea about that yet?

In a first version, a diff will be an array of

 ["insert", path, field or index, value]
 ["remove", path, field or index]
 ["set", path, field or index, value]

> How would we in the future implement sending a diff that moves a window from
> the list of open windows to the list of closed windows and vice versa? Would
> we try to optimize that or would send two diffs?

In a first version, I suspect that closing/reopening windows or tabs will not be optimized. At the moment, iirc, we immediately remove the window from _closedWindows and only later, once the window is opened, add it back to openWindows. That's something we will need to change if we wish to optimize. Additionally, open tabs and closed tabs do not have the exact same structure, which doesn't help either. Assuming we solve both issues, I envision the following extension to diff messages:

 ["remove", path, field or index, { cutTo: variableName }]
 ["insert", path, field or index, value, { copyFrom: variableName }]
 ["set", path, field or index, value, { copyFrom: variableName }]

in which `variableName` is valid only for a single diff. But I'm not planning to pursue this idea in the near future.

> Another thing confusing me is the annotations and private windows/tabs
> cleanup. Only the session workers wants and needs to know about the
> possibility of closed windows being secretly "restored" before writing to
> disk. The only exception is that the main thread needs to notify the worker
> about user actions so that annotations can be cleared. Private tabs and
> windows however should never make it to the worker in an ideal world.

I had an early prototype that attempted to filter private tabs and windows on the main thread and this complicated things considerably, so I decided to keep it simple for v1 and let the worker handle the charge.

> Would it be beneficial to try and make closed windows work by sending diffs
> to the worker in a second patch? As a proof-of-concept?

Will do.


> ::: browser/components/sessionstore/src/SessionState.jsm
> @@ +36,5 @@
> > +  /**
> > +   * A mechanism for storing in-memory annotations on values.
> > +   */
> > +  get annotations() {
> > +    return AnnotationsMap;
> 
> Great idea to use WeakMaps to get rid of that property but this doesn't
> really help us going forward, does it? "Restoring" windows is something that
> we would only do on disk writes so that would be done in the SessionWorker.
> The diff we would send to the session worker would basically insert another
> closed window and at the same time maintain the "shouldRestore" annotations.
> A special message would probably need to be sent to the worker to make it
> clear "shouldRestore" annotations on user action.

Well, I was attempting a straightfoward, less footgunish, port of the current algorithm. I admit that I do not understand well enough this algorithm to try and reinvent it, but I'm rather confident that the annotation mechanism is sufficient to implement it.

> @@ +182,5 @@
> > +
> > +  /**
> > +   * Add values at the ekend of the list
> > +   *
> > +   * WARNING: Do not modify `values` after this call.
> 
> Why not just clone the values using cloneInto()?

I was a bit scared of the cost, I admit.

> @@ +2090,5 @@
> >        }
> >      }
> >  
> > +    // shallow copy Session.closedWindows to preserve current state
> > +    let lastClosedWindowsCopy = [winData for (winData of SessionState.closedWindows)];
> 
> let lastClosedWindowsCopy = [...SessionState.closedWindows];

Nice.

> ::: browser/components/sessionstore/src/SessionWorker.js
> @@ +477,5 @@
> > +   * @param browserState (object)
> > +   *        The browser state for which we remove any private windows and tabs.
> > +   *        The given object will be modified.
> > +   */
> > +  _filterPrivateWindowsAndTabs: function(browserState) {
> 
> Why is this done in the worker? Differential updates for private windows
> shouldn't even be sent to the worker as well as updates for private tabs.

Because this forces us to be a bit schizophrenic. For one thing, if we do this, SessionState needs to remember all windows and all tabs, but SessionWorker remembers a different set of windows and a different set of tabs, with different indices everywhere. For a second thing, if we do this, every single diff needs to be filtered and rewritten entirely to eliminate private windows (easy), private tabs (harder, as it also requires changing the title if that's the active tab), non-private windows composed entirely of private tabs, etc.

This was much easier to do on the worker.

I am open to other ideas. Perhaps we should simply not store private windows or tabs in SessionData at all?

Comment 17

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

Ah, I was wrong. The annotation mechanism is completely ill-suited for `shouldRestore`. I have reimplemented something that should work much better. Waiting for the Try results.

Comment 18

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

Attachment: 1. Read-only membrane

Comment 19

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

Attachment: 2. SessionState, v6

After a few experiments, here is a rewrite of `shouldRestore` that should work with Differential Updates.

Comment 20

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

Attachment: 3. Tests

Comment 21

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

Attachment: 4. Differential Updates, proof of concept (DO NOT LAND)

As requested, here is a proof of concept for Differential Updates. I have thrown a few tests at it and they pass, but I haven't made sure that everything does. Lots of debugging code still lying around, too.

Comment 22

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

(actually, it seems to pass all tests on Linux)

Comment 23

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

Comment on attachment 8450835 [details] [diff] [review]
4. Differential Updates, proof of concept (DO NOT LAND)

Canceling review requests, due to review starvation.