1478601 - sessions.getRecentlyClosed returns objects with window[*].tabs[*].sessionId set to the "undefined" string

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Rob Wu [:robwu]]

Steps to reproduce:
1. Create a directory with the following files
> manifest.json
{
    "name": "Sessions extension",
    "version": "1",
    "manifest_version": 2,
    "background": {
        "scripts": ["background.js"]
    },
    "browser_action": {
        "default_title": ""
    },
    "permissions": [
        "sessions",
        "tabs"
    ]
}

> background.js
chrome.browserAction.onClicked.addListener(function() {
    chrome.sessions.getRecentlyClosed(console.log);
});

2. Start Firefox and load this extension (e.g. via about:debugging).
3. Open a new window (e.g. with Ctrl - N).
4. Open example.com and example.net in two new tabs in this window.
5. Click on the extension button.
6. Open the global JS console (Ctrl-Shift-J) and expand the printed object.

Actual result:
(1) […]
  0: {…}
    lastModified: 1532598933309
​​    window: {…}
    alwaysOnTop: false
    focused: false
    incognito: false
    sessionId: "0"
    state: "normal"
    tabs: (1) […]
      0: Object { sessionId: "undefined", index: 0, highlighted: false, … }
​​​​
Expected result:
The last line shows sessionId: "undefined". I expected sessionId: "1" or something, so that an individual tab can be restored.

Tested with Firefox 61.0.1
I also tested the extension in Chrome (68), and that has the expected result.

Comment 1

[Andrew Swan [:aswan]]

Rob, can you look more closely at what's going on?

Comment 2

[Shane Caraveo (:mixedpuppy)]

I also am looking at another issue with this api, want to keep tabs.

Comment 3

[Rob Wu [:robwu]]

As for why the value is as it is, see below.

In short, we don't store a separate sessionId for tabs of closed windows.
Chrome does have a unique session ID for each tab, but when you restore an individual tab of a closed window, the whole window is shown.

Test case (extends comment 1)
7. Expand the output of the chrome.sessions.getRecentlyClosed call, and find any sessionId of window[*].tabs[*]
8. Run chrome.sessions.restore("sessionID from step 7")
Result in Chrome: One window is opened, containing only that tab.
Result in Firefox: "undefined" is not a valid sessionId/closedId, so error.

There are multiple ways to solve this bug:
- Somehow "fix" SessionStore.jsm to have a unique "closedId" for the tab (meh)
- Re-use the sessionId of the window in all tabs.
- Don't set sessionId on tabs in the sessions API if "closedId" is not a string, and document this difference on MDN.

I think that we should pick one of the last two.
Opinions?

----

It is a string because we use String(tabData.closedId):
https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/browser/components/extensions/parent/ext-browser.js#777

The value comes from
https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/browser/components/extensions/parent/ext-browser.js#996,1007
and originates here, when a window is closed:
https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/browser/components/extensions/parent/ext-sessions.js#25

The following isolated snippet (run in the global/browser console (after enabling chrome debugging) shows that sessionstore does not have separate IDs for tabs.

console.log(JSON.stringify(SessionStore.getClosedWindowData(false)[0], null, 2));
{
  "tabs": [
    {
      "entries": [
        {
          "url": "about:home",
          "title": "New Tab",
          "cacheKey": 0,
          "ID": 2,
          "docshellUUID": "{d773228e-fecd-4bc7-9d0e-151c4f86d37c}",
          "resultPrincipalURI": null,
          "triggeringPrincipal_base64": "SmIS26zLEdO3ZQBgsLbOywAAAAAAAAAAwAAAAAAAAEY=",
          "docIdentifier": 2,
          "persist": true
        },
        {
          "url": "http://example.com/",
          "title": "Example Domain",
          "cacheKey": 0,
          "ID": 3,
          "docshellUUID": "{d773228e-fecd-4bc7-9d0e-151c4f86d37c}",
          "originalURI": "http://example.com/",
          "resultPrincipalURI": null,
          "principalToInherit_base64": "vQZuXxRvRHKDMXv9BbHtkAAAAAAAAAAAwAAAAAAAAEYAAAA4bW96LW51bGxwcmluY2lwYWw6ezQ0Mjc5MDM1LTc0NDMtNGIxYi1iMzY2LWFiMWM1Y2M5ZmE2NH0AAAAA",
          "triggeringPrincipal_base64": "SmIS26zLEdO3ZQBgsLbOywAAAAAAAAAAwAAAAAAAAEY=",
          "docIdentifier": 3,
          "persist": true
        }
      ],
      "lastAccessed": 1533293181010,
      "hidden": false,
      "mediaBlocked": true,
      "attributes": {},
      "userContextId": 0,
      "index": 2,
      "image": null,
      "iconLoadingPrincipal": null
    }
  ],
  "selected": 1,
  "_closedTabs": [],
  "width": 1280,
  "height": 945,
  "screenX": -4,
  "screenY": 0,
  "sizemode": "normal",
  "zIndex": 1,
  "title": "Example Domain",
  "closedAt": 1533293181012,
  "closedId": 1
}

Comment 4

[Shane Caraveo (:mixedpuppy)]

Trying to reuse the same id on the tabs wont work due to how undoCloseById works.

Adding closedId to the tabs is not a big deal, just iterate the tabs and add it when closing the window[1], but we'd also need to fix undoCloseById[2].  We could move all the [other] tabs to _closedTabs then restore the window.  So something like:

  undoCloseById(aClosedId) {
    // Check for a window first.
    for (let i = 0, l = this._closedWindows.length; i < l; i++) {
      if (this._closedWindows[i].closedId == aClosedId) {
        return this.undoCloseWindow(i);
      }
      // look for the closedId in the windows tabs
      if (this._closedWindows[i].tabs.length) {
         let closedWindow = this._closedWindows[i];
         for (let j = 0; j < closedWindow.tabs.length; j++) {
           if (closedWindow.tabs[j].closedId == aClosedId) {
              let tabs = [closedWindow.tabs[j]];
              closedWindow._closedTabs = closedWindow.tabs.splice(j, 1);
              closedWindow.tabs = tabs;
              return this.undoCloseWindow(i);
           }
         }
      }
    }
    ...

If we leave closedId as undefined, and there is an attempt to restore that tab, we'd probably need to restore the entire window, which may not be what we really want to do.  So leaving it translates to making sure that those tabs are not in the session objects returned by the sessions api.  The extension would have to open the entire window (thus all the tabs along with it).


See if @mikedeboer has an opinion.

[1] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/browser/components/sessionstore/SessionStore.jsm#1575
[2] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/browser/components/sessionstore/SessionStore.jsm#2714-2720

Comment 5

[Mike de Boer [:mikedeboer]]

Shane, I think your suggestion is a-ok :)

Comment 6

[Rob Wu [:robwu]]

This bug doesn't have much impact; extensions who care can check whether sessionId is "undefined".

Unassigning myself to free time for other stuff.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Rob Wu [:robwu] from comment #6)
> This bug doesn't have much impact; extensions who care can check whether
> sessionId is "undefined".

That isn't the solution.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> (In reply to Rob Wu [:robwu] from comment #6)
> > This bug doesn't have much impact; extensions who care can check whether
> > sessionId is "undefined".
> 
> That isn't the solution.

Sorry, mixed this up with another bug on the same api.

Comment 9

[juraj.masiar]

Why is sessionId: "undefined", this is clearly a bug - a wrong serialization of "undefined" value.

Even if we can't make the same as Chrome, let's at least make it undefined instead of "undefined". It is a breaking change for addon developers but it's the right thing to do.