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

VERIFIED FIXED in Firefox 40

Status

()

Firefox
Session Restore
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Mark A. Hershberger (hexmode), Assigned: ttaubert)

Tracking

unspecified
Firefox 40
Points:
2
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox40 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 2

3 years ago
Created attachment 8592802 [details] [diff] [review]
0001-Bug-1152341-Failure-to-read-one-of-the-session-file-.patch
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8592802 - Flags: review?(dteller)
(Assignee)

Updated

3 years ago
Iteration: --- → 40.2 - 27 Apr
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Comment 3

3 years ago
Hmm, guess we don't really need the finally clause anymore then...
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?
Attachment #8592802 - Flags: review?(dteller) → review+
(Reporter)

Comment 5

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
Agreed.
(Assignee)

Comment 7

3 years ago
Created attachment 8593331 [details] [diff] [review]
0001-Bug-1152341-Failure-to-read-one-of-the-session-file-.patch, v2

Added a test and counting an inaccessible file as corrupt.
Attachment #8592802 - Attachment is obsolete: true
Attachment #8593331 - Flags: review?(dteller)
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.
Attachment #8593331 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 10

3 years ago
(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 :)
(Assignee)

Comment 11

3 years ago
Created attachment 8593474 [details] [diff] [review]
0001-Bug-1152341-Failure-to-read-one-of-the-session-file-.patch, v3
Attachment #8593331 - Attachment is obsolete: true
Attachment #8593474 - Flags: review?(dteller)
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`.
Attachment #8593474 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/4a88a0871393
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
You need to log in before you can comment on or make changes to this bug.