1413525 - Implement a general mechanism for saving sessions

Status: NEW

(WebExtensions :: General, enhancement, P5)

Description

[u462496]

I have used SessionStore.getBrowserState and SessionStore.setBrowserState to provide feature for backing up and restoring sessions in one of my addons.

Can these APIs be exposed as a WebExtension?

Comment 1

[Caitlin Neiman [:caitmuenster]]

Hi Kevin, this has been added to the agenda for the November 14 WebExtensions APIs triage meeting. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/Add-ons/Contribute/Triage
* Meeting agenda: https://docs.google.com/document/d/1g3RMfKZ3671NcusMqkoOiKwfPekRe-VI7Rzqxo6F_Ao/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

Comment 2

[u462496]

Hey Andy, sorry I didn't make it to the meeting today.  I read over the notes however.  Glad to see this was approved :-).

The use cases I think I outlined in the OP, but more details, SessionStore already provides a ready-built API for backing up and restoring sessions.  As simple as implementing two methods SessionStore.getBrowserState and SessionStore.setBrowserState, wham bam.

One of my addons implemented a session backup and restore feature, and I would like to continue to offer this feature, either in one of my new addons or an addon of its own.  This feature is especially useful in the event of a fatal crash, and some of my users have been very grateful to be able to recover their session.  If the user felt a session might be corrupt, they could choose and earlier session they felt was safe to restore to.

IIRC, the mainstay addon Session Manager also implemented these methods, though I don't know Session Manager's future plans at this point.  In Session Manager's case, more than just backup and recovery, (IIRC) they also provided features for users to take snapshots of sessions to be kept in an archive for retrieval later.

I believe other session manager type addons use SessionStore.getBrowserState and SessionStore.setBrowserState as well, although I pretty sure TMP had its own system.  (That however may be much more difficult/impossible with WebExtensions)

Comment 3

[u462496]

Attachment: bug_1413525_fb1.diff

Here is a proposal.

Note that I included an alternate for sanitizeBrowserState in ext-sessions.js. (sanitizeBrowserState2 in the code).  This offers a simpler and less resource intensive option if we just want to keep things simple.

Comment 4

[u462496]

Attachment: 019.SUBMIT_feedback_bug_1413525_fb2.diff

Fixed coding errors, added more tests.

Comment 5

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8931903 [details] [diff] [review]
019.SUBMIT_feedback_bug_1413525_fb2.diff

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

Please pardon the drive-by...

I've added a comment/question about the sanitize function below. Also, I think you should request feedback/review from [e-mail] for this patch.

::: browser/components/extensions/ext-sessions.js
@@ +82,5 @@
>  
>    return {encodedKey, win};
>  };
>  
> +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {

I think I understand why we need to do this, but could you explain the logic from your perspective? What problems/risks are we trying to mitigate? Why do we need to do it both when setting browserState and when retrieving browserState? Also, do you have any concerns about the fact that there will be cases where the state we report to the extension, and/or the resulting browser state will not be the same as what was expected, due to this mitigation? Should any type of warning be generated if tabs get removed/set to about:blank as a result of this sanitization?

Comment 6

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #5)
> Comment on attachment 8931903 [details] [diff] [review]
> 019.SUBMIT_feedback_bug_1413525_fb2.diff
> 
> Review of attachment 8931903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please pardon the drive-by...

No problem, I appreciate the input.

> ::: browser/components/extensions/ext-sessions.js
> @@ +82,5 @@
> >  
> >    return {encodedKey, win};
> >  };
> >  
> > +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {
> 
> I think I understand why we need to do this, but could you explain the logic
> from your perspective? What problems/risks are we trying to mitigate?

I am only concluding from the fact that tabs.update does not allow internal urls that this would be necessary here as well.  eg, an addon could inject a tab with an internal url into the browser state.

If there is a distinction here which makes the sanitizing unnecessary, I am happy to know that.

Furthermore, I don't personally understand the safety concerns in allowing addons to open tabs with "about" urls for instance; if someone could explain that to me I'd very much appreciated.

> Why do
> we need to do it both when setting browserState and when retrieving
> browserState?

Probably only need it for setting, however it seems to me there is no point in giving a user a session which will just get mangled anyway if they try to apply it.  If they get one that is already mangled, they know what they will have to deal with.  Though resource-wise, I can see the argument for only sanitize on set.

> Also, do you have any concerns about the fact that there will
> be cases where the state we report to the extension, and/or the resulting
> browser state will not be the same as what was expected, due to this
> mitigation? Should any type of warning be generated if tabs get removed/set
> to about:blank as a result of this sanitization?

If there is a warning given when trying to update or create a tab with an illegal url then I suppose we should follow suite, but if not, I personally don't think we need to here and just having the distinction in the documentation should suffice.

Comment 7

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #5)
> Also, I
> think you should request feedback/review from [e-mail] for this
> patch.

Mike is PTO until January 13 :-/

Comment 8

[Shane Caraveo (:mixedpuppy)]

Kevin, could you use reviewboard? It makes this process easier.

Comment 9

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> Kevin, could you use reviewboard? It makes this process easier.

Well... Maybe I'll try.  It has always been a battle for me to get it to work and I haven't been successful.  I'd rather write code...

Comment 10

[Shane Caraveo (:mixedpuppy)]

> > > +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {
> > 
> > I think I understand why we need to do this, but could you explain the logic
> > from your perspective? What problems/risks are we trying to mitigate?
> 
> I am only concluding from the fact that tabs.update does not allow internal
> urls that this would be necessary here as well.  eg, an addon could inject a
> tab with an internal url into the browser state.

Yeah, this is unfortunate.  I wonder if anyone has any bright idea's on a way to round trip the items retrieved via get.  

In any case, internal urls are probably easy enough for users to reopen, whereas remembering all the bugzilla tabs you had open is an entirely different problem that this would still solve.

Comment 11

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > > > +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {
> > > 
> > > I think I understand why we need to do this, but could you explain the logic
> > > from your perspective? What problems/risks are we trying to mitigate?
> > 
> > I am only concluding from the fact that tabs.update does not allow internal
> > urls that this would be necessary here as well.  eg, an addon could inject a
> > tab with an internal url into the browser state.
> 
> Yeah, this is unfortunate.  I wonder if anyone has any bright idea's on a
> way to round trip the items retrieved via get.

So do you know the reason for not allowing about: pages to be opened by an addon?

> In any case, internal urls are probably easy enough for users to reopen,
> whereas remembering all the bugzilla tabs you had open is an entirely
> different problem that this would still solve.

I'm not following you here.  Are you saying that bugzilla pages are illegal urls?

Comment 12

[u462496]

Attachment: Bug 1413525 - browser.sessions.get/setBrowserState

Review commit: https://reviewboard.mozilla.org/r/203280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203280/

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Kevin Jones from comment #11)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > > > > +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {
> > > > 
> > > > I think I understand why we need to do this, but could you explain the logic
> > > > from your perspective? What problems/risks are we trying to mitigate?
> > > 
> > > I am only concluding from the fact that tabs.update does not allow internal
> > > urls that this would be necessary here as well.  eg, an addon could inject a
> > > tab with an internal url into the browser state.
> > 
> > Yeah, this is unfortunate.  I wonder if anyone has any bright idea's on a
> > way to round trip the items retrieved via get.
> 
> So do you know the reason for not allowing about: pages to be opened by an
> addon?

Some of them may have security issues around allowing extension access.  There are bugs to review that.

> > In any case, internal urls are probably easy enough for users to reopen,
> > whereas remembering all the bugzilla tabs you had open is an entirely
> > different problem that this would still solve.
> 
> I'm not following you here.  Are you saying that bugzilla pages are illegal
> urls?

no, I'm saying you'd be able to backup and restore those.  While it would be nice to do that for internal urls, I don't think its nearly as important.

Comment 14

[u462496]

(In reply to Kevin Jones from comment #12)
> Created attachment 8932240 [details]
> Bug 1413525 - browser.sessions.get/setBrowserState
> 
> Review commit: https://reviewboard.mozilla.org/r/203280/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/203280/

Don't if you were notified but here is the reviewboard version (I think).

Comment 15

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> (In reply to Kevin Jones from comment #11)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > > > > > +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {
> > > > > 
> > > > > I think I understand why we need to do this, but could you explain the logic
> > > > > from your perspective? What problems/risks are we trying to mitigate?
> > > > 
> > > > I am only concluding from the fact that tabs.update does not allow internal
> > > > urls that this would be necessary here as well.  eg, an addon could inject a
> > > > tab with an internal url into the browser state.
> > > 
> > > Yeah, this is unfortunate.  I wonder if anyone has any bright idea's on a
> > > way to round trip the items retrieved via get.
> > 
> > So do you know the reason for not allowing about: pages to be opened by an
> > addon?
> 
> Some of them may have security issues around allowing extension access. 
> There are bugs to review that.
> 
> > > In any case, internal urls are probably easy enough for users to reopen,
> > > whereas remembering all the bugzilla tabs you had open is an entirely
> > > different problem that this would still solve.
> > 
> > I'm not following you here.  Are you saying that bugzilla pages are illegal
> > urls?
> 
> no, I'm saying you'd be able to backup and restore those.  While it would be
> nice to do that for internal urls, I don't think its nearly as important.

I actually don't know what an addon would be getting access to by setBrowserState, maybe sanitization is not necessary in this case?

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Kevin Jones from comment #15)

> I actually don't know what an addon would be getting access to by
> setBrowserState, maybe sanitization is not necessary in this case?

Actually I think it's less important to sanitize get (but I'm fine doing so).  I think until a review is done (see bug 1269456) of what about urls can be accessed by extensions we should sanitize set.  We should also have a followup bug to update the sanitize once bug 1269456 is addressed.

Comment 17

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> (In reply to Kevin Jones from comment #15)
> 
> > I actually don't know what an addon would be getting access to by
> > setBrowserState, maybe sanitization is not necessary in this case?
> 
> Actually I think it's less important to sanitize get (but I'm fine doing
> so).  I think until a review is done (see bug 1269456) of what about urls
> can be accessed by extensions we should sanitize set.  We should also have a
> followup bug to update the sanitize once bug 1269456 is addressed.

I don't think we should sanitize anything if it is not useful for some reason (eg, get) as it is just a waste of resources.

What are your thoughts on using the "simple" sanitization method?

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8932240 [details]
Bug 1413525 - browser.sessions.get/setBrowserState

https://reviewboard.mozilla.org/r/203280/#review208658

::: browser/components/extensions/ext-sessions.js:86
(Diff revision 1)
>    let win = windowTracker.getWindow(id, context);
>  
>    return {encodedKey, win};
>  };
>  
> +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {

drop "const sanitizeBrowserState ="

::: browser/components/extensions/ext-sessions.js:97
(Diff revision 1)
> +
> +  let windows = [];
> +  for (let window of state.windows) {
> +    // If tabs get removed, window.selected may need correction.  If the originally
> +    // selected tab gets removed, select the tab immediately preceding.
> +

Seems like you could use filter/map and then fixup the indexing after.

roughly something kind of like:

canload = (url) => {
   return context.checkLoadURL(context.uri.resolve(url), {dontReportErrors: true});
}

function tabfilter(window) {
window.tabs = window.tabs.filter(tab => {
  let selected = tab.entries[tab.index];
  tab.entries = tab.entries.filter(entry => {
   return canload(entry.url);
  });
  ...some kind of index fixup...
  tab.index = entries.indexOf(selected) || 0
  return tab.entries.length >= 0 || tab.userTypedValue && canload(tab.userTypedValue)
});
return window.tabs.length >= 0;
}

windows = windows.filter(tabfilter);
windows.selected = windows.has(priorSelectedWin) || 0

::: browser/components/extensions/ext-sessions.js:154
(Diff revision 1)
> +  return JSON.stringify(state);
> +};
> +
> +const sanitizeBrowserState2 = function sanitizeBrowserState2(state, context) {
> +  // REVIEWER: Alternate to sanitizeBrowserState, simple and less resource intensive.
> +  // Replace entries with illegal URLs with about:blank.

I'd rather not have strange side effects.

Comment 19

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {
> 
> drop "const sanitizeBrowserState ="

Why are all the other functions scripted that way?

Comment 20

[Bob Silverberg [:bsilverberg]]

(In reply to Kevin Jones from comment #19)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > > +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {
> > 
> > drop "const sanitizeBrowserState ="
> 
> Why are all the other functions scripted that way?

https://bugzilla.mozilla.org/show_bug.cgi?id=1374237

Comment 21

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #20)
> (In reply to Kevin Jones from comment #19)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > > > +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {
> > > 
> > > drop "const sanitizeBrowserState ="
> > 
> > Why are all the other functions scripted that way?
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1374237

Okay, so now I'm confused, why are you asking me to drop it in this case?  Looking over the bug I didn't see why this would be an exception.

Comment 22

[u462496]

Related to bug 1421254.

Comment 23

[Bob Silverberg [:bsilverberg]]

(In reply to Kevin Jones from comment #21)
> (In reply to Bob Silverberg [:bsilverberg] from comment #20)
> > (In reply to Kevin Jones from comment #19)
> > > (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > > > > +const sanitizeBrowserState = function sanitizeBrowserState(state, context) {
> > > > 
> > > > drop "const sanitizeBrowserState ="
> > > 
> > > Why are all the other functions scripted that way?
> > 
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1374237
> 
> Okay, so now I'm confused, why are you asking me to drop it in this case? 
> Looking over the bug I didn't see why this would be an exception.

I didn't ask you to drop it, Shane did. Maybe he has a reason for doing so, but the bug I quoted above is, imo, a reason not to drop it. Perhaps the need for that has gone away with some changes? Shane?

Comment 24

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #23)
> I didn't ask you to drop it, Shane did. Maybe he has a reason for doing so,
> but the bug I quoted above is, imo, a reason not to drop it. Perhaps the
> need for that has gone away with some changes? Shane?

Ooops, sorry for the mix-up.

Comment 25

[u462496]

Attachment: Bug 1413525 - browser.sessions.get/setBrowserState - update #1

Review commit: https://reviewboard.mozilla.org/r/203574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203574/

Comment 26

[u462496]

(In reply to Kevin Jones from comment #25)
> Created attachment 8932525 [details]
> Bug 1413525 - browser.sessions.get/setBrowserState - update #1
> 
> Review commit: https://reviewboard.mozilla.org/r/203574/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/203574/

I've updated the patch with the suggestions Shane offered.  I'll look into reporting when a session gets altered due to illegal urls after everyone's okay with the functional code.

There is an intermittent bug in the test where sometimes the session returned from getBrowserState has an empty tab.entries entry for the about:config tab that gets initially loaded.  Not sure if this is a race thing or what, it's been pretty elusive and hard to debug, but I'll keep working on it.  In the meanwhile I'm wanting to see if things are looking like they should.

Comment 27

[u462496]

(In reply to Kevin Jones from comment #26)
> (In reply to Kevin Jones from comment #25)
> > Created attachment 8932525 [details]
> > Bug 1413525 - browser.sessions.get/setBrowserState - update #1
> > 
> > Review commit: https://reviewboard.mozilla.org/r/203574/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/203574/
> 
> I've updated the patch with the suggestions Shane offered.  I'll look into
> reporting when a session gets altered due to illegal urls after everyone's
> okay with the functional code.
> 
> There is an intermittent bug in the test where sometimes the session
> returned from getBrowserState has an empty tab.entries entry for the
> about:config tab that gets initially loaded.  Not sure if this is a race
> thing or what, it's been pretty elusive and hard to debug, but I'll keep
> working on it.  In the meanwhile I'm wanting to see if things are looking
> like they should.

Oh yes, and also I've refrained from removing "const sanitizeBrowserState =" until we hear back from Shane about this.

Comment 28

[Shane Caraveo (:mixedpuppy)]

(In reply to Bob Silverberg [:bsilverberg] from comment #23)

> > > > > drop "const sanitizeBrowserState ="
> > > > 
> > > > Why are all the other functions scripted that way?
> > > 
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=1374237
> > 
> > Okay, so now I'm confused, why are you asking me to drop it in this case? 
> > Looking over the bug I didn't see why this would be an exception.
> 
> I didn't ask you to drop it, Shane did. Maybe he has a reason for doing so,
> but the bug I quoted above is, imo, a reason not to drop it. Perhaps the
> need for that has gone away with some changes? Shane?

Nope, I forgot we moved to doing things that way.  Old habits die hard.

Comment 29

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8932525 [details]
Bug 1413525 - browser.sessions.get/setBrowserState - update #1

https://reviewboard.mozilla.org/r/203574/#review209050

::: browser/components/extensions/ext-sessions.js:106
(Diff revision 1)
> -          pushTab = true;
> -        }
> -      }
> -      if (pushTab) {
> -        tab.entries = entries;
> -        tabs.push(tab);
> +      let selected = tab.entries[tab.index - 1];
> +      tab.entries = tab.entries.filter(entry => {
> +        return canLoad(entry.url);
> +      });
> +      tab.index = tab.entries.indexOf(selected) || 0;
> +      tab.index++;

see comment below

::: browser/components/extensions/ext-sessions.js:110
(Diff revision 1)
> -        tab.entries = entries;
> -        tabs.push(tab);
> -        if (i < selected) {
> -          selectedCorrected++;
> -        }
> -      }
> +      tab.index = tab.entries.indexOf(selected) || 0;
> +      tab.index++;
> +      return tab.entries.length > 0 || (tab.userTypedValue && canLoad(tab.userTypedValue));
> +    });
> +    window.selected = window.tabs.indexOf(selectedTab) || 0;
> +    window.selected++;

This is a little weird for readability.  Can you add the +1 to the line above?  That will make it more obvious (due to getting selectedWindow at the beginning of this).

::: browser/components/extensions/ext-sessions.js:116
(Diff revision 1)
> -    }
> -    if (tabs.length) {
> -      window.tabs = tabs;
> -      window.selected = selectedCorrected;
> -      windows.push(window);
> -    }
> +    return window.tabs.length > 0;
> +  });
> +
> +  if (state.windows.length) {
> +    state.selectedWindow = state.windows.indexOf(selectedWindow) || 0;
> +    state.selectedWindow++;

same here again.

::: browser/components/extensions/ext-sessions.js:249
(Diff revision 1)
>  
>            SessionStore.deleteWindowValue(win, encodedKey);
>          },
>  
>          async getBrowserState() {
> -          return sanitizeBrowserState(SessionStore.getBrowserState(), context);
> +          return SessionStore.getBrowserState();

Just thinking about this.   We're returning the raw output from sessionstore, which in itself is not problematic, but what if that changes in the future?  I'm wondering if we should validate the structure, or at least consider if the structure is actually what we want.  It would be good to get a touch of documentation on what could be contained in it.

::: browser/components/extensions/ext-sessions.js:253
(Diff revision 1)
>          async getBrowserState() {
> -          return sanitizeBrowserState(SessionStore.getBrowserState(), context);
> +          return SessionStore.getBrowserState();
>          },
>  
>          setBrowserState(state) {
>            SessionStore.setBrowserState(sanitizeBrowserState(state, context));

Kind of related to the above thought.  Here we're sanitizing, but not validating.  Also, what happens if state is not what we expect it to be?  We proabably want to throw some error that will be reasonable for the extension to receive.

Comment 30

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8932525 [details]
Bug 1413525 - browser.sessions.get/setBrowserState - update #1

https://reviewboard.mozilla.org/r/203574/#review209050

> Just thinking about this.   We're returning the raw output from sessionstore, which in itself is not problematic, but what if that changes in the future?  I'm wondering if we should validate the structure, or at least consider if the structure is actually what we want.  It would be good to get a touch of documentation on what could be contained in it.

At a minimum, we probably want a test that validates the structure, so if it does change in the future, the test will fail.  Unfortunately the schema doesn't support this validation yet.

Comment 31

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8932525 [details]
Bug 1413525 - browser.sessions.get/setBrowserState - update #1

https://reviewboard.mozilla.org/r/203574/#review209050

> At a minimum, we probably want a test that validates the structure, so if it does change in the future, the test will fail.  Unfortunately the schema doesn't support this validation yet.

I agree with Shane's first point here. Rather than return our internal structure, perhaps we should design a structure that makes sense for the API, and then convert the internal state to that structure? We would then also expect the same structure to be passed into the method that attempts to set state. It seems a pretty bad idea to expose the internals of SessionStore to extensions.

Comment 32

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #31)
> Comment on attachment 8932525 [details]
> Bug 1413525 - browser.sessions.get/setBrowserState - update #1
> 
> https://reviewboard.mozilla.org/r/203574/#review209050
> 
> > At a minimum, we probably want a test that validates the structure, so if it does change in the future, the test will fail.  Unfortunately the schema doesn't support this validation yet.
> 
> I agree with Shane's first point here. Rather than return our internal
> structure, perhaps we should design a structure that makes sense for the
> API, and then convert the internal state to that structure? We would then
> also expect the same structure to be passed into the method that attempts to
> set state. It seems a pretty bad idea to expose the internals of
> SessionStore to extensions.

Here are my suggestions.  I would appreciate input on this:

[1] We should take a whitelist approach as far as the filter/converter goes.
[2] Keep the structure like it is now, with the absence of properties which do not qualify.  This will simplify conversion at least at present.  I'm guessing this structure will not change much internally in the future, but if it does the added complexity needed can be changed then.
[3] Compile a white list based on properties which, 1, do not expose data which could be exploited, 2, are not properties which only affect subtle sessionstore internals but are unlikely to be important from a user standpoint, 3, only seem necessary for what a casual user generally would want to be restored.  ie, keep the list minimal, rather than trying to cover every possible use case, and if along the way someone wishes to include a property this can be considered later.  Just having urls would be 90% to a user's relief if trying to recover a crashed/corrupted session. 

I have examined a fairly common and diverse session which I include at the end of this comment.  While it may not cover every possible property I am fairly certain it would include everything necessary for a whitelist.  Of course to find some actual documentation on this would be good.  Maybe :mikedeboer can comment when he returns.  But based on what I examined, here are my suggestions:

Global:

  include:

    windows
    selectedWindow
    _closedWindows
    cookies  (all attributes?)
    browserConsole
    scratchpads

  don't include or questionable?

    version
    session
    global

window:

  include:

    tabs
    selected
    _closedTabs
    width
    height
    screenX
    screenY
    sizemode

  don't include or questionable?

    busy

tab:

  include:

    entries
    lastAccessed
    hidden
    userContextId
    index
    userTypedValue
    userTypedClear
    formdata  (include all attributes of this?)
    scroll
    image

  don't include or questionable?

    mediaBlocked
    attributes
    storage  (what is this used for?)
    iconLoadingPrincipal

entry:

  include:

    url
    title
    referrer
    referrerPolicy
    originalURI

  don't include or questionable?

    ID
    docshellUUID
    resultPrincipalURI
    loadReplace
    loadReplace2
    presState  (what the heck is this?)
    principalToInherit_base64
    triggeringPrincipal_base64
    docIdentifier
    structuredCloneState
    structuredCloneVersion
    persist  (how is this applied?)

Sample:

{
  "version": ["sessionrestore", 1],
  "windows": [{
    "tabs": [{
      "entries": [{
        "url": "http://...",
        "title": "Title",
        "ID": 64817910,
        "docshellUUID": "...",
        "referrer": "https://...",
        "referrerPolicy": 4,
        "originalURI": "https://...",
        "resultPrincipalURI": null,
        "loadReplace": true,
        "loadReplace2": true,
        "presState": [{
          "stateKey": "0>html>1",
          "scroll": "0,15360"
        }],
        "principalToInherit_base64": "==",
        "triggeringPrincipal_base64": "==",
        "docIdentifier": 2,
        "structuredCloneState": "==",
        "structuredCloneVersion": 8,
        "persist": true
      }],
      "lastAccessed": 1511907875423,
      "hidden": false,
      "mediaBlocked": true,
      "attributes": {},
      "userContextId": 0,
      "index": 2,
      "userTypedValue": "http://...",
      "userTypedClear": 0,
      "formdata": {
        "id": {
          "textbox": "..."
        },
        "url": "..."
      },
      "storage": {
        "https://...": {
          "wmE-GeoFeaturesUser": "...",
          "mwuser-sessionId": "..."
        }
      },
      "scroll": {
        "scroll": "0,704"
      },
      "image": null,
      "iconLoadingPrincipal": null
    }],
    "selected": 2,
    "_closedTabs": [],
    "busy": false,
    "width": 1270,
    "height": 740,
    "screenX": 622,
    "screenY": 22,
    "sizemode": "normal"
  }],
  "selectedWindow": 1,
  "_closedWindows": [],
  "session": {
    "lastUpdate": 1511908101478,
    "startTime": 1509184255768,
    "recentCrashes": 0
  },
  "global": {},
  "cookies": [{
    "host": "...",
    "value": "...",
    "path": "/",
    "name": "...",
    "originAttributes": {
      "appId": 0,
      "firstPartyDomain": "",
      "inIsolatedMozBrowser": false,
      "privateBrowsingId": 0,
      "userContextId": 0
    }
  }],
  "browserConsole": false,
  "scratchpads": []
}

Comment 33

[Bob Silverberg [:bsilverberg]]

I know he's on PTO until the new year, but I really think we should get some input/feedback from Mike before finalizing a design, so perhaps we should shelve this for now.

Comment 34

[Shane Caraveo (:mixedpuppy)]

Yeah, there is a lot in that to digest, I agree we should get someone who is very familiar with the sessionstore.  Seeing this I'm inclined to not do this api at all without defining a small very specific data set.  Allowing extensions access to the raw underlying api data could easily tie our hands.  

I have a question (just exploring alternatives).  Is there a reason for an extension to have the ability to modify the session state at all?  What if there were just a browser.session.save/restore api that would save a backup written into the profile directory?  Restore would just read that.   Or, perhaps there is a way to extract/insert only particular data in a way that would work, without loosing the other items (seems hard).  Most of the rest of the sessions api seems to go that route.

I realize that a different approach may be a lot less capable in some ways, but the write-ability of that session data from an extension seems full of edge cases with so many options and values.  Given the breadth of information in there, I would be surprised if it did not change in the future.

Given all this, I'm going to r- for now.  Lets get more input and go back to the drawing board.

Comment 35

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #34)

I'm in agreement with getting more input.  However just a couple of comments:

> Seeing this I'm inclined to not do
> this api at all without defining a small very specific data set.

A minimal set of data would be okay with me personally.  My main concern in backup/restore sessions is that tabs and their respective urls are restore-able.  The main idea (from my ambitions) is that a user can at least restore their tabs in the event of a catastrophe.  Even being able to preserve things such as window sizes and position, form data, etc probably isn't all that meaningful once someone has moved along.

> I have a question (just exploring alternatives).  Is there a reason for an
> extension to have the ability to modify the session state at all?  What if
> there were just a browser.session.save/restore api that would save a backup
> written into the profile directory?  Restore would just read that.   Or,
> perhaps there is a way to extract/insert only particular data in a way that
> would work, without loosing the other items (seems hard).  Most of the rest
> of the sessions api seems to go that route.

That seems that would involve re-inventing a very large and complex wheel, which sessionstore already takes care of very neatly.

Comment 36

[Shane Caraveo (:mixedpuppy)]

(In reply to Kevin Jones from comment #35)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #34)

> That seems that would involve re-inventing a very large and complex wheel,
> which sessionstore already takes care of very neatly.

No, it would just be having sessionstore do a save/load to/from an alternate file.

Comment 37

[Elhem Enohpi]

I'd like to mention a couple of things about the Session Manager addon that might be relevant here.

Besides restoring multi-window browser sessions, you can independently save, close, and re-open any browser window, just like working with standard word processing or spreadsheet documents. I have windows for each project I'm working on, and can open and close any combination of them at any time. I can't tell you what a difference it makes to working with the browser: never having to stop and do "bookmark all tabs", and not having to lose all the related data like cookies, tab histories, Tree Style Tab structure, and so on, when you need to close a window to free up RAM/CPU resources!

If it's decided to expose SessionStore.getBrowserState and SessionStore.setBrowserState, then I'd ask to expose SessionStore.getWindowState and SessionStore.setWindowState as well, in order to support this capability.

(In reply to Shane Caraveo (:mixedpuppy) from comment #34)
> I have a question (just exploring alternatives).  Is there a reason for an
> extension to have the ability to modify the session state at all?  What if
> there were just a browser.session.save/restore api that would save a backup
> written into the profile directory?  Restore would just read that.   Or,
[...]

That's a good point. Session Manager users are used to having all session data saved and restored, exactly the way that "show my windows and tabs from last time" does. So it makes sense to use the same mechanism. Rather than sanitize the session data to some very restricted subset to be passed to an extension, why not just give SessionStore itself the ability to save/restore sessions (and individual windows!) to different named files or a database? A simple API could then be provided, that doesn't expose the session internals. I think most of the important features of Session Manager could be duplicated in a new extension without needing access to the session data itself.

Some of the things Session Manager does that might need access to the session data, or special consideration in a new API, include:
- show a list of windows and tabs when loading a session, and allow you to uncheck any you don't want loaded. Mostly useful when a tab is causing a crash.
- append sessions to the current one, rather than replace; also restore independent windows without closing others.
- control over some things that are saved in a session, such as number of back/forward history entries, closed windows and tabs, cookies, form data, etc., that are kept.
- cache web page data to allow searching inside sessions before loading.
- session metadata - grouping (like tags), renaming, deleting, etc.

One thing that many people asked for, though it was never implemented, was the ability to add/remove windows and tabs from saved sessions on disk, without having to load them.

Comment 38

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #36)
> (In reply to Kevin Jones from comment #35)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #34)
> 
> > That seems that would involve re-inventing a very large and complex wheel,
> > which sessionstore already takes care of very neatly.
> 
> No, it would just be having sessionstore do a save/load to/from an alternate
> file.

Oh, I see, I totally misinterpreted your comment.  I see what you are talking about now.  I think in the event of implementing something like you describe, there should be a way to read at least some of the data which is being stored, so an addon can offer the user a UI which gives a visual representation of what the saved sessions look like.

And no, for my purposes, there would never be a need to modify a session; I am just looking at saving a backup which the user can revert to in the event of a catastrophe which the native restore is unable to recover.

Comment 39

[u462496]

I will no longer be able to spend time contributing to bugs for Mozilla.  I will no longer be working on this bug.

Comment 40

[Tim Morley]

(In reply to Shane Caraveo (:mixedpuppy) from comment #34)
> I have a question (just exploring alternatives).  Is there a reason for an
> extension to have the ability to modify the session state at all?  What if
> there were just a browser.session.save/restore api that would save a backup
> written into the profile directory?  Restore would just read that.   Or,
> perhaps there is a way to extract/insert only particular data in a way that
> would work, without loosing the other items (seems hard).  Most of the rest
> of the sessions api seems to go that route.

I've used Session Managers ability for modify a session before I restore it for 2 reasons. 

The minor one is just that it can be easier to get rid of a bunch of tabs by deselecting them before the restore.

The more important use, is for when a tab breaks the browser. I've had a tab that basically wasn't closable, so being about to choose not to restore that tab on a restart can be useful.

Tim

Comment 41

[Tim Nguyen :ntim]

Looks like we could use the internal API setBrowserState to add a sessionId to the windows. Might be better WebExtensions-wise.

Comment 42

[Mike de Boer [:mikedeboer]]

I'm back! :-) Nonetheless, apology for the late reply here.

Alright, so I'm quite keen on having a backup/ restore method implemented that a WebExtension can peruse! I don't think, however, that exposing the internal data structure (JSON in this case - but for the sake of argument, it may change in the future!) and have WebExtensions rely on these implementation details.

So strictly speaking, that would mean a WONTFIX for this bug. I do think, however, that there's a big opportunity for adding something very specific, like:

`.saveToFile(fileName)` -> `Promise`, resolve upon successful write to disk and reject/ throw on error.
`.loadFromFile(fileName)` -> `Promise`, resolve up successful session file load (restore) and reject when aborted. Throw when error occurs and load was aborted.

This is much more specific, the code in extensions/ would be minimal since I'd recommend the implementation to be done in a separate JS module in the sessionstore/ folder.

What do you think?

Comment 43

[Mike de Boer [:mikedeboer]]

Oh and the filename would need to be unique and the folder is a fixed one that already exists in your profile directory: '~/[ProfilesDir]/[profileName]/sessionstore-backups/'.

Comment 44

[Rafał Mużyło]

@comment 42: just to make sure both sides are equally aware of each other, an interesting discussion took place here: https://github.com/sienori/Tab-Session-Manager/issues/131

Comment 45

[Elhem Enohpi]

(In reply to Mike de Boer [:mikedeboer] from comment #42)

> So strictly speaking, that would mean a WONTFIX for this bug. I do think,
> however, that there's a big opportunity for adding something very specific,
> like:
> 
> `.saveToFile(fileName)` -> `Promise`, resolve upon successful write to disk
> and reject/ throw on error.
> `.loadFromFile(fileName)` -> `Promise`, resolve up successful session file
> load (restore) and reject when aborted. Throw when error occurs and load was
> aborted.
> 
> What do you think?

I think it's a good idea. It would allow fully restoring a session, including eg. other extensions' data, set with SessionStore.setWindow/TabValue, which otherwise would be impossible. It would also hugely simplify coding of basic "session manager" extensions, which could still offer most of what legacy Session Manager did.

Support for "window sessions" like Session Manager's would just need something like:
`.saveWindowToFile(id,fileName)` - save window "id" as a single-window session.
Plus a method to load a session without closing the current windows; either `.loadFromFile(fileName)` always works that way and the extension has to take care of closing windows first, or it also needs `.appendFromFile(fileName)`.

There should be a way to get a list of the window and tab titles in a session, before loading it. Also to deselect any that are causing a problem, like Firefox's existing "session restore" window - the one that says "this is embarassing..." Obvious additions would be list/delete/renameFile. A huge bonus would be a facility to add windows or tabs to a saved session without having to load it.

I wonder if it would be valuable to have some additional metadata for sessions? Session Manager had "name", "timestamp", "autosave (session-type(session/window session) / save interval)", "count (of windows / tabs)", "group", and "screensize". I could see "tags" being helpful. On the other hand, I suppose an extension could keep its own database of metadata, keyed to the filenames.

PS, I should say that my dream actually is to have a "Save" item in the File menu, that would simply save a window as a session document anywhere in my file system, just like most every other app can save documents. I've never understood what that's not blazingly obvious to everyone. Just call me Cassandra...!

Comment 46

[Tim Nguyen :ntim]

I honestly think that the concept of file I/O shouldn't be introduced here, it's just inconsistent with every single WE API.

How about just adding `browser.sessions.save(sessionId)`, then changing the existing browser.sessions.restore(sessionId) method to work with saved sessions IDs ?

For Elhem's use case of getting session data before restoring, then a `browser.sessions.get(sessionId)` method returning WebExtensions `Session` objects could work ?

As for saving a specific window as session: `browser.sessions.save(windowId, sessionId)` could be supported.


This would work so much better with existing session APIs :)

Comment 47

[swleefers]

Incidentally (I don't know whether this would be relevant), there are various other cases where users could benefit from file I/O, such as when a user wants to edit a userscript in his own favourite code editor, as opposed to some editor created by the add-on developer. Greasemonkey has all sorts of bugs open where people ask for a linter in the editor and other features. Any file I/O should be restricted to the add-on's own special folder.

Comment 48

[Robert AB]

(In reply to Elhem Enohpi from comment #45)
> (In reply to Mike de Boer [:mikedeboer] from comment #42)
> 
> > So strictly speaking, that would mean a WONTFIX for this bug. I do think,
> > however, that there's a big opportunity for adding something very specific,
> > like:
> > `.saveToFile(fileName)` -> `Promise`, resolve upon successful write to disk
> > and reject/ throw on error.
> > `.loadFromFile(fileName)` -> `Promise`, resolve up successful session file
> > load (restore) and reject when aborted. Throw when error occurs and load was
> > aborted.
> > 
> > What do you think?
> 
>
>
>
> I think it's a good idea. It would allow fully restoring a session, including eg. other extensions' data, 
> set with SessionStore.setWindow/TabValue, which otherwise would be impossible. It would also hugely 
> simplify coding of basic "session manager" extensions, which could still offer most of what legacy 
> Session Manager did.
> 
> Support for "window sessions" like Session Manager's would just need something like:
> `.saveWindowToFile(id,fileName)` - save window "id" as a single-window session.
> Plus a method to load a session without closing the current windows; either `.loadFromFile(fileName)` 
> always works that way and the extension has to take care of closing windows first, or it also needs
> `.appendFromFile(fileName)`.


Saving single windows in addition to saving entire sessions could be beneficial. I do not know if this is relevant here
but there is Bug 1235231 (Sessions restore windows in random order), which has been present in Firefox for years and noticeable with and without Session Manager.


> There should be a way to get a list of the window and tab titles in a session, before loading it. Also to 
> deselect any that are causing a problem, like Firefox's existing "session restore" window - the one that 
> says "this is embarassing..." Obvious additions would be list/delete/renameFile. A huge bonus would be a 
> facility to add windows or tabs to a saved session without having to load it.


I should be also possible to change order of windows and tabs in saved sessions, preferably by dragging using the mouse. Maybe two sessions or windows from the same session could be viewed (without opening), the same way like files in Norton Commander.

 
> I wonder if it would be valuable to have some additional metadata for sessions? Session Manager had "name", 
> "timestamp", "autosave (session-type(session/window session) / save interval)", "count (of windows / tabs)", 
> "group", and "screensize". I could see "tags" being helpful. On the other hand, I suppose an extension 
> could keep its own database of metadata, keyed to the filenames.
> 
> PS, I should say that my dream actually is to have a "Save" item in the File menu, that would simply save 
> a window as a session document anywhere in my file system, just like most every other app can save 
> documents. I've never understood what that's not blazingly obvious to everyone. Just call me Cassandra...!


In addition, to saving sessions/windows with page addresses, page states, tab history for each tab, it would be useful to save complete page content of all tabs in the browser window/session, so windows/sessions could open on computer without internet access, or later (after days, months), so the content of pages would be visible the same way like at the time of saving, instead of current state on pages.



Possibly related important Sessionstore bugs: 
Bug 1298912
Bug 590448
Bug 845820
Bug 591957
Bug 1301041
Bug 1421673

Comment 49

[Mike de Boer [:mikedeboer]]

(In reply to Tim Nguyen :ntim from comment #46)
> I honestly think that the concept of file I/O shouldn't be introduced here,
> it's just inconsistent with every single WE API.

I'm the first to agree that my proposal isn't a prime example of brilliant API design ;-)

> How about just adding `browser.sessions.save(sessionId)`, then changing the
> existing browser.sessions.restore(sessionId) method to work with saved
> sessions IDs ?

Allow me to think out loud a bit:

```js
let sessionID = await browser.sessions.save(/*[outerWindowID]*/);
let savedSessions = await browser.sessions.getSavedList(); // 'getSaves()' sounded odd to me, somehow...
await browser.sessions.restore(sessionID);
```

Since we stringify the JSON before we flush it to disk, it should be cheap to modify sessionstore to return a (SHA1) hash of the saved session. If we were to use a different storage layer for sessionstore in the future, I'm predicting that it will support transactions and rollbacks, thus able to provide an ID as well.
The WebExtension implementation is in charge of updating and recording a metadata.json file which contains the list of saved sessions and their properties:

```json
{
  "1fe585dff008184d3ea03332c50b4fcceadf8b76": {
    date: 1517310809821,
    windows: 2,
    tabs: 42
  }
}
```

I don't know if we need more information TBH.

(In reply to swleefers from comment #47)
> Incidentally (I don't know whether this would be relevant), there are
> various other cases where users could benefit from file I/O, such as when a
> user wants to edit a userscript in his own favourite code editor, as opposed
> to some editor created by the add-on developer. Greasemonkey has all sorts
> of bugs open where people ask for a linter in the editor and other features.

Unfortunately, I/O is not some kind of generic concept that lends itself to be abstracted in this way. The storage layer for sessionstore just happens to be 'writing a physical file to disk' at the moment, but there's no guarantee that it will stay that way.
It's quite a stretch to extend the work here to encompass the needs for other type of extensions that use different APIs.

> Any file I/O should be restricted to the add-on's own special folder.

Why? This depends on the use case as well; you're defining a scoping rule that may work for editors, but not for browser sessions. If we want this sessionstore data to be available to all API consumers, then scoping writes to the extension folder would make that impossible.

Comment 50

[swleefers]

(In reply to Mike de Boer [:mikedeboer] from comment #49)
> (In reply to swleefers from comment #47)
> > Incidentally (I don't know whether this would be relevant), there are
> > various other cases where users could benefit from file I/O, such as when a
> > user wants to edit a userscript in his own favourite code editor, as opposed
> > to some editor created by the add-on developer. Greasemonkey has all sorts
> > of bugs open where people ask for a linter in the editor and other features.
> 
> Unfortunately, I/O is not some kind of generic concept that lends itself to
> be abstracted in this way. The storage layer for sessionstore just happens
> to be 'writing a physical file to disk' at the moment, but there's no
> guarantee that it will stay that way.
> It's quite a stretch to extend the work here to encompass the needs for
> other type of extensions that use different APIs.
> 
> > Any file I/O should be restricted to the add-on's own special folder.
> 
> Why? This depends on the use case as well; you're defining a scoping rule
> that may work for editors, but not for browser sessions. If we want this
> sessionstore data to be available to all API consumers, then scoping writes
> to the extension folder would make that impossible.

You're right: if the goal is for add-ons to handle sessions saved by other extensions or by Firefox itself, then file I/O would only make sense if they could all access the same folder.

I rather was thinking of an API for an add-on to retrieve (and set/append) the current session only; then the add-on can save it to the add-on's own folder through the file I/O API, after having retrieved it. Then other add-ons couldn't touch that session and whatever other data had been stored in that folder.

The reason why I imagined it this way was that it would seem simpler (less coding) for Firefox to implement? Firefox itself wouldn't need to be saving any session files. Having an add-on save and load a file from its own folder seems clean and safe to me (as a layman...), a bit like a 'file sandbox'—because I think the reason why file access was removed for WE was that it could be insecure? Unless I misunderstood.

And I think it could kill several birds with one stone, because this file I/O API would at the same time solve various other issues that other types of extensions have, like the editor thing, easy finding and copy-pasting of settings and data files from user profiles, etc. etc.

Comment 51

[Robert AB]

https://bugzilla.mozilla.org/show_bug.cgi?id=1381922#c17
and following comments.


About Bug 1381922:
> "This feature may or may not be needed, depending on how other parts of the sessionStore are exposed via the WebExtensions API. One idea is to create a simple save/load mechanism that extensions could use, rather than expose each session control point (like history) individually."

Comment 52

[Tim McCormack]

Opaque blobs are not enough.

Consider the following scenario, which I have personally experienced any numbers of times: I have a window with many tabs. One of the tabs causes a crash. I reopen Firefox, reload the session, and... of course, it crashes again!

The Session Manager extension had the very important feature of allowing me to uncheck certain tabs when restoring an extension. If you were to implement the API in terms of opaque, encrypted blobs, not only would this crash-loop prevent me from using the UI to restore my session, there's a very good chance I'd be unable to do it even with basic command-line tools!

I'm fine with an extension having the ability to see all my saved tabs, just like I'm fine with Firefox itself being able to. I don't know what technical impediments there are in WebExtensions around this, but you will never have a reliable session manager without the ability for the user to interact with the *contents* of a saved session.

Comment 53

[Tim McCormack]

(Apologies, posted this to the wrong bug.)

Comment 54

[Robert AB]

Shane - Thank you for working on session manager-related bugs, including Bug 1378647, which is crucial for many session manager developers.

Can you also look into Bug 1413525? There are some other bugs related to Bug 1413525: Bug 1378651, Bug 1381922, Bug 833791, Bug 833792.

Thanks!!

Comment 55

[Shane Caraveo (:mixedpuppy)]

Bug 1474130 might end up blocking much work here, but for now bug 1378651 and bug 1364019 are possibly reasonable to do.  There are other priorities in the way.

Comment 56

[Robert AB]

Bug 1378651 is no longer in preparation.
Does bug 1413525 need bug 1378651 to be solved first, on it can be prepared independently?

My understanding is that bug 1413525 is a complete solution and it is going to include the area covered by bug 1378651, bug 1381922, bug 833792, and much more. Am I correct?

Comment 57

[Shane Caraveo (:mixedpuppy)]

I think my comment 55 covered it.

Comment 58

[Worcester12345]

Can we get a version and/or a milestone/tracking/status on this bug?