Correctly throw the exceptions in TPS framework

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: cosmin-malutan, Assigned: cosmin-malutan)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [sprint])

Attachments

(1 attachment, 9 obsolete attachments)

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
(Assignee)

Comment 1

4 years ago
Created attachment 8494470 [details] [diff] [review]
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.
Attachment #8494470 - Flags: review?(hskupin)
Attachment #8494470 - Flags: review?(mhammond)
Comment on attachment 8494470 [details] [diff] [review]
patch v1.0

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

LGTM
Attachment #8494470 - Flags: review?(mhammond) → review+
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
Attachment #8494470 - Flags: review?(hskupin)
(Assignee)

Comment 4

4 years ago
Created attachment 8497305 [details] [diff] [review]
patch v1.1

I had a syntax error, it failed in try build and I fixed it.
Attachment #8494470 - Attachment is obsolete: true
Attachment #8497305 - Flags: review?(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.
(Assignee)

Comment 6

4 years ago
Created attachment 8497313 [details] [diff] [review]
patch v2.0

I fixed that, I didn't saw similar issues.
Attachment #8497305 - Attachment is obsolete: true
Attachment #8497305 - Flags: review?(mhammond)
Attachment #8497313 - Flags: review?(mhammond)
(Assignee)

Comment 7

4 years ago
Created attachment 8497314 [details] [diff] [review]
patch v2.0

I changed that. I didn't saw similar situations.
Attachment #8497314 - Flags: review?(mhammond)
Attachment #8497314 - Flags: review?(mhammond) → review+
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?
Attachment #8497314 - Flags: review+
(Assignee)

Comment 9

4 years ago
Yes, I had multiple tabs open ... sorry for noise
(Assignee)

Updated

4 years ago
Attachment #8497314 - Attachment is obsolete: true
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.
Attachment #8497313 - Flags: review-
Created attachment 8497406 [details] [diff] [review]
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.
Attachment #8497313 - Attachment is obsolete: true
Attachment #8497313 - Flags: review?(mhammond)
Attachment #8497406 - Flags: review?(hskupin)
I had to push to try again given that inbound was busted at the last time:
https://tbpl.mozilla.org/?tree=Try&rev=6d16904d0842
Status: NEW → ASSIGNED
All green on TBPL, only known issues.
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.
Attachment #8497406 - Flags: review?(hskupin) → review-
(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.
(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.
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");
Created attachment 8498180 [details] [diff] [review]
patch v3.0

I made fixes in all affected xpcshell tests. All testes ran well locally now.
Attachment #8497406 - Attachment is obsolete: true
Attachment #8498180 - Flags: review?(hskupin)
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.
Attachment #8498180 - Flags: review?(hskupin) → feedback+
(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.
(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?
(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
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.
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.
Created attachment 8498782 [details] [diff] [review]
patch v3.1

Here is the patch all tests passes locally.
Attachment #8498180 - Attachment is obsolete: true
Attachment #8498782 - Flags: review?(mhammond)
Attachment #8498782 - Flags: review?(hskupin)
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.
Attachment #8498782 - Flags: review?(mhammond)
Attachment #8498782 - Flags: review?(hskupin)
Attachment #8498782 - Flags: review-
Created attachment 8499409 [details] [diff] [review]
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.
Attachment #8498782 - Attachment is obsolete: true
Attachment #8499409 - Flags: review?(rnewman)
Whiteboard: [sprint]
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?
Attachment #8499409 - Flags: review?(rnewman) → review+
(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.
Summary: Correctly throw the exceptions in sync:backend /services/sync/ → Correctly throw the exceptions in TPS framework
Created attachment 8503047 [details] [diff] [review]
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.
Attachment #8499409 - Attachment is obsolete: true
Attachment #8503047 - Flags: review?(hskupin)
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.
Attachment #8503047 - Flags: review?(hskupin) → review-
Created attachment 8503964 [details] [diff] [review]
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
Attachment #8503047 - Attachment is obsolete: true
Attachment #8503964 - Flags: review?(hskupin)
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.
Attachment #8503964 - Flags: review?(hskupin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/048495545352
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → affected
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/048495545352
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-firefox36: --- → fixed
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.