942080 - [WAP push] New SI message should replace received one with same SI-ID.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Wappush, defect)

Description

[Enpei]

Attachment: time stamp: 15:42, SI message with "second" text content.

When device receive a SI message which created date is newer than received one with same SI-ID, old one will be replaced.

* Build Number                
Gaia:     ce276842c9ac1746073271fb736dfdb626a89240
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/36c4c667b9f2
BuildID   20131121004002
Version   26.0

* Reproduce Steps
1. Send a SI with SI-ID and created date, ex:2013-11-22T04:33:33Z
2. Send a SI with same SI-ID and created date is later than last one, ex:2013-11-22T05:33:33Z

* Expected Result
Original one will be replaced and removed from utility tray.

* Actual Result
Original one is marked as expired.

* Occurrence rate
100%

Comment 1

[Gabriele Svelto [:gsvelto]]

Blocking on bug 938540 as we need the new notification API to implement this.

Comment 2

[Jason Smith [:jsmith]]

Not a system bug - moving into the Gaia component, until we get a bugzilla component created for WAP Push.

Comment 3

[Kai-Chih Hu [:khu]]

Joe, can you help to triage? Thanks.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

triage: should not block release. bug 938540 needs to land first before working on this one

Comment 7

[:gerard-majax]

In bug 938540 we are now making use of the timestamp as a tag, so I think this bug will be solved at the same time.

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Alexandre LISSY :gerard-majax from comment #7)
> In bug 938540 we are now making use of the timestamp as a tag, so I think
> this bug will be solved at the same time.

No this requires a further modification: when the database logic detects that an existing message is being replaced it should look up the notification with the tag corresponding to the old message, and update it. There's already some scaffolding for this (we correctly detect updated messages) but we were still missing the use of the new API without which updating a notification is impossible.

Comment 11

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Convert asynchronous activities to use promises and use the new functionality to update old notifications

This is a large patch that looks real scary but I swear it's not complicated, honest :)

In a nutshell this patch takes all asynchronous activities from the MessageDB & ParsedMessage objects that took success/error callback pairs and turns them into promises. The reason why the patch is large is that I had to indent a lot of mostly unchanged code when creating the promises.

The tests were heavily modified to account for this change; they're now almost all asynchronous (and don't have to use the callback-pyramid-of-death). I've extended the coverage of certain tests and removed a couple of redundant ones too.

As for the actual changes MessageDB.put() now doesn't return a string with the status of the message (new/updated/discarded) but the message itself (if it's new), a modified copy of it (if it updated an existing message) or null (if it was expired / discarded). This makes the existing code for sending notification update the existing notifications w/o requiring significant changes. It already works pretty much as-is. The new use case is also covered with an additional unit-test.

Comment 12

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8426339 [details] [diff] [review]
[PATCH] Convert asynchronous activities to use promises and use the new functionality to update old notifications

Clearing the review flag, this needs another spin.

Comment 13

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Convert asynchronous activities to use promises and use the new functionality to update old notifications

I've reworked the patch and made it apply on top of the fixes for bug 1014638 and bug 1014706 because they're both smaller and they're both regressions so I'd like to have them fixed first.

This patch cuts a couple of hundred lines of changes from the previous version by being a bit smarter with promise chains and reducing the amount of modifications needed in the code.

The major highlights of this patch are the following:

- All asynchronous methods now use promises, this include methods in WapPushManager where promises are only used for testing.

- The resolved value of a new message promise is now used to update the notification thus fixing this bug. Unit-tests were added to cover this scenario.

- All unit-tests now leverage the promise-based architecture to ensure proper completion of the tested activities. This is not only more robust but allows us to remove a lot of mocks and increase coverage. The number of unit tests was reduced because a few that shared the initialization phase are now consolidated into a single activity that checks functionality end-to-end.

Comment 14

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Convert asynchronous activities to use promises and use the new functionality to update old notifications

Third iteration, further reduces the amount of changes required to the code.

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v4] Convert asynchronous activities to use promises and use the new functionality to update old notifications

This patch should be ready for prime time, here's a quick summary of what's going on here since the patch is fairly large (though I trimmed it down as much as possible):

- All asynchronous methods now use promises (in WapPushManager, ParsedMessage and MessageDB). WapPushManager now exposes some additional methods which return promises. These methods have been modified and made visible outside of WapPushManager for use in the unit-tests. The fact that they use promises make the tests asynchronous (which is the intended behavior), more robust and with better coverage.

- When saving a new message (ParsedMessage.save) we return a promise that resolves to the status of the new message. The underlying method (MessageDB.put) was also modified to update the underlying message. These changes are used to fix the issue here and this scenario has been covered with unit-tests.

- All unit-tests now leverage the promise-based architecture to ensure proper completion of the tested activities. This is not only more robust but allows us to remove a lot of mocks/stubs and increase coverage. The number of unit tests was reduced because a few that shared the initialization phase are now consolidated into a single test that checks functionality end-to-end.

- Comments throughout the code have been adjusted to reflect the use of promises in various methods.

Comment 16

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Convert asynchronous activities to use promises and use the new functionality to update old notifications

And here's the PR.

Comment 17

[Julien Wajsberg [:julienw]]

I think the description in comment 0 is rather obsolete now, can you please tell me the failing STR?

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8432637 [details] [diff] [review]
[PATCH v4] Convert asynchronous activities to use promises and use the new functionality to update old notifications

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

First of all, congrats, you know why :)

Wondering if SMS' task runner might be of some help for you: [1]. I remember we have some tricks to prevent running different indexeddb operations at once, so maybe it would be easier to run the tasks sequentially?

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/task_runner.js

Here is a review of the promise-based refactoring which looks very good. I'll wait for the updated STR to review the (small) functional changes.

::: apps/wappush/js/messagedb.js
@@ +28,2 @@
>      if (db) {
> +      return Promise.resolve(db);

nit: empty line

@@ +47,5 @@
>  
> +      try {
> +        req = indexedDB.open(DB_NAME, DB_VERSION);
> +      } catch (e) {
> +        reject(e.message);

better reject with the error itself.

Now, I don't know what happens if you just don't use a try/catch: do we get a rejected promise.
I know that's what happens in "then" callback, but I don't know what happens in the Promise resolver.

So, either throwing here "just works" (means: creates a rejected promise) and you can remove the try/catch block; either it doesn't and you can either keep the try/catch or use a "Promise.resolve().then(function...)" construct (but since there are more asynchronous stuff later where you'll have to use the Promise constructor anyway, I'm fine with the try/catch block, provided we need it).

@@ +56,5 @@
> +        db = event.target.result;
> +        resolve(db);
> +      };
> +      req.onerror = function mdb_openError(event) {
> +        reject(event.target.error.name);

reject with the error itself

@@ +116,5 @@
> +        transaction.oncomplete = function mdb_putComplete(event) {
> +          resolve(status);
> +        };
> +        transaction.onerror = function mdb_putError(event) {
> +          reject(event.target.error.name);

please reject with the error

@@ +117,5 @@
> +          resolve(status);
> +        };
> +        transaction.onerror = function mdb_putError(event) {
> +          reject(event.target.error.name);
> +        };

so, we see a lot of this construct in this file (starting a transaction and returning a promise from its oncomplete/onerror handlers). How about extract this in a separate function?

@@ +191,5 @@
>        if (cursor) {
>          cursor.delete();
>          cursor.continue();
>        }
>      };

so, no error anymore?
Or does it work automatically thanks to the transaction.onerror?

@@ +213,5 @@
> +        transaction.oncomplete = function mdb_retrieveComplete() {
> +          resolve(state.message);
> +        };
> +        transaction.onerror = function mdb_retrieveError(event) {
> +          reject(event.target.error.name);

please reject with the error itself

@@ +250,5 @@
> +        transaction.oncomplete = function mdb_clearComplete() {
> +          resolve();
> +        };
> +        transaction.onerror = function mdb_clearError(event) {
> +          reject(event.target.error.name);

please reject with the error itself

::: apps/wappush/js/parsed_message.js
@@ +80,4 @@
>       */
> +    save: function pm_save() {
> +      return MessageDB.put(this.toJSON()).then(function(result) {
> +        return Promise.resolve(result);

this function is quite useless; just return MessageDB.put(...) :)

@@ +215,4 @@
>     */
> +  ParsedMessage.load = function pm_load(timestamp) {
> +    return MessageDB.retrieve(timestamp).then(function(message) {
> +      return Promise.resolve(message ? new ParsedMessage(message) : null);

just return the value itself, you don't need to wrap in a Promise:

::: apps/wappush/js/wappush.js
@@ +262,5 @@
>          }
>  
>          wpm_sendNotification(message);
>          wpm_finish();
> +        return Promise.resolve();

you don't really need to return something, it will do the same.

But it's a style, so do as you want here.

@@ +267,2 @@
>        },
> +      function wpm_saveRejected(error) {

I think it's better to call .catch(function wpm_saveRejected...) after the .then, because this way, if wpm_sendNotification and wpm_finish fail, you'll be able to log the failure.

@@ +269,3 @@
>          console.log('Could not add a message to the database: ' + error + '\n');
>          wpm_finish();
> +        return Promise.reject(error);

I think this is more readable to rethrow error (and this will do the same).

@@ +307,5 @@
> +      }
> +    ).then(function wpm_gotNotification(notifications) {
> +      for (var i = 0; i < notifications.length; i++) {
> +        notifications[i].close();
> +      }

how about extracting Notification.get + notifications...close in a separate function (returning a promise obviously)?

Here, you're doing 2 separate things in this function, but closing the notification is related to getting it earlier :)

"closeNotificationsForTimestamp" would work for me ;)

@@ +326,1 @@
>        }

this part could also be a separate function: populateScreenForMessage(message)

@@ +326,3 @@
>        }
> +
> +      return Promise.resolve();

same here: you don't need to return a Promise here.

@@ +326,4 @@
>        }
> +
> +      return Promise.resolve();
> +    });

we don't do much with the returned promise, so you should probably catch any error somewhere, either here or in the callers. (I would do it here because it's simpler)

::: apps/wappush/test/unit/messagedb_test.js
@@ +95,5 @@
>      }
>    };
>  
> +  setup(function(done) {
> +    MessageDB.clear().then(done);

.then(done, done)

@@ +119,5 @@
>      });
>  
>      test('message is removed after retrieval', function(done) {
> +      MessageDB.put(messages.current).then(function() {
> +        return MessageDB.retrieve(messages.current.timestamp);

nit: .then(MessageDB.retrieve.bind(MessageDB, messages.current.timestamp))

(not sure it's better, so it's your call)

::: apps/wappush/test/unit/wappush_test.js
@@ +15,5 @@
>  require('/shared/test/unit/mocks/mock_navigator_moz_icc_manager.js');
>  require('/shared/test/unit/mocks/mock_navigator_moz_settings.js');
>  
>  require('/js/cp_screen_helper.js');
> +require('/js/messagedb.js');

Why don't you want to use a mock anymore?

I perfectly agree that the mock can be greatly reduced now, but I think it still makes sense to keep the complicated messagedb file out of this unit test file.

You can use sinon to control what sort of promises messagedb functions return.

@@ +97,5 @@
>      MockNavigatorMozIccManager.addIcc(0, {});
>      MockNavigatormozSetMessageHandler.mSetup();
> +    WapPushManager.init().then(function() {
> +      MessageDB.clear().then(done, done);
> +    });

nit: doesn't this work:

  WapPushManager.init().then(MessageDB.clear.bind(MessageDB)).then(done, done)

?

@@ +133,5 @@
> +
> +      WapPushManager.onWapPushReceived(message).then(function() {
> +        done(function checks() {
> +          sinon.assert.notCalled(MessageDB.put);
> +        });

it's easier to use something like this:

  asynchronous().then(function() {
    assertStuff();
  }).then(done, done);

(obviously this is for all the tests)

@@ +160,2 @@
>  
> +      WapPushManager.onWapPushReceived(message).then(function() {

This is your call, but using MockNavigatormozSetMessageHandler actually tests that your code properly registers the message handler.

but this is really your call, I won't block on this.

@@ +172,5 @@
>        container = screen.querySelector('.container');
>        text = container.querySelector('p');
>        link = container.querySelector('a');
>  
> +      this.sinon.stub(Date, 'now').returns(0);

maybe using sinon's fake timers would be more natural?

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v5] Convert asynchronous activities to use promises and use the new functionality to update old notifications

This patch addresses nearly all the comments in Julien's previous review and rebases the entire patch on top of master, reworking the new unit-tests and adding coverage.

Steve, since Julien's on PTO I'm asking you for review even if this is outside the SMS app because he did most of the reviews in that area since WAP Push is some kind of a spin-off the SMS app.

Some background to explain what's going on since the patch is large. WAP Push messages go through a certain logic whenever they are received. Normally when a message is received a notification is sent after the message has been stored in the internal database. Depending on certain fields in the message (date of creation, id, action, etc...) it is possible that a new message is actually an update to an older one. In this case we update the notification of the previous message with the new one.

This patch is quite large because besides fixing this specific problem it converts all asynchronous activities to use promises. The change is basically mechanical and the logic is otherwise unaffected but it might seem daunting at first. Unfortunately this was required because we had some nasty cases of callback indentation hell and fixing this functionality w/o promises would have made them worse.

BTW this patch is part of my personal backlog so it's not urgent, take your time.

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v6] Convert asynchronous activities to use promises and use the new functionality to update old notifications

Whoops, sorry, wrong patch attached. See comment 19 for more details. BTW the attached PR is always updated to have the latest patch if you prefer doing the review on GitHub.

Comment 21

[Steve Chung [:steveck]]

Comment on attachment 8476810 [details] [diff] [review]
[PATCH v6] Convert asynchronous activities to use promises and use the new functionality to update old notifications

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

Looks good for me overall, but seems you forgot to modify one of the API to promise structure. Please update it (and unit test part) for the next review, thanks!

::: apps/wappush/js/messagedb.js
@@ +41,5 @@
> +
> +  /**
> +   * Opens the database, creates it if it has not been created already. Returns
> +   * a promise that is resolved with the value of the database object or
> +   * rejected with an error message.

nit: rejected with an error object?

@@ +61,4 @@
>      }
>  
>      if (!indexedDB) {
> +      return Promise.reject('IndexedDB is not available');

maybe we should also put error object created with error message instead

@@ +69,5 @@
>  
> +      try {
> +        req = indexedDB.open(DB_NAME, DB_VERSION);
> +      } catch (e) {
> +        reject(e.message);

Please reject with error object

::: apps/wappush/js/parsed_message.js
@@ +228,4 @@
>    };
>  
>    ParsedMessage.delete = function pm_delete(timestamp, success, error) {
>      MessageDB.deleteByTimestamp(timestamp, success, error);

Since you apply Promise in mdb_deleteByTimestamp, this delete function should update the structure as well.

Comment 22

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v7] Convert asynchronous activities to use promises and use the new functionality to update old notifications

Thanks for the review Steve, this is an updated patch with your comments addressed, the PR was also updated. Regarding the ParsedMessage.delete() you made me realize that the method was superfluous (it was duplicating the functionality of MessageDB.deleteByTimestamp) and didn't really belong inside the ParsedMessage object. I've replaced its uses with MessageDB.deleteByTimestamp() and since the latter returns a promise I've used it to correct racy behavior that didn't wait for the deletion to complete before moving to the next step.

Comment 23

[Steve Chung [:steveck]]

Comment on attachment 8479757 [details] [diff] [review]
[PATCH v7] Convert asynchronous activities to use promises and use the new functionality to update old notifications

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

Since this patch is quite huge and you already met the original requirement without significant issue, r=me with some nits in testing part. But for the unit test part, it would be better that mocking the parsedMessage, so you don't have to require real messagedb for testing. And other nice-to-have thing is creating tests for the helpers instead of testing all the logic inside wappush_test.js, but you could plan it in the future.

::: apps/wappush/test/unit/messagedb_test.js
@@ +124,5 @@
> +      }).then(function(message) {
> +        return MessageDB.retrieve(messages.current.timestamp);
> +      }).then(function(message) {
> +        done(function checks() {
> +          assert.equal(message, null);

nit: use assert.isNull

@@ +185,5 @@
> +        done(function checks() {
> +          assert.equal(results.status, 'updated');
> +          assert.equal(results.timestamp, messages.current.timestamp);
> +          assert.equal(results.text, messages.newer.text);
> +          assert.equal(message, null);

nit: use assert.isNull

@@ +201,5 @@
> +        return MessageDB.retrieve(messages.older.timestamp);
> +      }).then(function(message) {
> +        done(function checks() {
> +          assert.equal(results.status, 'discarded');
> +          assert.equal(message, null);

nit: use assert.isNull

@@ +219,5 @@
> +        return MessageDB.retrieve(messages.current.timestamp);
> +      }).then(function(message) {
> +        done(function checks() {
> +          assert.equal(results.status, 'discarded');
> +          assert.equal(message, null);

nit: use assert.isNull. And please adjust the following isNull assertions

::: apps/wappush/test/unit/wappush_test.js
@@ +332,5 @@
> +
> +            assert.equal(title.textContent, messages.netwpin.sender);
> +            assert.isFalse(acceptButton.classList.contains('hidden'),
> +              'the accept button should be visible');
> +            assert.equal(acceptButton.hidden, false);

nit : assert.isFalse

@@ +358,5 @@
> +
> +            assert.equal(title.textContent, messages.userpin.sender);
> +            assert.isFalse(acceptButton.classList.contains('hidden'),
> +              'the accept button should be visible');
> +            assert.equal(acceptButton.hidden, false);

nit : assert.isFalse

@@ +524,1 @@
>        this.clock = this.sinon.useFakeTimers();

nit : you could declare clock outside and simply set clock = this.sinon.useFakeTimers().

Comment 24

[Gabriele Svelto [:gsvelto]]

(In reply to Steve Chung [:steveck] from comment #23)
> Since this patch is quite huge and you already met the original requirement
> without significant issue, r=me with some nits in testing part. But for the
> unit test part, it would be better that mocking the parsedMessage, so you
> don't have to require real messagedb for testing. And other nice-to-have
> thing is creating tests for the helpers instead of testing all the logic
> inside wappush_test.js, but you could plan it in the future.

Thanks for the review Steve, I've addressed the nits and will be merging the patch as soon as Try turns green. The tests in wappush_test.js leverage the existing code instead of mocks because they work almost as integration tests since they're exercising the whole stack. I'll file a follow up to simplify them using mocks and add proper integration tests instead.

Comment 25

[Gabriele Svelto [:gsvelto]]

Green try run (save for a harness issue and an intermittent, sigh): https://tbpl.mozilla.org/?rev=76187c60710c518d412a79fb478d20cc45e81209&tree=Gaia-Try

Pushed to gaia/master 84a5bf6632d559bb690d2134fc4b9ef19d9b78cc

https://github.com/mozilla-b2g/gaia/commit/84a5bf6632d559bb690d2134fc4b9ef19d9b78cc

Comment 26

[Hubert Lu[:hlu] <[e-mail]>]

Partner reports this issue also happened on Woodduck (2.0M), so change status-b2g-v2.0M to affected and blocking-b2g to 2.0M?.

   Gaia      891dadee6803f84b7782ab37fd32a29e05663794
   Gecko     f47b9d18f2cf88702d9dc079a7393a95b10b0125
   BuildID   20140915161205
   Version   32.0
   Device Name 	 Woodduck
   FW-Release 	 4.4.2
   FW-Incremental 	 1410746885
   FW-Date 	 Mon Sep 15 10:08:15 CST 2014

Comment 27

[Kai-Zhen Li [:seinlin][:kli]]

Hi Gabriele, it is conflict when cherr-pick the patch to 2.0m. Could you make a patch for 2.0m? Thanks!

Comment 28

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Uplifted patch for the v2.0m branch

Here's an uplifted patch that applies cleanly to the v2.0m branch. The conflicts were caused by the changes in bug 974984 and bug 1015300. They were both non-functional changes so I could remove them easily while keeping the patch working.

Comment 29

[Gabriele Svelto [:gsvelto]]

Pushed to gaia/v2.0m b337a8839d6302c19469512dd3bb4a5f6d06e300

https://github.com/mozilla-b2g/gaia/commit/b337a8839d6302c19469512dd3bb4a5f6d06e300

Comment 30

[Josh Cheng [:josh]]

Hi Norry,
Please help to verify this bug is fix in Woodduck 2.0M and Flame 2.1. Thanks!

Comment 32

[Eric]

Hi Hubert,

As we talked on Skype, please help to verify it, thanks a lot.

Comment 33

[Norry.L.F(Leave from Mozilla)]

(In reply to Eric from comment #32)
> Hi Hubert,
> 
> As we talked on Skype, please help to verify it, thanks a lot.

Comment 34

[Gabriele Svelto [:gsvelto]]

This bug has been public for a long time, I don't think marking it as confidential makes sense.

Comment 35

[Julien Wajsberg [:julienw]]

Josh, can we remove the confidential status for this bug? It has a landed patch, I think this is inappropriate.

Comment 36

[Josh Cheng [:josh]]

(In reply to Julien Wajsberg [:julienw] from comment #35)
> Josh, can we remove the confidential status for this bug? It has a landed
> patch, I think this is inappropriate.

Sorry my bad, accidentally mark it confidential.

Comment 37

[sarsenyev]

Verified the issue is fixed on 2.2, 2.1 and 2.0
Newer SMS with later time stamp replays the older SMS and removes it from the notification tray

Device: Flame 2.2 Master KK
BuildID: 20141030040201
Gaia: 0dfc1996eb583c8b507a82bf6b8319624bba23ea
Gecko: 80e18ff7c7b2
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1
BuildID: 20141030001201
Gaia: 3e585d8be5e2dffc376f83313299c9b6d53c3db4
Gecko: b643d78a23c6
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.0
BuildID: 20141030000200
Gaia: 7b8df9941700c1f6d6d51ff464f0c8ae32008cd2
Gecko: acf80e04b4d5
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 38

[Hubert Lu[:hlu] <[e-mail]>]

Verified the issue is fixed on Woodduck 2.0m.

== Build Information ==
Gaia-Rev        fdb8236d7d99061ef6bedc021fd6b482e1af3f5a
Gecko-Rev       3d95c1683ef5eb017bd15bc38ba111ddb1ad792e
Build-ID        20141024050313
Version         32.0
Device-Name     soul35
FW-Release      4.4.2
FW-Incremental  1414098327
FW-Date         Fri Oct 24 05:05:49 CST 2014