1389564 - Extend session store test to cover writing of restored sessions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, enhancement)

Description

[Jan Henning [:JanH]]

We have a test that writes a basic session store file and tests that the tabs are restored properly and we have a test that opens some tabs and then checks the written session store file.
We should extend this to test that a restored session is written properly as well.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1389564 - Part 1 - Move common session test functionality into base class.

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

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1389564 - Part 2 - Test that restored (background) tabs are correctly written back to disk.

So far, we haven't tested session restoring and saving in combination, which led to bug 1379374 where restored background tabs weren't being written back to disk after upgrading to Firefox 55.

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

Comment 3

[Geoff Brown [:gbrown]]

Comment on attachment 8896617 [details]
Bug 1389564 - Part 1 - Move common session test functionality into base class.

https://reviewboard.mozilla.org/r/167900/#review173492

Comment 4

[Geoff Brown [:gbrown]]

Comment on attachment 8896618 [details]
Bug 1389564 - Part 2 - Test that restored (background) tabs are correctly written back to disk.

https://reviewboard.mozilla.org/r/167664/#review173502

This looks good - thanks!

Please run at least 5 times on try before check-in.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/SessionTest.java:399
(Diff revision 1)
>          }
>      }
>  
> +    protected void deleteProfileFile(String filename) {
> +        File fileToDelete = new File(mProfile, filename);
> +        fileToDelete.delete();

File.delete() returns a boolean -- worth checking and at least logging when there is an error.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testSessionFilePreservation.java:26
(Diff revision 1)
> +    private static final String PREFS_NAME = "GeckoApp";
> +    private static final String PREFS_ALLOW_STATE_BUNDLE = "allowStateBundle";
> +    private final static int SESSION_TIMEOUT = 25000;
> +
> +    @Override
> +    public void setActivityIntent(Intent intent) {

Is this identical to the code in testSessionOOMRestore? You wouldn't want to move setActivityIntent to SessionTest, but you could move the body to a new helper function in SessionTest...just a suggestion.

Comment 5

[Jan Henning [:JanH]]

Comment on attachment 8896618 [details]
Bug 1389564 - Part 2 - Test that restored (background) tabs are correctly written back to disk.

https://reviewboard.mozilla.org/r/167664/#review173502

> Is this identical to the code in testSessionOOMRestore? You wouldn't want to move setActivityIntent to SessionTest, but you could move the body to a new helper function in SessionTest...just a suggestion.

I did move VerifyJSONCondition into the base class after all, so yes, I should do that here as well.

Comment 6

[Jan Henning [:JanH]]

This shows catching bug 1379374 (if sadly after the fact):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06148dd2f9e6f6a31338a4c06454bf1851a53ab6&selectedJob=122632874

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8896617 [details]
Bug 1389564 - Part 1 - Move common session test functionality into base class.

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

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8896618 [details]
Bug 1389564 - Part 2 - Test that restored (background) tabs are correctly written back to disk.

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

Comment 9

[Jan Henning [:JanH]]

... and https://treeherder.mozilla.org/#/jobs?repo=try&revision=c00b692874f041abc535fe3b89fa037f0abe7a7e

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16f18931fd06
Part 1 - Move common session test functionality into base class. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/7d03c3a5bb4b
Part 2 - Test that restored (background) tabs are correctly written back to disk. r=gbrown

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16f18931fd06
https://hg.mozilla.org/mozilla-central/rev/7d03c3a5bb4b

Comment 12

[Geoff Brown [:gbrown]]

:JanH -- Several intermittent-failure bugs have been filed against testSessionOOMSave since these changes: bug 1391761, bug 1394166, bug 1394275, bug 1394685. Perhaps something went wrong here?

Comment 13

[Jan Henning [:JanH]]

Hmm, as far as testSessionOOMSave is concerned, all I did was move/extract some methods into the base class, so that shouldn't actually make a difference. Given that some of these crashes seem to be somewhere within the JVM, all bets might be off if we might be triggering some sort of OS bug? I've no idea, really...

Comment 14

[Jan Henning [:JanH]]

Although there does seem to be something weird going on - now we've got bug 1395689, bug 1395690 and bug 1395687 as well.

I'm not intimately familiar with our various Robocop tests to say how unique testSessionOOMSave really is, but what's probably characteristic about it is that we're opening three tabs and loading two URLs in each of them, and we're doing it the Robocop way, i.e. simulating user input. This means that we'll be entering and exiting editing mode quite bit and likewise loading and unloading the home panels relatively frequently.

So an alternative theory could be that something (Something about the Photon redesign? Some recent Activity Stream changes?) related to that might be triggering the crashes and testSessionOOMSave happens to attract them because we're showing and hiding the home panels so frequently?