Closed Bug 1375212 Opened 7 years ago Closed 7 years ago

Wrap thrown Strings in Error objects

Categories

(Firefox :: Sync, enhancement, P3)

enhancement

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..
Summary: Wrap throw Strings in Error objects → Wrap thrown Strings in Error objects
We should ensure that this doesn't change how telemetry reports these errors.
Mentor: eoger
Keywords: good-first-bug
Priority: -- → P3
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?
(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)
Attached patch 1375212.patch (obsolete) — Splinter Review
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&regexp=true&path=services%2Fsync

I'd like some feedback & guidance on what to do next, to ensure stability.
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 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+
Assignee: nobody → nicolaso
> 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)
(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!
Attached patch 1375212.patch (obsolete) — Splinter Review
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
(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)
Attached patch 1375212.patch (obsolete) — Splinter Review
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
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.
> 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.
Attachment #8890473 - Flags: review?(markh)
Attachment #8890473 - Flags: review?(eoger)
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.
Attachment #8890233 - Attachment is obsolete: true
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 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+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/b936f241e649
Wrap thrown strings in Error objects r=eoger,markh
https://hg.mozilla.org/mozilla-central/rev/b936f241e649
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.