601955 - Pinning/unpinning an app tab does not trigger saving of session state

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Michael Kraft [:morac]]

Steps to reproduce:

1. Make a tab into an app tab
2. Wait about 10 to 20th seconds
3. Crash the browser (or kill the task)
4. Start browser and choose to restore session

Actual Results:

1. Tab that was made into an app tab is still a regular tab.

Expected Results:

1. Tab that was made into an app tab should still be an app tab.


Other notes:

If "browser.sessionstore.resume_from_crash" is set to false, then the entire session is lost since the browser will only save app tabs.  The app tab won't be saved until some other change is made to the window that the app tab is in that triggers a save such as adding a tab, switching tabs, etc.

Comment 1

[Michael Kraft [:morac]]

I'll point out that this can also be reproduced by setting the browser to restore windows and tabs from last time, then opening two windows and adding some tabs to them.  Make one or more of the tabs in the first window into app tabs.  Then in the second window choose File->Exit.   Since the first window was never marked as "dirty", that window is never saved on shutdown so the app tabs are either lost or remain as normal tabs (depending on the resume_from_crash preference).

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Should be easy enough to fix. We have events for TabPinned and TabUnpinned.

Blocking? I'd block final or betaN, but not of the utmost urgency, though potentially annoying

Comment 3

[Dietrich Ayala (:dietrich)]

Marking blocking+ for the state-loss issue in comment #1.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Handing off to Mehdi given that he needs blockers and this one should be a pretty easy one to jump into.

Comment 5

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v1, adds events for tab (un)pinning along with a test.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 503986 [details] [diff] [review]
Patch v1, adds events for tab (un)pinning along with a test.

r=zpao so long as you address the comments for the test.

>+++ b/browser/components/sessionstore/test/browser/browser_601955.js

>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.

This should be Mozilla Foundation.

>+function test() {
>+  waitForExplicitFinish();
>+  originalSSInterval = Services.prefs.getIntPref("browser.sessionstore.interval");
>+  // We speed up the interval between session saves to ensure that the test
>+  // runs quickly.
>+  Services.prefs.setIntPref("browser.sessionstore.interval", 2000);
>+
>+  // Loading a tab causes a save state and this is meant to catch that event.
>+  waitForSaveState(testBug601955_1);
>+
>+  // Assumption: Only one window is open and it has one tab open.
>+  window.gBrowser.loadTabs(["about:mozilla"]);

Just one tab so you can use gBrowser.addTab(url).
Also, you can leave off "window." - we're in the window scope so gBrowser works by itself."

>+}
>+
>+function testBug601955_1() {

I know it's just a test, but I'd prefer if these have more meaningful names. Or keep the names and add a short comment about what each is doing.

>+  // Because pinned tabs are at the front of |gBrowser.tabs|, pinning tabs
>+  // re-arranges the |tabs| array.
>+  ok(!window.gBrowser.tabs[0].pinned, "first tab should not be pinned yet");
>+  ok(!window.gBrowser.tabs[1].pinned, "second tab should not be pinned yet");
>+
>+  waitForSaveState(testBug601955_2);
>+  window.gBrowser.pinTab(window.gBrowser.tabs[0]);
>+}
>+
>+function testBug601955_2() {
>+  let state = JSON.parse(ss.getBrowserState());
>+  ok(state.windows[0].tabs[0].pinned, "first tab should be pinned by now");
>+  ok(!state.windows[0].tabs[1].pinned, "second tab should still not be pinned");
>+
>+  waitForSaveState(testBug601955_3);
>+  // This causes the second tab to be moved to the first spot.
>+  window.gBrowser.unpinTab(window.gBrowser.tabs[0]);
>+  window.gBrowser.pinTab(window.gBrowser.tabs[1]);

Let's just unpin. If something were to change, we wouldn't know if pinning or unpinning is causing the save state. We don't really care that together something will happen since the same thing should be happening for each individually.

>+}
>+
>+function testBug601955_3() {
>+  let state = JSON.parse(ss.getBrowserState());
>+  ok(state.windows[0].tabs[0].pinned, "first tab should be pinned");
>+  ok(!state.windows[0].tabs[1].pinned, "second tab should not be pinned");
>+
>+  waitForSaveState(testBug601955_4);
>+  window.gBrowser.unpinTab(window.gBrowser.tabs[0]);

Then we can skip unpinning here and don't need test_4.

>+}
>+
>+function testBug601955_4() {
>+  let state = JSON.parse(ss.getBrowserState());
>+  ok(!state.windows[0].tabs[0].pinned && !state.windows[0].tabs[1].pinned,
>+     "both tabs should now be unpinned");
>+
>+  // Just to clean up, swap the tab order again.
>+  window.gBrowser.moveTabTo(window.gBrowser.tabs[0], 1);

Don't need to do this so long as you remove the right tab.

>+
>+  done();
>+}
>+
>+function done() {
>+  for (let i = 1; i < window.gBrowser.tabs.length; i++)
>+    window.gBrowser.removeTab(window.gBrowser.tabs[i]);

You know you only added 1 tab, so just remove it directly. If it makes it easier, assign it in test() when opening the tab.

>+
>+  Services.prefs.setIntPref("browser.sessionstore.interval", originalSSInterval);

Please use clearUserPref here instead of tracking the original value.


>+++ b/browser/components/sessionstore/test/browser/head.js
>@@ -90,6 +90,16 @@
>   SS_SVC.setBrowserState(JSON.stringify(aState));
> }
> 
>+// |waitForSaveState| literally waits for a state write but not necessarily for
>+// the state to turn dirty.

Please remove the enclosing || and "literally"

Comment 7

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v2, fixes nits from zpao.

Talked to zpao in person, he didn't mind not having comments since each callback is pretty simple.

Pushed to try yesterday, was clean. Pushed to m-c.
http://hg.mozilla.org/mozilla-central/rev/3cd214ffaf95

Comment 8

[u279076]

Verified using Minefield nightly from 2011-01-22.