1426214 - `XPIProvider.tryGetMTime` should avoid using `try/catch` as a mechanism for checking whether its argument is `null`

Status: NEW

(Toolkit :: Add-ons Manager, defect, P3)

Description

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

This will orange every single mochitest once bug 1426145 has landed.

Comment 1

[Andrew Swan [:aswan]]

I don't understand this, bug 1426145 isn't very detailed, what does "unexpected" mean here?  If the exception is caught why would that cause a test failure?

Comment 2

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

Attachment: Bug 1426214 - Clean up XPIProvider.tryGetMTime's handling of null arguments;

Method `XPIProvider.tryGetMTime` works with `null` argument by
wrapping its own body in a try/catch everything. This try/catch may
have been necessary when we needed to handle arbitrary extensions, but
these days, it's a code smell, bound to catch random undocumented
errors. In addition, the pattern will cause general mochitest failures
once bug 1426145 lands.

Replacing this by a simple `if (!file) ...`.

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

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8937831 [details]
Bug 1426214 - Clean up XPIProvider.tryGetMTime's handling of null arguments;

https://reviewboard.mozilla.org/r/208534/#review214312

The try-catch is not meant to handle null arguments, it's meant to handle nonexistent files.

Comment 4

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

So we should check if the exception is `Components.result.NS_ERROR_FILE_NOT_FOUND`, right? Catching everything is generally a bad idea.

Comment 5

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

(In reply to Andrew Swan [:aswan] from comment #1)
> I don't understand this, bug 1426145 isn't very detailed, what does
> "unexpected" mean here?  If the exception is caught why would that cause a
> test failure?

We're in the process of making every TypeError, ReferenceError and SyntaxError in our code that is not explicitly whitelisted a test error, *even if it is caught*. The reason being that we realized that we accidentally catch many of these, all over the place.

Comment 6

[Kris Maglione [:kmag]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)
> So we should check if the exception is
> `Components.result.NS_ERROR_FILE_NOT_FOUND`, right? Catching everything is
> generally a bad idea.

I suppose. But I'm also not especially concerned if it fails for another reason. I'd rather that not be fatal.

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #5)
> We're in the process of making every TypeError, ReferenceError and
> SyntaxError in our code that is not explicitly whitelisted a test error,
> *even if it is caught*. The reason being that we realized that we
> accidentally catch many of these, all over the place.

Who is "we"?

Comment 7

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

Comment on attachment 8937831 [details]
Bug 1426214 - Clean up XPIProvider.tryGetMTime's handling of null arguments;

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

Comment 8

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

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #6)
> (In reply to David Teller [:Yoric] (please use "needinfo") from comment #5)
> > We're in the process of making every TypeError, ReferenceError and
> > SyntaxError in our code that is not explicitly whitelisted a test error,
> > *even if it is caught*. The reason being that we realized that we
> > accidentally catch many of these, all over the place.
> 
> Who is "we"?

I'm spearheading this, along with Florian and RyanVM, in an effort to reduce the number of ignored errors in our JS codebase.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8937831 [details]
Bug 1426214 - Clean up XPIProvider.tryGetMTime's handling of null arguments;

https://reviewboard.mozilla.org/r/208534/#review216268