1072244 - Correctly throw the exceptions in TPS framework

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

While investigating bug 1066493, I found out that in sync sometimes instead of throwing exceptions we throw strings, those lead to unexpected behavior when rethrowing the exceptions in TPS due to fact that string objects don't have a "message" attribute.
Example of throwing:
http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/service.js?rev=da30483ac566#994
Example of consumer;
http://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/tps/resource/auth/fxaccounts.jsm?rev=6196c121c4f4#93

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

Henrik, I didn't know who to tag for review so I added you, if you can't land in m-c I'll add the keyword "checkin-needed" once the review is done.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8494470 [details] [diff] [review]
patch v1.0

Review of attachment 8494470 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8494470 [details] [diff] [review]
patch v1.0

Review of attachment 8494470 [details] [diff] [review]:
-----------------------------------------------------------------

I cannot review this patch. But given that Mark already did it, lets get a try run to see if all is fine, and nothing gets regressed.

https://tbpl.mozilla.org/?tree=Try&rev=98302ec5db19

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.1

I had a syntax error, it failed in try build and I fixed it.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8497305 [details] [diff] [review]
patch v1.1

Review of attachment 8497305 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/common/utils.js
@@ +307,4 @@
>        function advance() {
>          c  = str[cOffset++];
>          if (!c || c == "" || c == "=") // Easier than range checking.
> +          throw new Error("Done");     // Will be caught far away.

line 358 catches this "Done" and it just reads:

        if (ex == "Done")

So I suspect that will need to change too.

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

I fixed that, I didn't saw similar issues.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

I changed that. I didn't saw similar situations.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8497314 [details] [diff] [review]
patch v2.0

Review of attachment 8497314 [details] [diff] [review]:
-----------------------------------------------------------------

it looks like you uploaded the same patch twice?

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Yes, I had multiple tabs open ... sorry for noise

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8497313 [details] [diff] [review]
patch v2.0

Review of attachment 8497313 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like the change as done here might still cause some problems if we have dedicated catch conditions. So inspecting the code would be kinda important, to ensure nothing slips through. I feel it has a high risk of causing breakage somewhere in the sync modules. Not sure how complete the unit tests are for sync, so we could catch everything?

::: services/common/utils.js
@@ +359,2 @@
>            break;
>          throw ex;

A better way to simplify the logic would be:

catch (ex if ex.message == "Done") {
  break;
}

That way we also do not have to re-throw the exception, which might modify the stack.

::: services/sync/tps/extensions/mozmill/resource/stdlib/EventUtils.js
@@ +482,4 @@
>      if (aDOMKeyCode.indexOf("VK_") == 0) {
>        aDOMKeyCode = KeyEvent["DOM_" + aDOMKeyCode];
>        if (!aDOMKeyCode) {
> +        throw new Error("Unknown key: " + aDOMKeyCode);

Please do not modify resources of Mozmill! So please remove those changes to EventUtils.js and httpd.js.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1

I changed what was requested, I searched to see if we try to catch the errors we rise in a different place and we don't.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I pushed a try build for now:
https://tbpl.mozilla.org/?tree=Try&rev=7c172e0d40e8

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I had to push to try again given that inbound was busted at the last time:
https://tbpl.mozilla.org/?tree=Try&rev=6d16904d0842

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

All green on TBPL, only known issues.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8497406 [details] [diff] [review]
patch v2.1

Review of attachment 8497406 [details] [diff] [review]:
-----------------------------------------------------------------

I do not see why all should be green. I see dozen of failures in XPCShell tests caused by those changes here. Have you run those tests locally on your machine? You should really do that. Even better is to run all the tests of the component you are making changes in. Here are clearly changes to tests necessary.

Comment 16

[Richard Newman [:rnewman]]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Not sure how complete the
> unit tests are for sync, so we could catch everything?

Sync does not have very complete unit test coverage. Not terrible, but far from the coverage you'd need to confidently make changes to error handling.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

(In reply to Richard Newman [:rnewman] from comment #16)
> Sync does not have very complete unit test coverage. Not terrible, but far
> from the coverage you'd need to confidently make changes to error handling.

Although to be fair, the vast majority of these exceptions are clearly not designed to be caught by anyone - IOW, they seem more like assertions.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So to be a bit more helpful, here an example why we fail now:
http://mxr.mozilla.org/mozilla-central/search?string=Unknown%2Bcharacter%2Bin%2Bbase32

The unit tests directly run tests against the formerly thrown string, which is an Error now. So updating the tests to the following will fix it:

do_check_eq(err.message, "Unknown character in base32: 0");

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

I made fixes in all affected xpcshell tests. All testes ran well locally now.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8498180 [details] [diff] [review]
patch v3.0

Review of attachment 8498180 [details] [diff] [review]:
-----------------------------------------------------------------

Just some nits I spotted when looking over. Please update the patch and request review from Mark again. I can only give feedback on that code. Once the patch is up lets do another try run to be safe.

::: services/sync/modules/util.js
@@ +109,5 @@
>    },
>  
>    isLockException: function isLockException(ex) {
> +    let message = String(ex);
> +    return message.indexOf("Could not acquire lock.") !== -1;

You can use `ex.message.indexOf` directly here.

@@ +243,5 @@
>  
>    isHMACMismatch: function isHMACMismatch(ex) {
>      const hmacFail = "Record SHA256 HMAC mismatch: ";
> +    var error = String(ex);
> +    return error.indexOf(hmacFail) != -1;

Same here.

::: services/sync/tests/unit/test_utils_lock.js
@@ +62,4 @@
>    }
>    catch(ex) {
>      // Should throw an Error, not a string.
> +    do_check_begins(ex.message, "Could not acquire lock");

You can also use `Utils.isLockException()` here. Also I think the comment can be removed.

Comment 21

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> >    isLockException: function isLockException(ex) {
> > +    let message = String(ex);
> > +    return message.indexOf("Could not acquire lock.") !== -1;
> 
> You can use `ex.message.indexOf` directly here.
Not all exceptions are strings, I force cast them so I can use indexOf safely without other checks, what if we have null/undefined/object? That won't work, it fails in other tests. 

> >    catch(ex) {
> >      // Should throw an Error, not a string.
> > +    do_check_begins(ex.message, "Could not acquire lock");
> 
> You can also use `Utils.isLockException()` here. Also I think the comment
> can be removed.
That seems to be okey.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Cosmin Malutan from comment #21)
> (In reply to Henrik Skupin (:whimboo) from comment #20)
> > >    isLockException: function isLockException(ex) {
> > > +    let message = String(ex);
> > > +    return message.indexOf("Could not acquire lock.") !== -1;
> > 
> > You can use `ex.message.indexOf` directly here.
> Not all exceptions are strings, I force cast them so I can use indexOf
> safely without other checks, what if we have null/undefined/object? That
> won't work, it fails in other tests. 

When will we not throw a real exception? I thought you converted all of them from strings to Error. In those cases an Error object should always exist, which has a message property. Even with a message being empty, the indexOf() works because it's a string. Or do I miss something?

Comment 23

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> > > > +    return message.indexOf("Could not acquire lock.") !== -1;
> > > 
> > > You can use `ex.message.indexOf` directly here.
> > Not all exceptions are strings, I force cast them so I can use indexOf
> > safely without other checks, what if we have null/undefined/object? That
> > won't work, it fails in other tests. 
> 
> When will we not throw a real exception? I thought you converted all of them
> from strings to Error. In those cases an Error object should always exist,
> which has a message property. Even with a message being empty, the indexOf()
> works because it's a string. Or do I miss something?
In Utils there is a catch and re-throw which I don't fully understands how it works but the exception from there doesn't have a message attribute.
https://github.com/mozilla/gecko-dev/blob/GECKO50_2011051718_RELBRANCH/services/sync/modules/util.js#L102
https://github.com/mozilla/gecko-dev/blob/GECKO50_2011051718_RELBRANCH/services/sync/modules/util.js#L124

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The only thing I see which would need an update is:
https://github.com/mozilla/gecko-dev/blob/GECKO50_2011051718_RELBRANCH/services/sync/modules/util.js#L128

Here we shouldn't throw a string neither. All the rest only wraps a function which can throw an exception, so it can be handled at a single place.

Comment 25

[Cosmin Malutan, [:cosmin-malutan]]

I did that with an exception thrown I get the same result, I will use Utils.exceptionStr in library, it's safe and used somewhere else.

Comment 26

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.1

Here is the patch all tests passes locally.

Comment 27

[Richard Newman [:rnewman]]

Comment on attachment 8498782 [details] [diff] [review]
patch v3.1

Review of attachment 8498782 [details] [diff] [review]:
-----------------------------------------------------------------

A few drive-by comments from the module owner :P

::: services/common/utils.js
@@ +307,4 @@
>        function advance() {
>          c  = str[cOffset++];
>          if (!c || c == "" || c == "=") // Easier than range checking.
> +          throw new Error("Done");     // Will be caught far away.

I don't see a point in this change; it's a control-flow thing that we can statically determine never escapes from decodeBase32, not an error. You're instantiating a new object for no reason.

::: services/sync/modules/util.js
@@ +242,5 @@
>  
>    isHMACMismatch: function isHMACMismatch(ex) {
>      const hmacFail = "Record SHA256 HMAC mismatch: ";
> +
> +    return Utils.exceptionStr(ex).indexOf(hmacFail) != -1;

This is a particularly heavyweight and unpleasant way of doing this check.

You just need to pair up isHMACMismatch and throwHMACMismatch.

If you're touching these two functions (and I don't see the latter in this patch!), why not do what the comment says?!

  // Generator and discriminator for HMAC exceptions.
  // Split these out in case we want to make them richer in future, and to
  // avoid inevitable confusion if the message changes.

Make them richer objects.

::: services/sync/tests/unit/test_service_sync_locked.js
@@ +28,4 @@
>    Service.sync();
>    Service._locked = false;
>  
> +  do_check_true(Utils.isLockException(debug[debug.length - 2]));

This is wrong. The test was checking that we were locked in login; now you're just checking that we were locked at all.

Comment 28

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0

(In reply to Richard Newman [:rnewman] from comment #27)
> ::: services/sync/modules/util.js
> @@ +242,5 @@
> >  
> >    isHMACMismatch: function isHMACMismatch(ex) {
> >      const hmacFail = "Record SHA256 HMAC mismatch: ";
> > +
> > +    return Utils.exceptionStr(ex).indexOf(hmacFail) != -1;
> 
> This is a particularly heavyweight and unpleasant way of doing this check.
> 
> You just need to pair up isHMACMismatch and throwHMACMismatch.
Done that by setting/checking the error type: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/name
 
> > +  do_check_true(Utils.isLockException(debug[debug.length - 2]));
> 
> This is wrong. The test was checking that we were locked in login; now
> you're just checking that we were locked at all.
When throwing an exception instead of a string we will have the stack of error so the method we used before won't work anymore as the string my differ, but I see your point, and now I check for inclusion of the string we care about.

Comment 29

[Richard Newman [:rnewman]]

Comment on attachment 8499409 [details] [diff] [review]
patch v4.0

Review of attachment 8499409 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK, but it doesn't look high-value to me.

I'd be inclined to wait to land this until the start of the 36 cycle, just to make sure you aren't backing stuff out of Aurora for breaking some error path that wasn't covered by tests.

::: services/sync/modules/util.js
@@ +237,4 @@
>    // Split these out in case we want to make them richer in future, and to
>    // avoid inevitable confusion if the message changes.
>    throwHMACMismatch: function throwHMACMismatch(shouldBe, is) {
> +    var e = new Error("Record SHA256 HMAC mismatch: should be " + shouldBe +

`let` not `var`, always always always.

::: services/sync/tests/unit/test_service_sync_locked.js
@@ +28,4 @@
>    Service.sync();
>    Service._locked = false;
>  
> +  let debugEx = Utils.exceptionStr(debug[debug.length - 2]);

I don't like the call to exceptionStr here. What's wrong with ex.message?

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Richard Newman [:rnewman] from comment #29)
> 
> This looks OK, but it doesn't look high-value to me.
> 
> I'd be inclined to wait to land this until the start of the 36 cycle, just
> to make sure you aren't backing stuff out of Aurora for breaking some error
> path that wasn't covered by tests.
Those changes are indeed error prone and they might not be covered by the tests, and I won't want to take the responsibility for braking something in someones else code. I'l obsolete this patch and change the way we throw the exception solely in TPS, which is not called by other components, and won't broke anything. 

> > +  let debugEx = Utils.exceptionStr(debug[debug.length - 2]);
> 
> I don't like the call to exceptionStr here. What's wrong with ex.message?
That was because we re-throw the exception and it won't have an message attribute anyway, but as I said I'll drop this change, the purpose of this patch was to throw exception correctly so we can now what failed in TPS (bug 1066493), for which we would need only tho modify the TPS.

Comment 31

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 0001-Bug-1072244-Correctly-throw-the-exceptions-in-TPS-fr.patch

Whimboo that's all we need for our dependency bug 1066493.
I won't change the code in other modules as is error-prone and can't broke things that are not covered by the tests, also changing only the TPS framework won't affect any other module because is not called from outside.

All tests passed.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8503047 [details] [diff] [review]
0001-Bug-1072244-Correctly-throw-the-exceptions-in-TPS-fr.patch

Review of attachment 8503047 [details] [diff] [review]:
-----------------------------------------------------------------

That all makes sense to me. I did a further check and found the following files where we also throw atomic objects: tps-cmdline.js, bookmarks.jsm

Please also check addons.jsm for 'throw Error'. There is a missing 'new'. Maybe even other modules are affected.

Comment 33

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 0001-Bug-1072244-Correctly-throw-the-exceptions-in-TPS-fr.patch

(In reply to Henrik Skupin (:whimboo) from comment #32)
> That all makes sense to me. I did a further check and found the following
> files where we also throw atomic objects: tps-cmdline.js, bookmarks.jsm
I thought we might catch those, that's why they were constants, but after a quick search we don't.

> Please also check addons.jsm for 'throw Error'. There is a missing 'new'.
> Maybe even other modules are affected.

Done. 
Thanks

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8503964 [details] [diff] [review]
0001-Bug-1072244-Correctly-throw-the-exceptions-in-TPS-fr.patch

Review of attachment 8503964 [details] [diff] [review]:
-----------------------------------------------------------------

The changes are looking fine now. I assume you have tested all this, so I will land the patch on inbound.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/048495545352

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/048495545352

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/14f066441ebb
https://hg.mozilla.org/releases/mozilla-beta/rev/48e3c2f927d5