1228593 - Opened tabs are not restored after quitting Fennec with 'Clear on exit' and 'Always restore tabs' options enabled

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Flaviu Cos]

Environment: 
Device: Nexus 5 (Android 6.0);
Build: Nightly 45.0a1 (2015-11-26);

Steps to reproduce:
1. Go to Setting and enable 'Always restore tabs' option;
2. Go to Settings -> Privacy and enable 'Clear private data on exit';
3. Open multiple pages in different tabs;
4. Tap Options -> Quit;
5. Launch Fennec.

Expected result:
The tabs opened at step 3 are not cleared and are successfully restored.

Actual result:
The tabs opened at step 3 are not restored; Only about:home tab is launched.

Comment 1

[u421692]

This seems to be an old regression:
Firefox 39 release unaffected ;  Firefox 40 release affected
Pushlog:
https://hg.mozilla.org/releases/mozilla-release/pushloghtml?fromchange=FENNEC_39_0_BUILD1&tochange=FENNEC_40_0_BUILD1
Possible regressions: Bug 1118818, Bug 1154952, Bug 1152308

Comment 2

[:Margaret Leibovic]

This is working as designed. If you want to clear your history, we can't restore your tabs.

Comment 3

[u421692]

Bug description is incomplete.
2. Go to Settings -> Privacy and enable 'Clear private data on exit';
Enable only data that does not affect history (e.g. "Downloads")
"Browsing history" is NOT checked

Comment 4

[Jim Mathies [:jimm]]

Can we close this out or should this track a release?

Comment 5

[:Margaret Leibovic]

We could address this in Fx48 as part of fixing bug 1219343.

ahunt, this isn't high priority, but it would be good to address this. 

I'm worried this may be hairy because it deals with session restore, but hopefully it's just a simple mistake.

Comment 6

[Andrzej Hunt :ahunt]

Heh, I'm actually getting even worse behaviour: I was using an existing profile which restored 5 tabs, then did the configuration as inc comment 1 (I added clear "Downloads" only on exit, restore all tabs was already enabled), closed some tabs (2 tabs remaining), select quit -> All 5 tabs were restored, not the 2 tabs which I had when I quit.

I'm guessing we're not updating the list of tabs at all when we quit (as opposed to clearing the list).

Comment 7

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #6)
> Heh, I'm actually getting even worse behaviour: I was using an existing
> profile which restored 5 tabs, then did the configuration as inc comment 1
> (I added clear "Downloads" only on exit, restore all tabs was already
> enabled), closed some tabs (2 tabs remaining), select quit -> All 5 tabs
> were restored, not the 2 tabs which I had when I quit.
> 
> I'm guessing we're not updating the list of tabs at all when we quit (as
> opposed to clearing the list).

Wow, this is actually much worse than I expected (assuming I'm understanding how this should work).

(1) We don't clear tabs even when we enable "browsing history" in "clear on exit" - we just don't write new restore data (but existing restore data is retained) -> to fix this we need to explicitly clear tabs on exit if this is enabled (and in any other place that we might want to write session restore data).
(2) We try to clear tabs on exit even when we don't explicitly select "browsing history" (but that doesn't work due to (1)) -> we need to only do this when "browsing history" is selected in clear on exit, and not when

Comment 8

[Andrzej Hunt :ahunt]

Attachment: Bug 1228593 - Save session store data when quitting fennec

If we don't pass in dontSaveSession, then dontSaveSesion is undefined
in Browser.quit() (in browser.js), which results in us not correctly
sanitising the session during shutdown.

NOTE: we probably don't need this patch, the better solution (IMHO) is to
check-for/handle  undefined in browser.js

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

Comment 9

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1228593 - dontSaveSession may be undefined, assume false if so r?liuche

GeckoApp sometimes passes in an aClear where only sanitize is defined, and dontSaveSession
has never been set. We need to be able to handle this, and should assume that this
means we should retain the current session (as these are also the default params).
Previously this would result in session restore data either not being saved, or
not being cleared respectively.

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

Comment 10

[Andrzej Hunt :ahunt]

Turns out we had some simple JS issues causing both issues. Either fix is quite simple, and probably worth uplifting.

Comment 11

[Jan Henning [:JanH]]

(In reply to Andrzej Hunt :ahunt from comment #6)
> Heh, I'm actually getting even worse behaviour: I was using an existing
> profile which restored 5 tabs, then did the configuration as inc comment 1
> (I added clear "Downloads" only on exit, restore all tabs was already
> enabled), closed some tabs (2 tabs remaining), select quit -> All 5 tabs
> were restored, not the 2 tabs which I had when I quit.
> 
> I'm guessing we're not updating the list of tabs at all when we quit (as
> opposed to clearing the list).

Generally, after any action that gets recorded by the session store there is a minimum delay of max(10 s - time since last write, 2 s) before those changes get written out to disk.
We do write the session to disk immediately if we receive an "application-background" notification, however quitting the application is a different matter - any special support for writing the session store data to disk on quitting was ripped out in bug 965017.

Comment 12

[Chenxia Liu [:liuche]]

Just to be explicit, what about "Clear Private Data" (the not on exit one)? If we select clear Browsing History, we do not want to clear tabs.

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8755534 [details]
MozReview Request: Bug 1228593 - dontSaveSession may be undefined, assume false if so r?liuche

https://reviewboard.mozilla.org/r/54658/#review51342

::: mobile/android/chrome/content/browser.js:1358
(Diff revision 1)
>      if (cancelQuit.data) {
>        return;
>      }
>  
>      // Tell session store to forget about this window
> -    if (aClear.dontSaveSession) {
> +    // dontSaveSession might not be defined, especially for calls made from java, we should

I don't think you need to do this in JS - undefined is falsey.

Comment 14

[Chenxia Liu [:liuche]]

Oops, so I read the STR wrong the first time.

I'm seeing the following behavior:
1. If I wait a short while (5 sec) after opening my last tab, I can't repro the issue using the STR
2. as Andrzej and JanH describe, if you click quit very soon after closing some tabs (or quit very soon after opening some tabs), an outdated state is re-shown
3. Having "Always restore" tabs and "Clear Browsing History on Exit" both turned on (as described in comment #2) always restores tabs - is that expected? Comment #2 suggests the opposite is expected (e.g. "always restore" and "clear on exit" should clear the tabs) so this would be another bug.

While testing this, patch I don't see anything different, so this just seems like a race condition, as Jan suggested in comment #11.

Comment 15

[Andrzej Hunt :ahunt]

Heh, so I completely misunderstood how things work, to summarise:

- Manually clear private data:
* Current tabs always remain open, we clear all the requested data (no issues that I know of here, but I haven't tested in detail)

- Clear on exit:
* Keep current tabs open if "restore tabs" is enabled (A)
** EXCEPT: we seem to have an expectation of closing the existing tabs if clear "history" is selected in "clear on exit" (i.e. that this should ovverride tab restore. (B)

With the comments above and more testing it seems like:
(A) works, with the caveat that there's a 10 second delay
(B) doesn't work. We always restore tabs if "always restore" is selected.

(B) would be a separate bug, and seems debatable: should "restore tabs" or "clear history" dominate when quitting? IMHO we shouldn't actual bother with (B) at all: restoring tabs is an explicit option, and is corthogonal to clearing history, we shouldn't conflate the two?

(I'll try to see if we can force storing tab state on exit as part of this bug, as per Jan's comments / Bug 965017).

Comment 16

[Andrzej Hunt :ahunt]

Comment on attachment 8755533 [details]
Bug 1228593 - Save session store data when quitting fennec

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54656/diff/1-2/

Comment 17

[Andrzej Hunt :ahunt]

Comment on attachment 8755533 [details]
Bug 1228593 - Save session store data when quitting fennec

Does this look sane to you?

Backing out Bug 965017 didn't seem to work, but we can just copy the current approach from application-background and reuse that when quitting.

This patch seems to actually fix things locally: opening a new tab, force quitting immediately (menu->quit), and opening FF again results in the new tab being persisted (whereas previously the new tab was lost).

Comment 18

[Chenxia Liu [:liuche]]

Talked to Anthony, and filed bug 1275662 for not removing tabs when clearing browser history.

Comment 19

[Jan Henning [:JanH]]

Comment on attachment 8755533 [details]
Bug 1228593 - Save session store data when quitting fennec

https://reviewboard.mozilla.org/r/54656/#review51706

Sorry for taking over your patch like that, but I think you might have opened a can of worms here - although hopefully not a terribly large one.

The thing is that for users who are clearing their history on quit, we're also getting a `browser:purge-session-history` notification, and for users who at the same time aren't restoring their tabs either, a call to `removeWindow()` as well.
There's also a call to `onWindowClose()` during a quit which attempts to write the result of the window closing back to disk which shouldn't really happen, although thankfully it's using `saveStateDelayed()`, so normally Firefox will long have exited before that makes it to disk, but we should really avoid this in the first place (probably a side effect of removing `STATE_QUITTING` in [bug 965017](https://bugzilla.mozilla.org/show_bug.cgi?id=965017), because the call to `saveStateDelayed()` in `onWindowClose()` is still conditional on us being in `STATE_RUNNING`.).

So the basic idea is that during a shutdown we shouldn't start any new writes (by calling `saveState()` directly or responding to a timer callback), but only queue them through `saveStateDelayed()` and incrementing the pending writes and then they are all flushed to storage in one go when we are actually quitting.

::: mobile/android/components/SessionStore.js:40
(Diff revision 2)
>  // -----------------------------------------------------------------------
>  // Session Store
>  // -----------------------------------------------------------------------
>  
>  const STATE_STOPPED = 0;
>  const STATE_RUNNING = 1;

It's probably best to add back `STATE_QUITTING`, so we can change our behaviour during an explicit application shutdown.

::: mobile/android/components/SessionStore.js:147
(Diff revision 2)
>  
>          this._lastClosedTabIndex = -1;
>  
>          if (this._loadState == STATE_RUNNING) {
>            // Save the purged state immediately
>            this.saveState();

Else if loadState = STATE_QUITTING, we should only do a `saveStateDelayed()` here, so we don't immediately write to disk and wait for `flushPendingState()` instead.

::: mobile/android/components/SessionStore.js:159
(Diff revision 2)
>          break;
>        case "timer-callback":
>          // Timer call back for delayed saving
>          this._saveTimer = null;
>          log("timer-callback, pendingWrite = " + this._pendingWrite);
>          if (this._pendingWrite) {

I'd probably make this conditional on us being in `STATE_RUNNING`, or alternatively us *not* being in `STATE_QUITTING`. This is so we don't start any additional writes during shutdown and instead write it all in one go through the `flushPendingState()` call once we're finally quitting for real.

::: mobile/android/components/SessionStore.js:207
(Diff revision 2)
>        case "application-background":
>          // We receive this notification when Android's onPause callback is
>          // executed. After onPause, the application may be terminated at any
>          // point without notice; therefore, we must synchronously write out any
>          // pending save state to ensure that this data does not get lost.
> -        log("application-background");
> +      case "quit-application-requested":

I'd rather use `quit-application-granted` for flushing the changes to disk, because
- after `quit-application-requested` the quit could (theoretically at least) still get canceled
- `quit-application-granted` happens after `removeWindow()` and `browser:purge-session-history`

Also, you'll probably need an additional notification for setting the load state to `STATE_QUITTING`: `quit-application-requested` is too early (it could still get canceled), while `quit-application-granted` happens too late (after both of the history clearing functions have potentially already run). So you'd probably want to send a custom notification to the session store from [here](https://dxr.mozilla.org/mozilla-central/rev/46fe2115d46a5bb40523b8466341d8f9a26e1bdf/mobile/android/chrome/content/browser.js#1356).

::: mobile/android/components/SessionStore.js:604
(Diff revision 2)
>        } else {
>          log("saveStateDelayed() no delay");
>          this.saveState();
>        }
>      }
>      log("saveStateDelayed() timer already running, taking no action");

If I might piggyback on your patch, I've noticed that I didn't implement this piece of logging correctly. It needs to be wrapped in an `else {}`, i.e. it should only happen if `this._saveTimer` evaluates to true.

::: mobile/android/components/SessionStore.js:1403
(Diff revision 2)
>      }
>  
>      delete this._windows[aWindow.__SSID];
>      delete aWindow.__SSID;
>  
>      this.saveState();

Again, if we're in STATE_QUITTING, we only want to `saveStateDelayed()` here.

Comment 20

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/54656/#review51706

> If I might piggyback on your patch, I've noticed that I didn't implement this piece of logging correctly. It needs to be wrapped in an `else {}`, i.e. it should only happen if `this._saveTimer` evaluates to true.

Scratch that, I have another session store related bug of my own in which I can do this.

Comment 21

[Andrzej Hunt :ahunt]

(In reply to Jan Henning [:JanH] from comment #18)
> Comment on attachment 8755533 [details]
> MozReview Request: Bug 1228593 - Force session-store sync when quitting
> fennec
> 
> https://reviewboard.mozilla.org/r/54656/#review51706
> 
> Sorry for taking over your patch like that, but I think you might have
> opened a can of worms here - although hopefully not a terribly large one.

Thank you for the review : ). I'm still not too familiar with this code, so it's good to have you checking this!

I've updated the patch, incorporating parts of the old code too - I think I've managed to cover the various issues you raised. One thing I'm not too sure about is what name to use for the new custom notification, "quit-application-proceeding" is what I'm using for now, but it's not too obvious IMHO.

Comment 22

[Andrzej Hunt :ahunt]

Comment on attachment 8755533 [details]
Bug 1228593 - Save session store data when quitting fennec

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54656/diff/2-3/

Comment 23

[Jan Henning [:JanH]]

Comment on attachment 8755533 [details]
Bug 1228593 - Save session store data when quitting fennec

https://reviewboard.mozilla.org/r/54656/#review55400

There are still a few issues remaining, but they're pretty trivial to fix. Other than that, this seems to be working nicely now, so on that basis I'm happy to go ahead.

::: mobile/android/chrome/content/browser.js:1357
(Diff revision 3)
>      // Quit aborted.
>      if (cancelQuit.data) {
>        return;
>      }
>  
> +    Services.obs.notifyObservers(null, "quit-application-proceeding", null);

I haven't really got any better idea for naming this, either.
I suppose it might have been possible to simply call this something like "Session:Shutdown", although at least your choice of name sounds more generally applicable if somebody else should ever need a notification at precisely that point, too.

::: mobile/android/components/SessionStore.js:142
(Diff revision 3)
>          break;
>        }
>        case "domwindowclosed": // catch closed windows
>          this.onWindowClose(aSubject);
>          break;
> +      case "quit-application-requested":

log("quit-application-requested");

::: mobile/android/components/SessionStore.js:144
(Diff revision 3)
>        case "domwindowclosed": // catch closed windows
>          this.onWindowClose(aSubject);
>          break;
> +      case "quit-application-requested":
> +        // Get a current snapshot of all windows
> +        this._forEachBrowserWindow(function(aWindow) {

On the basis that the window data should be up to date if no save has been queued, you could save a few cycles by making this conditional on `this._pendingWrite` (being > 0), but your call.

::: mobile/android/components/SessionStore.js:145
(Diff revision 3)
>          this.onWindowClose(aSubject);
>          break;
> +      case "quit-application-requested":
> +        // Get a current snapshot of all windows
> +        this._forEachBrowserWindow(function(aWindow) {
> +          self._collectWindowData(aWindow);

This is probably not strictly necessary given that we currently have only one browser window and that only gets closed after "application-quit", so there's no problem in updating the tab data for the window as usual through `__getCurrentState()` in `_saveState()`.

But at least it means we're prepared should shutdown procedures ever change in such a way that the window starts closing *before* "application-quit", so there's no harm in bringing this back.

::: mobile/android/components/SessionStore.js:148
(Diff revision 3)
> +        // Get a current snapshot of all windows
> +        this._forEachBrowserWindow(function(aWindow) {
> +          self._collectWindowData(aWindow);
> +        });
> +        break;
> +      case "quit-application-proceeding":

log("quit-application-proceeding");

::: mobile/android/components/SessionStore.js:152
(Diff revision 3)
> +        break;
> +      case "quit-application-proceeding":
> +        // Freeze the data at what we've got (ignoring closing windows)
> +        this._loadState = STATE_QUITTING;
> +        break;
> +      case "quit-application":

log("quit-application");

::: mobile/android/components/SessionStore.js:158
(Diff revision 3)
> +        observerService.removeObserver(this, "domwindowopened");
> +        observerService.removeObserver(this, "domwindowclosed");
> +        observerService.removeObserver(this, "quit-application-requested");
> +        observerService.removeObserver(this, "quit-application-proceeding");
> +        observerService.removeObserver(this, "quit-application");
> +        observerService.removeObserver(this, "Session:Restore");

We already remove that observer as part of handling this notification, so attempting to remove it here only leads to an exception and breaks the following code.
Since this notification is normally called really early during initialisation (as soon as Gecko is up and running and the session store has initialised), it's probably not worth specially tracking its lifecycle, although if you want to be on the safe side, you could exit early from the notification handler if we're in `STATE_QUITTING`.

::: mobile/android/components/SessionStore.js:161
(Diff revision 3)
> +        observerService.removeObserver(this, "quit-application-proceeding");
> +        observerService.removeObserver(this, "quit-application");
> +        observerService.removeObserver(this, "Session:Restore");
> +
> +        // If a save has been queued, kill the timer and save now
> +        if (this._saveTimer) {

This timer-killing bit won't be needed if you use `flushPendingState()` instead, since we do it in `saveState()` anyway.

::: mobile/android/components/SessionStore.js:164
(Diff revision 3)
> +
> +        // If a save has been queued, kill the timer and save now
> +        if (this._saveTimer) {
> +          this._saveTimer.cancel();
> +          this._saveTimer = null;
> +          this.saveState();

Any particular reason for this switch to `saveState()`? Since we are quitting, I'd strongly suggest attempting to write the data synchronously, i.e. just use `flushPendingState()` as you did in the previous revision. Otherwise it is possible that Firefox exits before the new data is written to disk, especially if Firefox is already busy with other stuff (e.g. right after startup, or when loading a JS-heavy page, or ...)

::: mobile/android/components/SessionStore.js:190
(Diff revision 3)
>          if (this._notifyClosedTabs) {
>            this._sendClosedTabsToJava(Services.wm.getMostRecentWindow("navigator:browser"));
>          }
>          break;
>        case "timer-callback":
> +        if (STATE_RUNNING) {

this._loadState == ...

::: mobile/android/components/SessionStore.js:200
(Diff revision 3)
> -          this.saveState();
> +            this.saveState();
> -        }
> +          }
> +        }
>          break;
>        case "Session:Restore": {
>          Services.obs.removeObserver(this, "Session:Restore");

If you want to be on the safe side, return early after removing the observer if we're in `STATE_QUITTING`.

::: mobile/android/components/SessionStore.js:397
(Diff revision 3)
>      if (aWindow && aWindow.__SSID && this._windows[aWindow.__SSID]) {
>        return;
>      }
>  
>      // Ignore non-browser windows and windows opened while shutting down
> -    if (aWindow.document.documentElement.getAttribute("windowtype") != "navigator:browser") {
> +    if (aWindow.document.documentElement.getAttribute("windowtype") != "navigator:browser" || this._loadState == STATE_QUITTING) {

I wonder whether there currently ever is a situation where this is relevant, but still not a bad idea to bring back this check.

::: mobile/android/components/SessionStore.js:448
(Diff revision 3)
>        delete this._windows[aWindow.__SSID];
>  
>        // Save the state without this window to disk
>        this.saveStateDelayed();
> +    } else if (this._loadState == STATE_QUITTING) {
> +      this.saveStateDelayed();

Are you sure you actually want/need this? There is a "domwindowclosed" notification at the end of each shutdown [1], but that is just a byproduct of Firefox shutting down, not a user action that we actually want to record in the session store.
Also, even if shutdown event ordering should change one day such that we receive this notification before writing out the data during "application-quit", because we aren't deleting that window data from `this._windows[]` here if we're shutting down, the session store data that gets written out effectively doesn't change, so I think there's no need to trigger a save.

I'm happy to be convinced otherwise, though.

[1] Although since you've unregistered that "domwindowclosed" listener during application-quit, we're currently no longer receiving it anyway.

Comment 24

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/340649675132c2307d975dd21a5155c5ccc28b55
Bug 1228593 - Save session store data when quitting fennec r=janh

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/340649675132

Comment 26

[Sorina Florean [:sflorean]]

Verified as fixed in build 50.0a1 2016-07-05;
Device: Nexus 5 (Android 6.0.1).