830656 - Lots of _SessionFile.jsm errors when running tests

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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 :)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: don't report errors when wipe() tries to remove non-existant session files

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Saw it on Windows, too.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Can you use:

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

?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Seems clearer to me (maybe with a more useful comment than "// Nothing"), but I guess I don't feel strongly.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: don't report errors when wipe() tries to remove non-existant session files

Incorporated suggestion from comment #5.

Comment 7

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

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;
}

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(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 :)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/2475d3d05a9c

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2475d3d05a9c