Closed
Bug 911947
Opened 11 years ago
Closed 11 years ago
[WAP push] DUT can still receive same SI ID created earlier than already in device one.
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(blocking-b2g:koi+, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 verified)
Tracking | Status | |
---|---|---|
b2g18 | --- | unaffected |
b2g-v1.1hd | --- | unaffected |
b2g-v1.2 | --- | verified |
People
(Reporter: echu, Assigned: gsvelto)
References
Details
(Keywords: late-l10n, Whiteboard: [FT:RIL, burirun2])
Attachments
(3 files, 5 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
DUT can still receive same SI ID created earlier than already in device one. * Build Number Gaia: 9fb5802df60a9081846d704def01df814ed8fbd4 Gecko: http://hg.mozilla.org/mozilla-central/rev/b6c29e434519 BuildID 20130901040215 Version 26.0a1 * Reproduce Steps 1. Send SI with SI ID:abc, create date 2013-09-03T17:35:33Z from NowSMS to DUT. 2. Send another SI with same ID, but create date 2013-09-03T10:35:33Z * Expected Result DUT won't receive 2nd older message. * Actual Result DUT still receive it. * Occurrence rate 100%.
User story define in bug 891248, comment 8.
Updated•11 years ago
|
blocking-b2g: koi? → koi+
Updated•11 years ago
|
Assignee: nobody → gsvelto
Assignee | ||
Comment 2•11 years ago
|
||
This patch is a follow up to bug 911247 and applies on top of the patch for that bug. This adds parsing the optional 'si-id' and 'created' fields and uses them to handle out-of-order delivery of messages. Old messages are automatically expired when replaced by newer ones or ignored if a new one has already arrived. The patch also contains unit-tests covering the additional functionality.
Attachment #811195 -
Flags: review?(felash)
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•11 years ago
|
||
Comment on attachment 811195 [details] [diff] [review] [PATCH] Handle out-of-order message reception Clearing the review as I'll be refreshing this to accomodate for changes in bug 911247.
Attachment #811195 -
Flags: review?(felash)
Assignee | ||
Comment 4•11 years ago
|
||
Refreshed patch, applies on top of attachment 812573 [details] [diff] [review].
Attachment #811195 -
Attachment is obsolete: true
Assignee | ||
Comment 5•11 years ago
|
||
Updated patch rebased on top of attachment 813010 [details] [diff] [review].
Attachment #812620 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
Rebased on top of attachment 814826 [details] [diff] [review]. This implements out-of-order message handling and replacement according to the 'si-id' (identification) and 'created' (creation time fields). The behavior is described here (paragraph 5.2.1 and 6.2 articles 2 and 3): http://is.gd/JnXr1a The complete flow is shown here (paragraph 6.2): http://is.gd/3jJNcf In a nutshell this inspect messages and checks if they have si-id and created fields. If they have both messages the time stored in the created field is used to establish if the message is new (no other messages have been received with that si-id, or only older messages) or if it's obsolete (we've already seen a message with the same si-id and it's creation time is newer). Depending on this condition previously received messages can be expired and newly received ones can be ignored if they're already obsolete. Message ordering and replacement is done within the database logic to enforce the atomicity of the operation and avoid race conditions. Unit-tests are provided.
Attachment #813029 -
Attachment is obsolete: true
Attachment #814928 -
Flags: review?(felash)
Assignee | ||
Comment 8•11 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•11 years ago
|
Attachment #814929 -
Attachment description: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12755 → [PULL REQUEST] Handle out-of-order message reception
Comment 9•11 years ago
|
||
Comment on attachment 814928 [details] [diff] [review] [PATCH v4] Handle out-of-order message reception Review of attachment 814928 [details] [diff] [review]: ----------------------------------------------------------------- looks good, a few comments still. Not for this bug, because we run out of time, but I still don't like the very tightly coupled logic in MessageDB, I think we can do better. I'm thinking of an promise-like API like this: DB.transaction().then(...).then(...).finish(...). "then" callbacks would be called in "onsuccess", so won't end the transaction. "finish" callbacks would be called in "oncomplete", so at the end of the transaction. A .then callback could pass parameters to the next .then callback, in addition to the transaction object. Thought I could share it with you now, so that you can think about it in background :) For your follow-ups, please keep your existing commit and push on github a single new commit, + your full patch here if you want. Both are good for me, but the single commit makes it obvious to see what changed :) Thanks ! ::: apps/wappush/js/messagedb.js @@ +87,5 @@ > * @param {Function} error A callback invoked if an operation fails. > */ > function mdb_put(message, success, error) { > mdb_open(function mdb_putCallback(db) { > + var state = { status: 'new' }; I don't get why you have an object and not just a string here ? ::: apps/wappush/js/parsed_message.js @@ +118,5 @@ > + } > + > + // 'created' attribute, optional, date in ISO 8601 format > + if (indicationNode.hasAttribute('created')) { > + obj.created = Date.parse(indicationNode.getAttribute('created')); note that Date.parse is more permissive that what your wap push spec says. Is it ok for you to accept time zones for example ? Also, you should do : `if (isNaN(obj.create)) { obj.create = undefined; }` in case `Date.parse` returns NaN. (doing `Date.parse(...) || undefined` will be incorrect for the epoch time :-) ) Last but not least, Date.parse returns a timezone-dependant timestamp. That means that if the user changes his timezone between 2 message receptions, your comparison will be wrong. So it's maybe better to do `new Date(value).getTime()`. This still returns NaN for badly formed strings, so my previous comments still hold. ::: apps/wappush/js/wappush.js @@ +139,5 @@ > } > > message.save( > + function wpm_saveSuccess(status) { > + if ((status === 'new') || (status === 'updated')) { better do an early return here, if the status is different of both values @@ +163,5 @@ > + self.close(/* background */ true); > + }; > + req.onerror = function wpm_getAppError() { > + self.close(/* background */ true); > + }; seems to me this req is out of the if block @@ +164,5 @@ > + }; > + req.onerror = function wpm_getAppError() { > + self.close(/* background */ true); > + }; > + } add .bind(this) at the end of the function (so: `message.save(function() { ... }.bind(this))`) and you can use `this` instead of `self` inside the function. Same for all nested functions of course. Might also be a good idea to extract this function to a top level function. ::: apps/wappush/test/unit/mock_messagedb.js @@ +37,5 @@ > + } else { > + messages.push(message); > + } > + > + success(status); I think you're throwing too much logic here. Instead, from the test, you should spy on this, and call `spy.yield(value)` with whatever value you want to test your code for. This has a lot of advantages over your code: 1 - it is really asynchronous-like, like the reality: the callback is called only after the code after the "MessageDB.put" call has run. 2 - you can test very easily different cases. 3 - you're not testing yor mock (which is what you do when you have too much logic in it) @@ +51,4 @@ > } > } > > + success(null); check if you have the callback before calling it ::: apps/wappush/test/unit/parsed_message_test.js @@ +58,5 @@ > + ' created="2013-09-03T10:35:33Z">' + > + 'check this out' + > + '</indication>' + > + '</si>' > + }, one case with a bad `created` attribute would be good too, Date.parse should throw or whatever in that case :) @@ +104,5 @@ > + > + test('SI message with explicit si-id field', function() { > + var message = ParsedMessage.from(messages.si_id, timestamp); > + > + assert.equal(message.id, 'gaia-test@mozilla.org'); please test href too here
Attachment #814928 -
Flags: review?(felash)
Assignee | ||
Comment 10•11 years ago
|
||
Here's an updated patch with all your comments addressed (more below): (In reply to Julien Wajsberg [:julienw] from comment #9) > Not for this bug, because we run out of time, but I still don't like the > very tightly coupled logic in MessageDB, I think we can do better. I'm > thinking of an promise-like API like this: > > DB.transaction().then(...).then(...).finish(...). > > "then" callbacks would be called in "onsuccess", so won't end the > transaction. > "finish" callbacks would be called in "oncomplete", so at the end of the > transaction. > > A .then callback could pass parameters to the next .then callback, in > addition to the transaction object. > > Thought I could share it with you now, so that you can think about it in > background :) Sounds like a good idea for a generalized IDB helper, I'll try to implement something like that in the future. > I don't get why you have an object and not just a string here ? I can't remember why I did it either... I replaced it with a string. > note that Date.parse is more permissive that what your wap push spec says. > Is it ok for you to accept time zones for example ? Yeah because I'll never get one, we generate the date internally in that format so there's no need to check it: http://dxr.mozilla.org/mozilla-central/source/dom/wappush/src/gonk/SiPduHelper.jsm#l35 > Last but not least, Date.parse returns a timezone-dependant timestamp. That > means that if the user changes his timezone between 2 message receptions, > your comparison will be wrong. So it's maybe better to do `new > Date(value).getTime()`. I just did that, nice catch! > better do an early return here, if the status is different of both values Done. > add .bind(this) at the end of the function (so: `message.save(function() { > ... }.bind(this))`) and you can use `this` instead of `self` inside the > function. Same for all nested functions of course. Done, it's a long chain of bind()s but if it's preferred to the 'var self = this' style I don't mind. > I think you're throwing too much logic here. [snip] Got rid of all that stuff and used spy.yield() instead as you suggested :) > check if you have the callback before calling it Done. > one case with a bad `created` attribute would be good too, Date.parse should > throw or whatever in that case :) I didn't do it as it should not happen (and if it does then it's a Gecko issue). > please test href too here Done.
Attachment #814928 -
Attachment is obsolete: true
Attachment #821079 -
Flags: review?(felash)
Comment 11•11 years ago
|
||
Comment on attachment 821079 [details] [diff] [review] [PATCH v5] Handle out-of-order message reception Review of attachment 821079 [details] [diff] [review]: ----------------------------------------------------------------- Next time, please generate patches with 8 lines of context ;) |git format-patch -U8| works fine, but I couldn't find how to make it the default. r=me with the comments adressed and provided a green Travis. But feel free to ask me another review if you don't feel confident about the tests. ::: apps/wappush/test/unit/wappush_test.js @@ +173,5 @@ > MockNavigatormozSetMessageHandler.mTrigger('wappush-received', message); > }); > > + teardown(function() { > + MockMessageDB.clear(); I'd create a `mTeardown` function in MockMessageDB, that would call `clear`. and `mTeardown` functions are called automatically by the mocks helper (when you use mocks that are handled by the mocks helper), so you don't even need to call it yourself :) @@ +241,5 @@ > + WapPushManager.init(done); > + }); > + > + suiteTeardown(function() { > + MockMessageDB.clear(); you don't need to do this if you create a mTeardown function in this mock. @@ +244,5 @@ > + suiteTeardown(function() { > + MockMessageDB.clear(); > + MockNavigatormozApps.mTeardown(); > + MockNavigatorSettings.mTeardown(); > + MockNavigatormozSetMessageHandler.mTeardown(); The name of these functions should have been a good reminder: they're supposed to be called in a `teardown` function, so that every test start with a clean state. @@ +249,5 @@ > + }); > + > + test('the old message is expired', function() { > + var putSpy = this.sinon.spy(MockMessageDB, 'put'); > + var retrieveSpy = this.sinon.spy(MockMessageDB, 'retrieve'); I'd put all the spies in the `setup` function, so that you don't clobber the various test functions, and make it clearer to read. And I wouldn't use variables either, for example using `MockMessageDB.retrieve.yield(null)` works fine and is imo more explicit.
Attachment #821079 -
Flags: review?(felash) → review+
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #11) > Next time, please generate patches with 8 lines of context ;) |git > format-patch -U8| works fine, but I couldn't find how to make it the default. I just added these lines to my .gitconfig file and they seem to do the job: [diff] context = 8 > r=me with the comments adressed and provided a green Travis. But feel free > to ask me another review if you don't feel confident about the tests. I've addressed all the changes you requested in the review and especially the unit-test so I think we're good. I have learned a few more things about how to use the mocks and Sinon in this bug :-) The Travis run is here, I'll wait for it to be green before merging: https://travis-ci.org/mozilla-b2g/gaia/builds/13042724
Attachment #821079 -
Attachment is obsolete: true
Comment 13•11 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #12) > [diff] > context = 8 This works for diff but it didn't work for format-patch for me. But maybe they fixed it ?
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #13) > This works for diff but it didn't work for format-patch for me. But maybe > they fixed it ? I've got version 1.8.3.1 FYI.
Assignee | ||
Comment 15•11 years ago
|
||
Pushed to master and v1.2 gaia/master 1b2f11a0ad9a0deca867547be1a8cf95e2757d4a gaia/v1.2 14f3f4e7b65ff79ef131ce4d3a8a3e45cab2d9d9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-b2g18:
--- → unaffected
status-b2g-v1.1hd:
--- → unaffected
status-b2g-v1.2:
--- → fixed
Resolution: --- → FIXED
Reporter | ||
Comment 16•11 years ago
|
||
Verified on Buri Gaia: df049e3177ced0ca493ff0d192c65f18392bb462 Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/93eafd042c1c BuildID 20131031004003 Version 26.0
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
Target Milestone: --- → 1.2 C4(Nov8)
You need to log in
before you can comment on or make changes to this bug.
Description
•