Closed
Bug 1375212
Opened 7 years ago
Closed 7 years ago
Wrap thrown Strings in Error objects
Categories
(Firefox :: Sync, enhancement, P3)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: eoger, Assigned: nicolaso, Mentored)
Details
(Keywords: good-first-bug)
Attachments
(1 file, 3 obsolete files)
It seems like we throw String objects in Sync a lot: http://searchfox.org/mozilla-central/search?q=throw+%22&path=services%2Fsync This might cause some logging issues in xpcshell-tests for example. We should wrap these strings in Error objects, which will provide stack traces, line numbers etc..
Reporter | ||
Updated•7 years ago
|
Summary: Wrap throw Strings in Error objects → Wrap thrown Strings in Error objects
Comment 1•7 years ago
|
||
We should ensure that this doesn't change how telemetry reports these errors.
Assignee | ||
Comment 2•7 years ago
|
||
This seems good for my first bug, I'd like to pick this up. How do we make sure we're not breaking anything, as Mark mentioned?
Comment 3•7 years ago
|
||
(In reply to Nicolas Ouellet-Payeur from comment #2) > This seems good for my first bug, I'd like to pick this up. How do we make > sure we're not breaking anything, as Mark mentioned? I *think* the existing telemetry tests might cover most of this, but we might need a new test in test_telemetry that a simulated 'throw "oh no"' is reported exactly as 'throw new Error("oh no")' (and as mentioned, checking that test file might actually indicate the existing tests already do check that, either directly or indirectly)
Assignee | ||
Comment 4•7 years ago
|
||
Here's a patch with just enough code to fix the issues highlighted, while keeping the tests passing. Even though the tests are still passing, I'm confident that merging this patch would break something. I had to change a bit of test code, and I'm not very familiar with Sync code. Also, there are a few places where we treat exceptions as strings, and I didn't touch most of those. For instance: http://searchfox.org/mozilla-central/search?q=%5C%2B+ex%5Cb&case=false®exp=true&path=services%2Fsync I'd like some feedback & guidance on what to do next, to ensure stability.
Reporter | ||
Comment 5•7 years ago
|
||
Comment on attachment 8889112 [details] [diff] [review] 1375212.patch Review of attachment 8889112 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a good start to me, let's run a try build of this to identify possible breakages: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f82cbd6a29f7dc5c05097e105e436032e890a5 this._log methods can take multiple arguments, let's use that for the cases you listed. (e.g. this._log.error("Message", ex) ) ::: services/sync/modules/engines/bookmarks.js @@ +704,4 @@ > // Figure out the local id of the parent GUID if available > let parentGUID = record.parentid; > if (!parentGUID) { > + throw new Error( Let's use template literals instead: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Template_literals And everywhere else in this patch where you do concatenations. ::: services/sync/modules/util.js @@ +122,5 @@ > }, > > isLockException: function isLockException(ex) { > + return ex && ex.message && ex.message.indexOf && > + ex.message.indexOf("Could not acquire lock.") == 0; Let's replace the 2 indexOf conditions by 1 includes() since we know message is always a String. However, I think we should instead create our own Error subclass for this case, which would make our error matching cleaner, what do you think Mark? See AbortRepairError: http://searchfox.org/mozilla-central/source/services/sync/modules/bookmark_repair.js#40-45 http://searchfox.org/mozilla-central/source/services/sync/modules/bookmark_repair.js#259 @@ +251,5 @@ > > isHMACMismatch: function isHMACMismatch(ex) { > const hmacFail = "Record SHA256 HMAC mismatch: "; > + return ex && ex.message && ex.message.indexOf && > + (ex.message.indexOf(hmacFail) == 0); Ditto ::: services/sync/tests/unit/test_utils_catch.js @@ +53,4 @@ > return this._catch(function() { > rightThis = this == obj; > didCall = true; > + return Promise.resolve().then( () => { Let's just make the closure async and simply throw the Error instead.
Attachment #8889112 -
Flags: feedback?(markh)
Comment 6•7 years ago
|
||
Comment on attachment 8889112 [details] [diff] [review] 1375212.patch Review of attachment 8889112 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, thanks! I think those test changes are fine. I agree with Ed's comments, and it would also be great if you could apply the following change as part of your patch: diff --git services/.eslintrc.js services/.eslintrc.js index 30dfbde..ccdec8b 100644 --- services/.eslintrc.js +++ services/.eslintrc.js @@ -3,5 +3,8 @@ module.exports = { plugins: [ "mozilla" - ] + ], + "rules" : { + "no-throw-literal": 2, + } } and try running "./mach eslint services" - without your patch it shows 56 problems, but with your patch it should show none - this will help make sure that you didn't miss any, and that no one adds more later. Thanks!
Attachment #8889112 -
Flags: feedback?(markh)
Attachment #8889112 -
Flags: feedback+
Updated•7 years ago
|
Assignee: nobody → nicolaso
Assignee | ||
Comment 7•7 years ago
|
||
> with your patch it should show none Not with the patch I attached earlier. There are still other occurrences, in other sub-directories of services/. Should I fix those too, while we're at it? We're also throwing non-string literals in other places in services/sync/, making the rule fail. Things like: http://searchfox.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#466 http://searchfox.org/mozilla-central/source/services/sync/tests/unit/test_utils_catch.js#30 Those should be easy enough to fix. > services/common/rest.js > 284:7 error Expected an object to be thrown. no-throw-literal (eslint) > 300:7 error Expected an object to be thrown. no-throw-literal (eslint) > > services/common/tests/unit/test_load_modules.js > 37:7 error Expected an object to be thrown. no-throw-literal (eslint) > 52:7 error Expected an object to be thrown. no-throw-literal (eslint) > > services/common/utils.js > 158:7 error Expected an object to be thrown. no-throw-literal (eslint) > 306:11 error Expected an object to be thrown. no-throw-literal (eslint) > 309:11 error Expected an object to be thrown. no-throw-literal (eslint) > > services/crypto/modules/WeaveCrypto.js > 110:13 error Expected an object to be thrown. no-throw-literal (eslint) > 179:17 error Expected an object to be thrown. no-throw-literal (eslint) > > services/fxaccounts/FxAccountsClient.jsm > 602:7 error Expected an object to be thrown. no-throw-literal (eslint) > > services/fxaccounts/tests/xpcshell/test_accounts.js > 124:39 error Expected an object to be thrown. no-throw-literal (eslint) > 1067:84 error Expected an object to be thrown. no-throw-literal (eslint) > > services/sync/modules/engines/bookmarks.js > 466:7 error Expected an object to be thrown. no-throw-literal (eslint) > > services/sync/tests/unit/test_utils_catch.js > 30:9 error Expected an object to be thrown. no-throw-literal (eslint) > 38:9 error Expected an object to be thrown. no-throw-literal (eslint) > > services/sync/tests/unit/test_utils_notify.js > 24:9 error Expected an object to be thrown. no-throw-literal (eslint) > > ✖ 16 problems (16 errors, 0 warnings)
Comment 8•7 years ago
|
||
(In reply to Nicolas Ouellet-Payeur from comment #7) > > with your patch it should show none > > Not with the patch I attached earlier. There are still other occurrences, in > other sub-directories of services/. Should I fix those too, while we're at > it? I think it's probably worth doing many of them, yeah. > We're also throwing non-string literals in other places in services/sync/, > making the rule fail. Things like: > http://searchfox.org/mozilla-central/source/services/sync/modules/engines/ > bookmarks.js#466 hrmph - I think fixing that seems risky (so better tackled in a different bug), but we always have the option of adding // eslint-disable-line style comments in those places - I guess it depends on what the final patch ends up looking like (ie, the eslint rule *seems* like a good idea, but I could be convinced it is not :) > http://searchfox.org/mozilla-central/source/services/sync/tests/unit/ > test_utils_catch.js#30 In general, exceptions thrown in tests will be safe to change. > > services/common/rest.js > > 284:7 error Expected an object to be thrown. no-throw-literal (eslint) > > 300:7 error Expected an object to be thrown. no-throw-literal (eslint) ... Apart from tests, the vast majority of these seem to be more like assertions than expected exceptions, so are probably safe - checking searchfox for the string literal will generally show they are never caught. If we can get this down to just a handful requiring // eslint comments I'd be inclined to think that it's worth continuing down this route - but I don't want to turn this into a massive can-of-worms that makes you regret getting involved ;) Thanks!
Assignee | ||
Comment 9•7 years ago
|
||
Second pass. Addressed comments, fixed more occurrences in services/, and fixed a failing test in test_ext_storage_sync_crypto.js (from the trybot). The one that has me worried is the exception in FxAccountsClient.jsm, since it's harder to know where it gets used (if at all): http://searchfox.org/mozilla-central/source/services/fxaccounts/FxAccountsClient.jsm#602 Also, Mark, what do you think of Edouard's comment about creating our own Error subclasses? I think it would be more robust than comparing strings. > we should instead create our own Error subclass for this case, which would > make our error matching cleaner
Attachment #8889112 -
Attachment is obsolete: true
Comment 10•7 years ago
|
||
(In reply to Nicolas Ouellet-Payeur from comment #9) > The one that has me worried is the exception in FxAccountsClient.jsm, since > it's harder to know where it gets used (if at all): > http://searchfox.org/mozilla-central/source/services/fxaccounts/ > FxAccountsClient.jsm#602 I think it's fine and safer to add an // eslint decoration there. > Also, Mark, what do you think of Edouard's comment about creating our own > Error subclasses? I think it would be more robust than comparing strings. > > we should instead create our own Error subclass for this case, which would > > make our error matching cleaner That sounds fine, although I think you may end up with issues if the error is thrown and compared in different modules (but IIRC, Ed's comment involved throwing and comparing in the same module, so *should* be fine)
Assignee | ||
Comment 11•7 years ago
|
||
Third pass. Reverted FxAccounts.jsm, added an `eslint-disable-next-line` comment, and added two custom exception types. On an unrelated note, since this is my first contribution, what's the usual way to go through code review? Re-uploading the whole patch makes it difficult to track changes between each pass, but incremental patches would be a headache... Or do people normally use MozReview?
Attachment #8889745 -
Attachment is obsolete: true
Reporter | ||
Comment 12•7 years ago
|
||
Comment on attachment 8890233 [details] [diff] [review] 1375212.patch Review of attachment 8890233 [details] [diff] [review]: ----------------------------------------------------------------- This looks in great shape, thank you. I left some comments there and there. ::: services/sync/modules/engines/prefs.js @@ +153,4 @@ > try { > this._prefs.set(pref, value); > } catch (ex) { > + this._log.trace("Failed to set pref: ", pref, ": ", ex); (1) Try to use literals in cases similar to this and keep these log functions within 2 arguments. Also since we are not concatenating strings anymore, you can drop the colon. e.g this._log.trace(`Failed to set pref: ${pref}`, ex); ::: services/sync/modules/record.js @@ +570,4 @@ > try { > payload = storage_keys.decrypt(syncKeyBundle); > } catch (ex) { > + log.warn("Got exception \"", ex, "\" decrypting storage keys with sync key."); Ditto (1), just put the ex as the last arg: log.warn("Got exception decrypting storage keys with sync key.", ex) ::: services/sync/modules/resource.js @@ +215,4 @@ > }, > > _onComplete(ex, data, channel) { > + this._log.trace("In _onComplete. Error is ", ex, "."); Ditto (1) ::: services/sync/modules/service.js @@ +257,4 @@ > return false; // Don't try again: same keys. > > } catch (ex) { > + this._log.warn("Got exception \"", ex, "\" fetching and handling " + Ditto (1) @@ +575,4 @@ > return false; > } > } catch (ex) { > + this._log.warn("Got exception \"", ex, "\" fetching cryptoKeys."); Ditto (1) ::: services/sync/modules/util.js @@ +115,4 @@ > }; > }, > > + throwLockException(label) { This is only used once right? You can probably inline this.
Reporter | ||
Comment 13•7 years ago
|
||
> On an unrelated note, since this is my first contribution, what's the usual way to go through code review? Re-uploading the whole patch makes it difficult to track changes between each pass, but incremental patches would be a headache... Or do people normally use MozReview?
MowReview is pretty good for that, because it does show differences within versions of a same commit.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8890473 -
Flags: review?(markh)
Attachment #8890473 -
Flags: review?(eoger)
Assignee | ||
Comment 15•7 years ago
|
||
Fixed the log messages per Edouard's comment.
> This is only used once right?
It's also used in test_utils_catch.js. Left it like this for now.
Reporter | ||
Updated•7 years ago
|
Attachment #8890233 -
Attachment is obsolete: true
Reporter | ||
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8890473 [details] Bug 1375212 - Wrap thrown strings in Error objects https://reviewboard.mozilla.org/r/161552/#review166996 LGTM, thank you! Let's make sure we run this on try before we land it.
Attachment #8890473 -
Flags: review?(eoger) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8890473 [details] Bug 1375212 - Wrap thrown strings in Error objects https://reviewboard.mozilla.org/r/161552/#review167110 Awesome - thanks - and extra thanks for fixing a few of those logging calls! ::: services/sync/tests/unit/test_utils_catch.js:6 (Diff revision 1) > Cu.import("resource://services-sync/util.js"); > Cu.import("resource://services-sync/service.js"); > > add_task(async function run_test() { > _("Make sure catch when copied to an object will correctly catch stuff"); > - let ret, rightThis, didCall, didThrow, wasTen, wasLocked; > + let ret, rightThis, didCall, didThrow, wasCovfefe, wasLocked; lol
Attachment #8890473 -
Flags: review?(markh) → review+
Comment 18•7 years ago
|
||
Pushed by mhammond@skippinet.com.au: https://hg.mozilla.org/integration/autoland/rev/b936f241e649 Wrap thrown strings in Error objects r=eoger,markh
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b936f241e649
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in
before you can comment on or make changes to this bug.
Description
•