Open
Bug 1426214
Opened 6 years ago
Updated 2 years ago
`XPIProvider.tryGetMTime` should avoid using `try/catch` as a mechanism for checking whether its argument is `null`
Categories
(Toolkit :: Add-ons Manager, defect, P3)
Toolkit
Add-ons Manager
Tracking
()
NEW
People
(Reporter: Yoric, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
This will orange every single mochitest once bug 1426145 has landed.
Comment 1•6 years ago
|
||
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 hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
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.
Attachment #8937831 -
Flags: review?(kmaglione+bmo) → review-
Reporter | ||
Comment 4•6 years ago
|
||
So we should check if the exception is `Components.result.NS_ERROR_FILE_NOT_FOUND`, right? Catching everything is generally a bad idea.
Flags: needinfo?(kmaglione+bmo)
Reporter | ||
Comment 5•6 years ago
|
||
(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•6 years ago
|
||
(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"?
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 8•6 years ago
|
||
(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.
Updated•6 years ago
|
Priority: -- → P3
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8937831 [details] Bug 1426214 - Clean up XPIProvider.tryGetMTime's handling of null arguments; https://reviewboard.mozilla.org/r/208534/#review216268
Attachment #8937831 -
Flags: review?(kmaglione+bmo) → review+
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•