1152341 - Error accessing recovery.js on non-default profile keeps FF from restoring session or starting properly

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Mark A. Hershberger (hexmode)]

I recently had a problem with some preferences not working and closed tabs not being recoverable.  I suffered for a while and even tried to use FF Sync to get back a working configuration, but nothing was helping.

Then I started FF in a terminal to see if there were any signs of the problem.  I got the following:

  console.error: 
    Message: Unix error 5 during operation read on file [non-default-session-dir]/sessionstore-backups/recovery.js (Input/output error)

I quit FF deleted the file and restarted.  Voila!  Suddenly, my tabs from my default session (tabs I had given up on recovering) appeared!

The "Self-Destructing cookies" addon worked again!

I could recover closed tabs again!

If FF runs into a problem reading recovery.js, the user should be given a more visible notification so they understand things are broken and why.  Better would be to fix the problem (move the file to recovery.js-broken?) and continue.

Even if the above can't be done then a problem with the recovery.js file for the profile the user is not using shouldn't cause a problem with FF's operations.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Yeah... I can reproduce that. Removing $profile/sessionstore.js and then doing a "chmod 0 $profile/sessionstore-backups/recovery.js" will yield a similar failure and we don't even try to restore from any other paths we should try after recovery.js

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1152341-Failure-to-read-one-of-the-session-file-.patch

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Hmm, guess we don't really need the finally clause anymore then...

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8592802 [details] [diff] [review]
0001-Bug-1152341-Failure-to-read-one-of-the-session-file-.patch

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

r=me
Shouldn't this be counted as a corrupted file, though?

Comment 5

[Mark A. Hershberger (hexmode)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #4)
> Shouldn't this be counted as a corrupted file, though?

I would think so, yeah.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Agreed.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1152341-Failure-to-read-one-of-the-session-file-.patch, v2

Added a test and counting an inaccessible file as corrupt.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8593331 [details] [diff] [review]
0001-Bug-1152341-Failure-to-read-one-of-the-session-file-.patch, v2

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

I think that the test needs a little more loving before landing this.

::: browser/components/sessionstore/test/browser_backup_recovery.js
@@ +116,5 @@
>    yield File.writeAtomic(Paths.recovery, "<Invalid JSON>");
>    is((yield SessionFile.read()).source, SOURCE, "Recovered the correct source from the recovery file");
> +  yield SessionFile.wipe();
> +
> +  info("Corrupting recovery file again, attempting to recover from recovery backup");

s/Corrupting recovery file again/Making recovery file unreadable/

@@ +117,5 @@
>    is((yield SessionFile.read()).source, SOURCE, "Recovered the correct source from the recovery file");
> +  yield SessionFile.wipe();
> +
> +  info("Corrupting recovery file again, attempting to recover from recovery backup");
> +  let SOURCE_CORRUPTED = yield promiseSource("Paths.recovery");

That doesn't look very corrupted to me. Why do you call this `SOURCE_CORRUPTED`?

@@ +124,5 @@
> +  yield File.writeAtomic(Paths.recoveryBackup, SOURCE);
> +
> +  // Write a valid recovery file but make it unreadable.
> +  yield File.writeAtomic(Paths.recovery, SOURCE_CORRUPTED);
> +  yield File.setPermissions(Paths.recovery, { unixMode: 0 });

The test is probably not going to work on all platforms. Might wish to make it conditional on the platform.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #9)
> > +  info("Corrupting recovery file again, attempting to recover from recovery backup");
> > +  let SOURCE_CORRUPTED = yield promiseSource("Paths.recovery");
> 
> That doesn't look very corrupted to me. Why do you call this
> `SOURCE_CORRUPTED`?

Fair point.

> > +  // Write a valid recovery file but make it unreadable.
> > +  yield File.writeAtomic(Paths.recovery, SOURCE_CORRUPTED);
> > +  yield File.setPermissions(Paths.recovery, { unixMode: 0 });
> 
> The test is probably not going to work on all platforms. Might wish to make
> it conditional on the platform.

Yeah, I thought so. Pushed it to try to find out :)

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1152341-Failure-to-read-one-of-the-session-file-.patch, v3

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8593474 [details] [diff] [review]
0001-Bug-1152341-Failure-to-read-one-of-the-session-file-.patch, v3

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

::: browser/components/sessionstore/test/browser_backup_recovery.js
@@ +117,5 @@
>    is((yield SessionFile.read()).source, SOURCE, "Recovered the correct source from the recovery file");
> +  yield SessionFile.wipe();
> +
> +  // Can't do chmod() on non-UNIX platforms, we need that for the tests below.
> +  if (AppConstants.platform != "macosx" && AppConstants.platform != "linux") {

Nit: Could you rather isolate the code that's platform-specific to its own task? Otherwise, we risk people adding code at the end of this task without realizing that there is an early `return`.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4a88a0871393

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4a88a0871393

Comment 15

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed on latest Nightly, build ID: 20150422030206
Tested on Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.9.5.