1088500 - Add mozmill test for "Restore Previous Session" feature

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

1 Launch Firefox.
  -> Firefox is opened.
2 Open at least 3 different tabs containing different web pages.
  -> Each webpage is correctly opened.
3 Close Firefox and confirm the Close Tabs pop-up window.
  -> A pop-up window is requesting confirmation for closing tabs.
4 Reopen Firefox and go to History -> Restore Previous Session.
  -> All the previously opened web pages are loaded. The websites are shown in history, the cache and the Firefox customizations are kept.

https://moztrap.mozilla.org/manage/case/11496/

Comment 1

[Teodor Druta]

Attachment: b1088500.patch

The test consists of two parts. 
The first part (testRestorePreviousSession1.js) will open three webpages in different tabs then will close the application maintaining the profile preferences. 
The second part (testRestorePreviousSession2.js) will restore the previous session with the application menu using keyboard shortcuts, then will check if each tab has been correctly restored.

Comment 2

[Andrei Eftimie]

Comment on attachment 8512058 [details] [diff] [review]
b1088500.patch

Review of attachment 8512058 [details] [diff] [review]:
-----------------------------------------------------------------

Its a good test. We can improve on it somewhat.

Please make a single test file. You can guide mozmill to restart firefox then follow-up with the next test with:
> controller.restartApplication("nextTest")

::: firefox/tests/functional/testSessionStore/testRestorePreviousSession1.js
@@ +15,5 @@
> +];
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +

please remove these empty newlines

@@ +33,5 @@
> +  // Open 3 webpages in 3 tabs
> +  // Open First WebPage in current tab
> +  controller.open(TEST_DATA[0])
> +  controller.waitForPageLoad();
> +  // Open 2 new tabs

You can remove this comment. It is pretty clear.

::: firefox/tests/functional/testSessionStore/testRestorePreviousSession2.js
@@ +18,5 @@
> +];
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +

nit: please remove this empty newline

@@ +42,5 @@
> +  controller.keypress(null, "VK_DOWN");
> +  controller.keypress(null, "VK_DOWN");
> +
> +  // Press Enter key to restore previous session
> +  controller.keypress(null, "VK_RETURN");

Instead of sleep + arrow keys, you should be able to use controller.mainMenu()

@@ +46,5 @@
> +  controller.keypress(null, "VK_RETURN");
> +
> +  // Check if the number of correct tabs have been opened
> +  assert.waitFor(() => (tabBrowser.length === TEST_DATA.length),
> +                 "There are " + (TEST_DATA.length + 1) + " opened tabs");

without "+ 1"

@@ +49,5 @@
> +  assert.waitFor(() => (tabBrowser.length === TEST_DATA.length),
> +                 "There are " + (TEST_DATA.length + 1) + " opened tabs");
> +
> +  // Check if correct tabs have been opened
> +  TEST_DATA.forEach(function(td, index) {

You can use the fat arrow syntax for anonymous functions.
Also use url instead of td.

> (aURL, aIndex) => {

Comment 3

[Teodor Druta]

Attachment: b1088500v2.patch

Updated the patch following Andrei's review lines, the test now uses controller.mainMenu to acces the 'Restore Previous Session' entry instead of keyboard shortcuts.

Comment 4

[Teodor Druta]

> controller.restartApplication("nextTest")

We can't use restartApplication in this test. restartApplication restarts firefox with all the previous tabs open by default

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You have to reset the preference for closing tabs during shutdown. We turn that off by default. Accepting to close all tabs, you won't see them on next startup.

Comment 6

[Teodor Druta]

Henrik, if you're talking about 'browser.tabs.warnOnClose' to true, It doesn't help it won't display the warn dialog when restartApplication is called. Even setting 'browser.startup.page' to 0 (Start with a blank page), will continue to automatically open the tabs after restartApplication. I think that in order for restore previous session to work, the application must be closed first with stopApplication, and then reopened again so the restore previous session option will be available.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There is no difference in the shutdown logic for restartAppliation and stopApplication.

Comment 8

[Teodor Druta]

After thoroughly investigating this issue I'm pretty sure that firefox automatically restores the previous session on restart, and I think this cannot be disabled from preferences.

I tried setting:
browser.sessionstore.resume_session_once > true
browser.sessionstore.resume_from_crash > false
browser.sessionstore.max_tabs_undo > 0
browser.sessionstore.max_windows_undo > 0
places.history.enabled > false
browser.sessionstore.max_resumed_crashes > 0
browser.sessionstore.max_tabs_undo > 0
browser.sessionstore.max_windows_undo > 0

None of the above tried options worked, firefox still restored the previous session tabs even with history disabled.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With a fresh profile, and restarting Firefox the previously opened tabs should not be restored. That is the behavior we have since years. So I don't think that something has been changed here. Default in preferences should still be "Show my Homepage".

Comment 10

[Teodor Druta]

Henrik, can you please specifiy if this works for you:
1. Open firefox nightly
2. Open a few webpages in tabs
3. Restart Firefox: 
 * Open Developer Toolbar [Shift + F12]
 * write "restart" in the toolbar
 - or use the "Restartless" add-on.
Thanks.

Comment 11

[Teodor Druta]

After further investigations I found this lines in SessionStore.jsm:
http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1150-1173

>     if (aData == "restart") {
>        this._prefBranch.setBoolPref("sessionstore.resume_session_once", true);
>     ...

I think this statement is responsible for automatically restoring the session on restart.
Setting this preference to false won't help because SessionStore will set it to true by default on restart.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Teodor Druta from comment #10)
> Henrik, can you please specifiy if this works for you:
> 1. Open firefox nightly
> 2. Open a few webpages in tabs
> 3. Restart Firefox: 
>  * Open Developer Toolbar [Shift + F12]
>  * write "restart" in the toolbar
>  - or use the "Restartless" add-on.

Sure, when Firefox is restarted only the tab with my homepage is loaded and nothing else. Please make sure that you are using a fresh profile if not done yet. Tested with latest Nightly on Ubuntu 14.04.

Comment 13

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #12) 
> Sure, when Firefox is restarted only the tab with my homepage is loaded and
> nothing else. Please make sure that you are using a fresh profile if not
> done yet. Tested with latest Nightly on Ubuntu 14.04.

This is interesting.
I get the same results as Teodor (I am on OSX).

1. open Nightly with a fresh profile
2. open a couple tabs
3. issue restart ("restart" command in DeveloperToolbar [Shift+f12])
4. Firefox restores my tabs
5. issue shutdown [Cmd+Q]
6. reopen Firefox
7. I am on the homepage and I have the ability to Restore Session

Comment 14

[Andrei Eftimie]

Comment on attachment 8513429 [details] [diff] [review]
b1088500v2.patch

Review of attachment 8513429 [details] [diff] [review]:
-----------------------------------------------------------------

Since we won't be able to have a single file, we'll need to keep both of them in a folder.

::: firefox/tests/functional/testSessionStore/testRestorePreviousSession1.js
@@ +20,5 @@
> +  aModule.tabBrowser.closeAllTabs();
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.controller.stopApplication(false);

`false` is the default value, you can omit it.

@@ +35,5 @@
> +  openTabWithUrl(TEST_DATA[2]);
> +
> +  // Check for correct number of opened tabs
> +  assert.equal(tabBrowser.length, TEST_DATA.length,
> +               "There are " + TEST_DATA.length + "opened tabs");

nit: missing space '" opened'

@@ +46,5 @@
> + *        Url of the page to navigate to
> + */
> +function openTabWithUrl(aUrl) {
> +  tabBrowser.openTab();
> +  controller.open(aUrl);

Interesting. `controller` should not leak to the global namespace, you shouldn't have access to it here.
Please use `tabBrowser.controller`

::: firefox/tests/functional/testSessionStore/testRestorePreviousSession2.js
@@ +39,5 @@
> +  // Check if correct tabs have been opened
> +  TEST_DATA.forEach((url, index) => {
> +    // Wait for each tab to load correctly
> +    tabBrowser.selectedIndex = index;
> +    controller.waitForPageLoad();

With this check I feel we should iterate on tabBrowser.getElements(type: 'tabs') and check vs TEST_DATA elements.
So the other way around.

Comment 15

[Teodor Druta]

Attachment: b1088500v3.patch

> 
> Since we won't be able to have a single file, we'll need to keep both of
> them in a folder.
> 

Moved the test to a separate folder.

> 
> ::: firefox/tests/functional/testSessionStore/testRestorePreviousSession2.js
> @@ +39,5 @@
> > +  // Check if correct tabs have been opened
> > +  TEST_DATA.forEach((url, index) => {
> > +    // Wait for each tab to load correctly
> > +    tabBrowser.selectedIndex = index;
> > +    controller.waitForPageLoad();
> 
> With this check I feel we should iterate on tabBrowser.getElements(type:
> 'tabs') and check vs TEST_DATA elements.
> So the other way around.

There is not getElements() method in for tabBrowser.

I've changed the code to wait for the tab to open and check if it exists, then check its index with the one from TEST_DATA.

Comment 16

[Andrei Eftimie]

Comment on attachment 8514889 [details] [diff] [review]
b1088500v3.patch

Review of attachment 8514889 [details] [diff] [review]:
-----------------------------------------------------------------

With the below mentioned changed, please also include a functional testrun.
Looks good to me.

::: firefox/tests/functional/testSessionStore/testRestorePreviousSession/manifest.ini
@@ +1,1 @@
> +[parent:../manifest.ini]

You'll need to include this in the parent manifest with [include:], otherwise it won't be run as part of a testrun.

Comment 17

[Teodor Druta]

Attachment: b1088500v3.patch

Included the manifest.ini in parent manifest.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrei Eftimie from comment #13)
> 3. issue restart ("restart" command in DeveloperToolbar [Shift+f12])
> 4. Firefox restores my tabs

Maybe the dev tools tell Firefox to restore the session after a restart? You may want to check what the 'restart' command is actually doing.

Comment 19

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> (In reply to Andrei Eftimie from comment #13)
> > 3. issue restart ("restart" command in DeveloperToolbar [Shift+f12])
> > 4. Firefox restores my tabs
> 
> Maybe the dev tools tell Firefox to restore the session after a restart? You
> may want to check what the 'restart' command is actually doing.

As Teodor pointed out in comment 11, I highly suspect it is the following pref:
> sessionstore.resume_session_once
http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1166-1167

An old article about it: http://kb.mozillazine.org/Browser.sessionstore.resume_session_once

"Firefox 2.0 introduces a built-in Session Restore feature, allowing the user to continue browsing from where they left off if browser restarts. This preference controls whether the last saved session is restored once the next time the browser starts (for use after software updates, extension installation, etc.)."

=== 

With all this, when you restart Firefox, the preference `sessionstore.resume_session_once` is set to `true` and when Firefox starts again it will automatically restore the previous session.

I get the same results with the "Restartless Restart" addon.

Comment 20

[Andrei Eftimie]

Comment on attachment 8514942 [details] [diff] [review]
b1088500v3.patch

Review of attachment 8514942 [details] [diff] [review]:
-----------------------------------------------------------------

Henrik, any updates on your side regarding the restart issue?

I would like to have this as 1 test only, but I don't see how we can do this right now...
If you have a specific way of restarting without getting the `sessionstore.resume_session_once` pref please share it.

Otherwise we should go forward with 2 tests.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8514942 [details] [diff] [review]
b1088500v3.patch

Given your question, you do not want a feedback on the patch but a needinfo flag set...

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

What you want here is clearly a discussion with one of the devs in charge of Sessionstore and its behavior triggered by a restart. I haven't seen a conversation yet, and I do not have the time to dive into this problem. Sorry.

Comment 23

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> What you want here is clearly a discussion with one of the devs in charge of
> Sessionstore and its behavior triggered by a restart. I haven't seen a
> conversation yet, and I do not have the time to dive into this problem.
> Sorry.

Given that the code is very clear, I don't see why a discussion is needed.
But nevertheless we'll initiate one.

Not sure who to ping about SessionStore. Tim, I found you active in certain SessionStore issues, if you can help with some information great, if not please advice on who to ask for more details.

TL;DR:
Henrik wants to know whether the behaviour of restoring the Session on a restart is indeed wanted / right behaviour.

Code seems to support that with setting the `sessionstore.resume_session_once` pref to true in case of a restart:
http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1166-1167

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Andrei Eftimie from comment #23)
> Henrik wants to know whether the behaviour of restoring the Session on a
> restart is indeed wanted / right behaviour.

Yes. If you restart the browser to apply an update or enable an add-on you clearly don't want to lose your session. That is the only way we expose restarts to users afaik.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrei Eftimie from comment #23)
> Henrik wants to know whether the behaviour of restoring the Session on a
> restart is indeed wanted / right behaviour.

This is not clearly what we want to know. What we are interested in is the question if this behavior can be disabled or not.

Comment 26

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> What we are interested in is the question if this behavior can be disabled or not.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> (In reply to Andrei Eftimie from comment #23)
> > Henrik wants to know whether the behaviour of restoring the Session on a
> > restart is indeed wanted / right behaviour.
> 
> This is not clearly what we want to know. What we are interested in is the
> question if this behavior can be disabled or not.

We currently have no way to disable it as you can tell from the code. You could reset the pref after shutdown or on startup if you're fast enough. You could also remove session files so that there is nothing to restore from...

Comment 28

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8514942 [details] [diff] [review]
b1088500v3.patch

Review of attachment 8514942 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Tim for the info!

Adding a checkin flag for Monday, it's safer than landing it now.

Comment 29

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8514942 [details] [diff] [review]
b1088500v3.patch

Review of attachment 8514942 [details] [diff] [review]:
-----------------------------------------------------------------

Local testruns were green, landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/9d5a44d3e25a (default)

Thanks Teodor!

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

If this is not an urgent test as requested by QA we should not get it backported down to beta. So do we have specific reasons for?

Comment 31

[Andreea Matei [:AndreeaMatei]]

It's from the regression testsuite we started to cover more of, and since the QA team runs this manually now, it would be helpful to remove as many manual tests as quickly as possible to leave time for other checks they have to do.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sounds fair. Thanks.

Comment 33

[Teodor Druta]

The patch applies well on mozilla-aurora and mozilla-beta branches.
http://mozmill-crowd.blargon7.com/#/functional/report/43b0604030afed1afa48590287ba4db3
http://mozmill-crowd.blargon7.com/#/functional/report/43b0604030afed1afa48590287bcd657

Comment 34

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8514942 [details] [diff] [review]
b1088500v3.patch

Review of attachment 8514942 [details] [diff] [review]:
-----------------------------------------------------------------

Backported:
http://hg.mozilla.org/qa/mozmill-tests/rev/bbc166ff702a (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/d23d24af886b (beta)