580512 - App tabs should outlive normal sessions

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Jesse Ruderman]

Steps:
1. Turn a tab into an app tab
2. Restart Firefox

Result:
* Blake has session restore enabled.  His app tabs turn into normal tabs.
* I have session restore disabled.  My app tabs are just forgotten.

Expected:
* Blake should have his app tab restored using session store.
* I should have my app tab restored from its URL.

Comment 2

[Jesse Ruderman]

It's still not working for me.

Comment 3

[Dão Gottwald [:dao]]

We need more precise STR then. I assume you're running a recent nightly...

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> I assume you're running a recent nightly...

Also, without add-ons which could break this?

Comment 5

[Jesse Ruderman]

App tabs are only remembered if I enable session restore.  I expect them to be remembered (URL only) even if I have session restore disabled, and to not be counted toward the session restore prompt (which by default appears if you have 2+ tabs open).

STR:
1. Launch Firefox with a new profile.
2. Make sure only one tab is shown.
3. Right-click on the tab and select "Make into App Tab".
4. Quit using Cmd+Q.
5. Relaunch Firefox.

Result: app tab is gone.

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

I just confirmed Jesse's steps (and kind of wish I hadn't, since now I have to recreate all my app tabs)

Comment 7

[juan becerra [:juanb]]

Adding whiteboard keyword to get it into triage list.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

It seems like we erred when we made App Tabs part of Session Restore; they shouldn't be considered part of a user's browsing session, they're a user's declared applications and should always be restored.

This blocks a future beta for testing.

Comment 12

[nikem2007-email]

On OSX 10.5.8 and Firefox 4.0b2 even when I simply close the window, without quitting the application, when I reopen a new window the app tabs disappear

Comment 13

[Keng Susumpow]

I do agree with Jesse that an app tab should be restored once it's app-tabbed.

But, would this be a paradigm-conflict between an app tab and a bookmark?

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

--> Dao, but not sure if there's a better App Tab owner. Maybe fryn can help?

Comment 18

[Dão Gottwald [:dao]]

Attachment: wip

this doesn't open the home page when restoring app tabs -- I don't know yet which code prevents this

Comment 19

[Dão Gottwald [:dao]]

Attachment: patch v1

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 21

[Dão Gottwald [:dao]]

Attachment: patch with test

Comment 22

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

Comment on attachment 464190 [details] [diff] [review]
patch with test

>-  saveState: function sss_saveState(aUpdateAll) {
>+  saveState: function sss_saveState(aUpdateAll, aPinnedOnly) {
>     // if crash recovery is disabled, only save session resuming information
>     if (!this._resume_from_crash && this._loadState == STATE_RUNNING)
>       return;
> 
>     // if we're in private browsing mode, do nothing
>     if (this._inPrivateBrowsing)
>       return;
> 
>-    var oState = this._getCurrentState(aUpdateAll);
>+    var oState = this._getCurrentState(aUpdateAll, aPinnedOnly);
>+    if (!oState)
>+      return;
>+
>+    if (aPinnedOnly)
>+      this._prefBranch.setBoolPref("sessionstore.resume_session_once", true);
>+
>     oState.session = {
>       state: this._loadState == STATE_RUNNING ? STATE_RUNNING_STR : STATE_STOPPED_STR,
>       lastUpdate: Date.now()
>     };
>     if (this._recentCrashes)
>       oState.session.recentCrashes = this._recentCrashes;
> 
>     this._saveStateObject(oState);

At the very least, this isn't going to save app tabs if you crash with browser.sessionstore.resume_from_crash set to false. I would still expect those to persist.

This also doesn't solve the problem with app tabs not being restored after closing the window on OS X. I'd be ok fixing that one in a followup though.

As an aside, I'm not a huge fan of app tabs being tacked onto session restore. I guess it'll stay, but it seems as though it wasn't planned out too well and is requiring more underlying changes to session restore than was probably assumed. Doing it in session restore has its advantages too though... Not much to do about it now.

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> At the very least, this isn't going to save app tabs if you crash with
> browser.sessionstore.resume_from_crash set to false. I would still expect those
> to persist.

Not sure why we'd draw a line here if the user wants his data blown away. I guess I don't see the point of the pref... Does it go back to the days before we had about:sessionrestore?

Comment 24

[Dietrich Ayala (:dietrich)]

> As an aside, I'm not a huge fan of app tabs being tacked onto session restore.
> I guess it'll stay, but it seems as though it wasn't planned out too well and
> is requiring more underlying changes to session restore than was probably
> assumed. Doing it in session restore has its advantages too though... Not much
> to do about it now.

Where else makes more sense? They are tabs, albeit with special properties in how they're displayed in chrome, and are treated as such in all the tab-related APIs. Their content needs to be restored like any other tab's content: form data, storage, whatnot, which is SS's primary job.

Comment 25

[Dietrich Ayala (:dietrich)]

(In reply to comment #23)
> (In reply to comment #22)
> > At the very least, this isn't going to save app tabs if you crash with
> > browser.sessionstore.resume_from_crash set to false. I would still expect those
> > to persist.
> 
> Not sure why we'd draw a line here if the user wants his data blown away. I
> guess I don't see the point of the pref... Does it go back to the days before
> we had about:sessionrestore?

Requesting UX input on this.

Comment 26

[Michael Lefevre]

(In reply to comment #24)
> Their content needs to be restored like any other tab's content: form
> data, storage, whatnot, which is SS's primary job.

I had kind of assumed they would work that way, but, FWIW, that's probably going to put me off using them. If I'm not restoring my session, I'd quite like a clean start. If I've got Gmail or Facebook or Twitter or whatever in an app tab, I'd be happy to see it load up the latest content on my "home page" on that site when I click on it, rather than reloading the state of my news feed or inbox or whatever other page on that "app" site I had moved to. Not to mention taking up memory and CPU time with something I may not even look at in this session.

Anyway, my individual view doesn't matter, my point is that it shouldn't be an assumption that app tabs have to preserve their exact state between sessions - actual desktop apps usually don't.

Comment 27

[Paul [pwd]]

Would it not just have made more sense to duplicate the session restore and use a tweaked version for the App Tabs. To simply tack them onto the existing session restore seems a bit ****.

Comment 28

[Dietrich Ayala (:dietrich)]

(In reply to comment #27)
> Would it not just have made more sense to duplicate the session restore and use
> a tweaked version for the App Tabs. To simply tack them onto the existing
> session restore seems a bit half-arsed.

Why would that have made more sense?

It would've entailed duplication of code, more constantly-running XPCOM services, and more XPCOM cruft at startup.

And please try to contribute to the conversation without verbally deriding the work of others. The guy who wrote the patch isn't stupid and wasn't drunk (I hope!). Sometimes it takes >1 try to get things right.

(In reply to comment #26)
> Anyway, my individual view doesn't matter, my point is that it shouldn't be an
> assumption that app tabs have to preserve their exact state between sessions -
> actual desktop apps usually don't.

True. I'm not 100% about restoring full state for app tabs. But these are web apps like Gmail, or Wordpress, where you might have a half-written document that you want restored, so I'm advocating the route of zero dataloss for now, until we get some more input from the UX team about how they envisioned app tabs to work.

However, this conversation is straying from the original bug. File a new bug if you don't want app tabs' state restored.

Comment 29

[Paul [pwd]]

(In reply to comment #28)
> (In reply to comment #27)
> > Would it not just have made more sense to duplicate the session restore and use
> > a tweaked version for the App Tabs. To simply tack them onto the existing
> > session restore seems a bit ****.
> 
> Why would that have made more sense?
> 
> It would've entailed duplication of code, more constantly-running XPCOM
> services, and more XPCOM cruft at startup.
> 
> And please try to contribute to the conversation without verbally deriding the
> work of others. The guy who wrote the patch isn't stupid and wasn't drunk (I
> hope!). Sometimes it takes >1 try to get things right.
> 

First of all, my apologies to Dao. It was disrespectful and that wasn't my intention.

Back on topic I think it's as simple a question as "Can App Tabs outgrow the usage of the session storage" and the answer is yes, so why not lay the foundations for that potential growth now? That way, should thinks go the other way, it's as easy to disconnect the App Tabs logic.

Comment 30

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

(In reply to comment #23)
> (In reply to comment #22)
> > At the very least, this isn't going to save app tabs if you crash with
> > browser.sessionstore.resume_from_crash set to false. I would still expect those
> > to persist.
> 
> Not sure why we'd draw a line here if the user wants his data blown away. I
> guess I don't see the point of the pref... Does it go back to the days before
> we had about:sessionrestore?

I don't know the exact origin of that pref but it indicates that the user doesn't want their session restored if the browser crashes. So if app tabs are truly independent from sessions, then the app tabs should *always* be restored.

(In reply to comment #24)
> > As an aside, I'm not a huge fan of app tabs being tacked onto session restore.
> > I guess it'll stay, but it seems as though it wasn't planned out too well and
> > is requiring more underlying changes to session restore than was probably
> > assumed. Doing it in session restore has its advantages too though... Not much
> > to do about it now.
> 
> Where else makes more sense? They are tabs, albeit with special properties in
> how they're displayed in chrome, and are treated as such in all the tab-related
> APIs. Their content needs to be restored like any other tab's content: form
> data, storage, whatnot, which is SS's primary job.

I realize that it fits best here. And I agree that we want the data, history, etc restored. It just seems like we might be chasing this down the rabbit hole. This is not the last thing that's going need to change in session restore. If that's what we need to do, then we need to do it. I didn't mean to derail this conversation, I just wanted to voice my concern.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

answer for ui-wanted request: The value of browser.sessionrestore.resume_from_crash shouldn't have an effect on restoring app tabs. Making a page an "app tab" should be thought of like installing an application on your (web) desktop. It's a permanent installation action. To undo it, you should need to take an explicit uninstall action.

Any chance this could be made ready for beta 4? Not sure how close this patch is given all the comments.

Comment 32

[Dão Gottwald [:dao]]

Attachment: patch v3

Could somebody please run sessionstore tests with this on OS X? Don't want to stress the tryserver too much. An OS X tryserver run with an older checkpoint had a failure in browser_394759.js and a timeout in browser_580512.js. I think I've got the latter covered. Not sure what was going on with the former.

All tests pass for me locally on Linux.

Comment 33

[Johnathan Nightingale [:johnath]]

Run OSX only?

http://blog.mozilla.com/cjones/2010/08/10/filter-your-tryserver-builds-a-bit-more-easily/

Comment 34

[Dão Gottwald [:dao]]

(In reply to comment #33)
> Run OSX only?

Yes, I did that, still eats resources and takes half a day to complete though.

Comment 35

[Dietrich Ayala (:dietrich)]

Comment on attachment 464803 [details] [diff] [review]
patch v3

>+    if (aPinnedOnly) {
>+      // fold pinned tabs from all windows into a single window
>+      let tabArrays = total.map(function (w) w.tabs.filter(function (t) t.pinned));
>+      total[0].tabs = tabArrays.reduce(function (a, b) a.concat(b));
>+      total.length = 1;
>+      if (total[0].tabs.length == 0)
>+        return null;
>+
>+      ix = -1;
>+      lastClosedWindowsCopy = [];
>+    } else {

This doesn't seem right. As a user, I'd expect the same number of windows to be restored as closed. In this app-tabs-aren't-mirrored world, we can definitely expect users to have task-specific windows with app tabs that are particular to those tasks on them. (Note: I think we might be shifting to app-tab mirroring across windows.)

The other session restore changes look ok though. Please get a browser peer to review the changes outside of SS.

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #35)
> This doesn't seem right. As a user, I'd expect the same number of windows to be
> restored as closed.

That's true, definitely.

> In this app-tabs-aren't-mirrored world, we can definitely
> expect users to have task-specific windows with app tabs that are particular to
> those tasks on them. (Note: I think we might be shifting to app-tab mirroring
> across windows.)

I believe that's the case, indeed. I agree that until that change is made, your comment here is valid.

Comment 37

[Dão Gottwald [:dao]]

Attachment: patch v4

Addressed comment 38. Requesting review from vlad for non-sessionstore bits.

Comment 38

[Dietrich Ayala (:dietrich)]

(In reply to comment #37)
> 
> Addressed comment 38.

That's fantastic, thanks. I expect my account should show the €1,000,000 any moment then?

Comment 39

[Dão Gottwald [:dao]]

I trusted you, carelessly. Won't do it again.

Comment 40

[Dietrich Ayala (:dietrich)]

Comment on attachment 465317 [details] [diff] [review]
patch v4

>-#ifndef XP_MACOSX
>     case "browser-lastwindow-close-granted":
>       // last browser window is quitting.
>       // remember to restore the last window when another browser window is openend
>       // do not account for pref(resume_session_once) at this point, as it might be
>       // set by another observer getting this notice after us

while you're here, can you fix the typo s/openend/opened/?

>         delete state.hidden;
>-        state = { windows: [state] };
>-        this._restoreCount = 1;
>-        this.restoreWindow(aWindow, state, this._isCmdLineEmpty(aWindow));
>+#ifndef XP_MACOSX
>+        if (!this._doResumeSession())
>+#endif
>+          state.tabs = state.tabs && state.tabs.filter(function (tab) tab.pinned);
>+        if (state.tabs && state.tabs.length > 0) {
>+          this._restoreCount = 1;
>+          state = { windows: [state] };
>+          this.restoreWindow(aWindow, state, this._isCmdLineEmpty(aWindow, state));
>+        }

state.tabs being either false or tabs is kind of wonky - maybe just make it an empty array?

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

We should get this for beta4, prioritizing based on user feedback!

Comment 42

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/3137ecdfdb60

Comment 43

[Dão Gottwald [:dao]]

Backed out for bug 587299, and relanded:

http://hg.mozilla.org/mozilla-central/rev/9fd65dc00474

Comment 44

[Cameron Dawson [:camd]]

Added to Litmus:
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=12816

Comment 45

[Michael Kraft [:morac]]

This change is causing a pretty major breakage of the SessionStore API when crash recovery is disabled since non-pinned tabs are getting removed during API calls.  See bug 600545 and bug 601161.  It also breaks the reliability of the restore closed window functionality in certain instances as well (bug 589246).