Closed
Bug 1072244
Opened 10 years ago
Closed 10 years ago
Correctly throw the exceptions in TPS framework
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: cosmin-malutan, Assigned: cosmin-malutan)
References
Details
(Whiteboard: [sprint])
Attachments
(1 file, 9 obsolete files)
4.29 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8494470 -
Flags: review?(mhammond)
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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•10 years ago
|
||
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 5•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
I changed that. I didn't saw similar situations.
Attachment #8497314 -
Flags: review?(mhammond)
Updated•10 years ago
|
Attachment #8497314 -
Flags: review?(mhammond) → review+
Comment 8•10 years ago
|
||
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•10 years ago
|
||
Yes, I had multiple tabs open ... sorry for noise
Assignee | ||
Updated•10 years ago
|
Attachment #8497314 -
Attachment is obsolete: true
Comment 10•10 years ago
|
||
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-
Assignee | ||
Comment 11•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
I pushed a try build for now: https://tbpl.mozilla.org/?tree=Try&rev=7c172e0d40e8
Comment 13•10 years ago
|
||
I had to push to try again given that inbound was busted at the last time: https://tbpl.mozilla.org/?tree=Try&rev=6d16904d0842
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•10 years ago
|
||
All green on TBPL, only known issues.
Comment 15•10 years ago
|
||
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-
Comment 16•10 years ago
|
||
(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•10 years ago
|
||
(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•10 years ago
|
||
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");
Assignee | ||
Comment 19•10 years ago
|
||
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 20•10 years ago
|
||
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+
Assignee | ||
Comment 21•10 years ago
|
||
(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•10 years ago
|
||
(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?
Assignee | ||
Comment 23•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 25•10 years ago
|
||
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.
Assignee | ||
Comment 26•10 years ago
|
||
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 27•10 years ago
|
||
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-
Assignee | ||
Comment 28•10 years ago
|
||
(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)
Updated•10 years ago
|
Whiteboard: [sprint]
Comment 29•10 years ago
|
||
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+
Assignee | ||
Comment 30•10 years ago
|
||
(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
Assignee | ||
Comment 31•10 years ago
|
||
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 32•10 years ago
|
||
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-
Assignee | ||
Comment 33•10 years ago
|
||
(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 34•10 years ago
|
||
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+
Comment 35•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/048495545352
status-firefox33:
--- → wontfix
status-firefox34:
--- → affected
status-firefox35:
--- → affected
Target Milestone: --- → mozilla36
Comment 36•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/048495545352
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 37•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/14f066441ebb https://hg.mozilla.org/releases/mozilla-beta/rev/48e3c2f927d5
Updated•10 years ago
|
status-firefox36:
--- → fixed
Updated•6 years ago
|
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.
Description
•