Lots of _SessionFile.jsm errors when running tests

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Session Restore
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
When running tests we seem to often not have a valid session state backup file to remove:

> JavaScript Error: "Uncaught asynchronous error: Unix error 2 during operation remove (No such file or directory) at undefined" {file: "resource:///modules/sessionstore/_SessionFile.jsm" line: 108}
> JavaScript Error: "Could not remove session state backup file: Unix error 2 during operation remove (No such file or directory)" {file: "resource:///modules/sessionstore/_SessionFile.jsm" line: 246}

These are caused by SessionFile.wipe(). I think wipe should not report an error to the console in case the files don't exist. After all that's what we want to achieve when wiping :)
(Assignee)

Comment 1

5 years ago
Created attachment 715140 [details] [diff] [review]
don't report errors when wipe() tries to remove non-existant session files
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #715140 - Flags: review?(dteller)
(Assignee)

Comment 2

5 years ago
Saw it on Windows, too.
OS: Linux → All
Hardware: x86_64 → All
Can you use:

} catch (ex if !self._isNoSuchFile(ex)) {
// Nothing
} catch (ex) {

?
(Assignee)

Comment 4

5 years ago
I actually wanted to give that a try because I was quite sure that this works but I didn't really like the empty catch clause in there. Is this really better than the if statement?
Seems clearer to me (maybe with a more useful comment than "// Nothing"), but I guess I don't feel strongly.
(Assignee)

Comment 6

5 years ago
Created attachment 715436 [details] [diff] [review]
don't report errors when wipe() tries to remove non-existant session files

Incorporated suggestion from comment #5.
Attachment #715140 - Attachment is obsolete: true
Attachment #715140 - Flags: review?(dteller)
Attachment #715436 - Flags: review?(dteller)
Comment on attachment 715436 [details] [diff] [review]
don't report errors when wipe() tries to remove non-existant session files

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

Looks good to me, with a possible suggestion.
Oh, and I'm not sure "non-existant" exists in English. Perhaps "non-existent"?

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +243,2 @@
>        }
> +

Why not the following?

catch (ex if self._isNoSuchFile(ex)) {
  // Ignore exceptions
} catch (ex) {
  Cu.reportError(...);
  throw ex;
}
Attachment #715436 - Flags: review?(dteller) → review+
(Assignee)

Comment 8

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> Looks good to me, with a possible suggestion.
> Oh, and I'm not sure "non-existant" exists in English. Perhaps
> "non-existent"?

Oops.

> Why not the following?
> 
> catch (ex if self._isNoSuchFile(ex)) {
>   // Ignore exceptions
> } catch (ex) {
>   Cu.reportError(...);
>   throw ex;
> }

Yeah, I like that more. I should've thought of that :)
(In reply to Tim Taubert [:ttaubert] from comment #8)
> > Why not the following?
> > 
> > catch (ex if self._isNoSuchFile(ex)) {
> >   // Ignore exceptions
> > } catch (ex) {
> >   Cu.reportError(...);
> >   throw ex;
> > }
> 
> Yeah, I like that more. I should've thought of that :)

Isn't that what I said in comment 3 (minus a typo and a better comment)? o_O
(Assignee)

Comment 10

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Isn't that what I said in comment 3 (minus a typo and a better comment)? o_O

Comment 3 has an exclamation mark. Or is that the typo? I assumed your comment was misplaced but probably the exclamation mark was :) Whatever, thanks to you both my patch is now nicer and I'll go ahead and land it.
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/mozilla-central/rev/2475d3d05a9c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
You need to log in before you can comment on or make changes to this bug.