1364483 - Customize Mode tab doesn't get restored anymore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Dão Gottwald [:dao]]

Probably another regression from bug 1306294?

Comment 1

[Matheus Longaray (:mlongaray)]

Not sure if this is intended behavior or after-the-fact regression. mikedeboer should be the best call to evaluate here.

What I did do was to debug what/how customize tab looks like in our session state. Here's what I dumped.
{
 "entries": [],
 "lastAccessed": 1494608959714
 "hidden": false,
 "attributes": {"customizemode":"true"},
 "image": "chrome://browser/skin/customizableui/customizeFavicon.ico",
 "iconLoadingPrincipal":"vQZuXxRvRHKDMXv9BbHtkAAAAAAAAAAAwAAAAAAAAEYAAAA4bW96LW51bGxwcmluY2lwYWw6ezQwZTAzNjJjLTcwODEtNDNmMi04ZWY5LWJhNTY4ODQ5NmMzZn0AAAAA"
}

Our current state saving behavior is (what came out from bug 1306294, bug 1323987 and bug 1343056):
  /**
   * Determine if the tab state we're passed is something we should keep to be
   * reopened at session restore. This is used when we are saving the current
   * session state to disk. This method is very similar to _shouldSaveTabState,
   * however, "about:blank" and "about:newtab" tabs will still be saved to disk.
   *
   * @param aTabState
   *        The current tab state
   * @returns boolean
   */
  _shouldSaveTab: function ssi_shouldSaveTab(aTabState) {
    // If the tab has one of the following transient about: history entry,
    // then we don't actually want to write this tab's data to disk.
    return aTabState.userTypedValue ||
           (aTabState.entries.length &&
            !(aTabState.entries[0].url == "about:printpreview" ||
              aTabState.entries[0].url == "about:privatebrowsing"));
  },

Customize mode tab does not have both userTypedValue and 'entries' properties registered to its state.

What do you think, Mike?

Comment 2

[Dão Gottwald [:dao]]

(In reply to Matheus Longaray (:mlongaray) from comment #1)
> Not sure if this is intended behavior or after-the-fact regression.

The intended behavior is to have this restored. From a user's perspective there's no particular reason why this tab should be excluded. SessionStore.jsm even has code to register that tab (if it were saved correctly) with CustomizeMode.jsm.

Comment 3

[Matheus Longaray (:mlongaray)]

Yes, exactly.

Comment 4

[Matheus Longaray (:mlongaray)]

What we could simply do is to also look for customizemode attribute when saving session state. Let's hear more from Mike.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Matheus Longaray (:mlongaray) from comment #4)
> What we could simply do is to also look for customizemode attribute when
> saving session state. Let's hear more from Mike.

This is the best way forward, for sure. Are you willing to write that patch, Matheus?

Comment 6

[Matheus Longaray (:mlongaray)]

Yes, let's do it. Patch is on the way.

Comment 7

[Matheus Longaray (:mlongaray)]

Attachment: Bug 1364483 - Take customizemode attribute into account when saving tabs to disk.

This patch adds customizemode attribute data validation when saving tabs to disk.

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

Comment 8

[Matheus Longaray (:mlongaray)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2036a7ac0940824ba8ccbe21a5174add7c0f43a&group_state=expanded

Comment 9

[Matheus Longaray (:mlongaray)]

Try run seems good.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8867780 [details]
Bug 1364483 - Take customizemode attribute into account when saving tabs to disk.

https://reviewboard.mozilla.org/r/139314/#review143464

Good stuff. It works for me, so let's ship it!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/199f8a2b3f08
Take customizemode attribute into account when saving tabs to disk. r=mikedeboer

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/199f8a2b3f08

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 14

[Matheus Longaray (:mlongaray)]

Comment on attachment 8867780 [details]
Bug 1364483 - Take customizemode attribute into account when saving tabs to disk.

Approval Request Comment

[Feature/Bug causing the regression]: Bug 1306294.

[User impact if declined]: Data loss when restoring session.

[Is this code covered by automated tests?]: SessionStore itself has a substantial automated test suite, but unfortunately, that suite did not catch the regression that this patch fixes.

[Has the fix been verified in Nightly?]: No.

[Needs manual test from QE? If yes, steps to reproduce]: No.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: I do not think so.

[Why is the change risky/not risky?]: One-liner patch that adds one specific attribute data validation in order to restore old session store working behavior as much as possible.

[String changes made/needed]: None.

Comment 15

[Gerry Chang [:gchang]]

Hi Emil,
Can you help check if this issue is fixed in the latest nightly?

Comment 16

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

I have re-tested this issue on Windows 10 x64 and Mac OS 10.12 with the latest Nightly (55.0a1-20170521030205) and managed to reproduce it, more than that it seems that it has introduced a new issue as described below:

[Notes]:
- When having multiple tabs and an unfocused customized nightly tab, when you close the browser and reopen it(or restarted from the console), the previously opened tabs are not successfully restored, displaying blank tabs instead.

[Prerequisites]:
- Have a clean Firefox profile with the "When Nightly starts" option set to "Show your windows and tabs from last time".

[Steps to reproduce]:
1. Open the browser with the profile from prerequisites and navigate to a website.
2. Open the Firefox Menu and click on the "Customize" button.
3. After the Customize tab is loaded, switch to the other tab(CTRL+TAB).
4. Close the browser and reopen it with the same profile.
5. Observe the opened tabs.

[Expected result]:
- The previously opened tabs are successfully restored.

[Actual result]:
- The browser only displays blank tabs.

[Additional Notes]:
- After reopening, the blank tabs are working normally.
- Attached a screen recording of the issue: https://goo.gl/iVWNTW
- I have also performed a regression, here is the result:
Last good revision: c27dda89b4eb396f641885fa0beaf93ad61eaa70
First bad revision: 199f8a2b3f08183cc92fe57ec5cacac30a9d9f54
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c27dda89b4eb396f641885fa0beaf93ad61eaa70&tochange=199f8a2b3f08183cc92fe57ec5cacac30a9d9f54

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1364483

Comment 17

[Matheus Longaray (:mlongaray)]

Wow - thanks for finding this, Emil! I'll try to reproduce it locally so we can capture what's going on with session store data.

Comment 18

[Matheus Longaray (:mlongaray)]

Following is how session store data looks like after saving two tabs to disk (CNN webpage & Customize tab):

{
  "version": [
    "sessionrestore", 1
  ],
  "windows": [
    {
      "tabs": [
        {
          "entries": [
            {
              "url": "http://edition.cnn.com/",
              "title": "CNN - Breaking News, U.S., World, Weather, Entertainment & Video News",
              "charset": "UTF-8",
              "ID": 25,
              ...
            }
          ],
          "lastAccessed": 1495464630247,
          "hidden": false,
          "attributes": {},
          ...
        },
        {
          "entries": [],
          "lastAccessed": 1495464626815,
          "hidden": false,
          "attributes": {
            "customizemode": "true"
          },
          ...
        }
      ],
      "selected": 1,
      "_closedTabs": [],
      ...
    }
  ],
  "selectedWindow": 1,
  "_closedWindows": [],
  "session": "{...}"
}

Here's how session store data looks like after restarting the browser:

{
  "version": [
    "sessionrestore",
    1
  ],
  "windows": [
    {
      "tabs": [
        {
          "entries": [
            {
              "url": "about:blank",
              "title": "about:blank",
              "charset": "",
              "ID": 0,
              ...
            }
          ],
          "lastAccessed": 1495464718799,
          "hidden": false,
          "attributes": {},
          ...
        }
      ],
      "selected": 1,
      "_closedTabs": [
        {
          "state": {
            "entries": [
              {
                "url": "about:sessionrestore",
                "title": "Restore Session",
                "charset": "",
                "ID": 1,
                ...
              }
            ],
            "lastAccessed": 1495464707438,
            "hidden": false,
            "attributes": {},
            ...
            "formdata": {
              "id": {
                "sessionData": {
                  "version": [
                    "sessionrestore",
                    1
                  ],
                  "windows": [
                    {
                      "tabs": [
                        {
                          "entries": [
                            {
                              "url": "http://edition.cnn.com/",
                              "title": "CNN - Breaking News, U.S., World, Weather, Entertainment & Video News",
                              "charset": "UTF-8",
                              "ID": 25,
                             ...
                            }
                          ],
                          "lastAccessed": 1495464630247,
                          "hidden": false,
                          "attributes": {},
                          ...
                        },
                        {
                          "entries": [],
                          "lastAccessed": 1495464626815,
                          "hidden": false,
                          "attributes": {
                            "customizemode": "true"
                          },
                          ...
                        }
                      ],
                      "selected": 1,
                      "_closedTabs": [],
                      ...
                    }
                  ],
                  "selectedWindow": 1,
                  "_closedWindows": [],
                  "session": "{...}",
                }
              },
              "url": "about:sessionrestore"
            },
            ...
          },
          "title": "Restore Session",
          ...
        }
      ],
      ...
    }
  ],
  "selectedWindow": 1,
  "_closedWindows": [],
  "session": "{...}",
  ...
}

Looks like the session data (prior to restarting the browser) ended up inside "formdata" attribute within the _closedTabs array (which contains an about:sessionrestore entry indeed). It seems that we are failing when restoring the session.

I'll try to dig a bit more to understand how this patch could affect session restore flow.

Comment 19

[Matheus Longaray (:mlongaray)]

Okay, found some exception going on when I opened Browser Console.

TypeError: tabData.entries[activeIndex] is undefined

NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "tabData.entries[activeIndex] is undefined" {file: "resource:///modules/sessionstore/SessionStore.jsm" line: 3292}]'[JavaScript Error: "tabData.entries[activeIndex] is undefined" {file: "resource:///modules/sessionstore/SessionStore.jsm" line: 3292}]' when calling method: [nsISessionStore::setWindowState]  aboutSessionRestore.js:150

It looks like bug 1360932 introduced this issue (comment #16) and a patch is on the way in order to fix it (see bug 1365933).

Hey Mike, what do you think?

Comment 20

[Mike de Boer [:mikedeboer]]

Yeah, when that's resolved, let's ask Emil if he can still reproduce this bug.

Comment 21

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

I have re-tested this issue on Windows 10 x64, Mac OS 10.12 and Ubuntu 12.04 x64 with the latest Nightly (55.0a1-20170523030206) and I can confirm that this is no longer reproducible.

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

I guess we can close this again.

Comment 23

[Gerry Chang [:gchang]]

Comment on attachment 8867780 [details]
Bug 1364483 - Take customizemode attribute into account when saving tabs to disk.

Fix a regression related to session restore. Beta54+. Should be in 54 beta 11.

Hi Emil,
Can you help verify again when this lands in beta?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1647074d9dd4

Comment 25

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
	
I have re-tested this issue on Windows 10 x64, Mac OS 10.12 and Ubuntu 14.04 x64 with the latest Beta (54.0b12-20170529025116) and I can confirm that this is not reproducible.