911944 - [WAP push] Device can still display SI message with "none" action type.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Enpei]

Attachment: 17:29.

Device can still receive SI message with "none" action type. It violate bug 891248, comment 8.

* Build Number                
Gaia:     9fb5802df60a9081846d704def01df814ed8fbd4
Gecko:    http://hg.mozilla.org/mozilla-central/rev/b6c29e434519
BuildID   20130901040215
Version   26.0a1

* Reproduce Steps
1. Send WAP push SI message with signal action "none" from NowSMS to DUT.

* Expected Result
DUT cannot receive the message.

* Actual Result
DUT can still receive the message.

* Occurrence rate
100%

Comment 1

[Kai-Chih Hu [:khu]]

Expected result: DUT does not have to display.

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH WIP] Handle message actions

Work in progress patch, this applies on top of attachment 813500 [details] [diff] [review] and implements default actions for SI and SL messages plus delete behavior for SI messages. signal-none actions are still missing as well as more extensive unit-tests.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH WIP] Handle message actions

Updated patch, this handles action=signal-none messages and improves the path for action=delete ones. Unit-tests are still missing though.

Comment 4

[bhavana bajaj [:bajaj]]

NI :gsvelto to help with  next steps here given this is a koi+ and has been untouched for a while.

Can you please help understand if this is still a blocker and help set the 1.2 target milestone to get a eta her ?

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

I've got a patch ready for this; I was just waiting for bug 911934 to land before landing it. This is the last of the long chain of WAP push bugs that I've unraveled one step at a time and will be closed by the end of the week.

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Handle message actions

This is the final patch for fully implementing SI/SL message functionality, it deals with message actions as described in WAP-167 5.2.1 [http://is.gd/qEMwTc] and in WAP-168 5.2 [http://is.gd/mE0QRt].

I've added unit-tests covering the new functionality but also covered the MessageDB functions that were not covered (we didn't have any tests for that code in fact so it was due).

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Handle message actions

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 824703 [details] [diff] [review]
[PATCH] Handle message actions

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

review finished, please request review again once you're ready!

Could be a good idea to fix the jshint warnings too! Since the app is quite small, it's probably not a big work to do. Doing this in another bug is fine to me.

::: apps/wappush/js/messagedb.js
@@ +126,5 @@
> +             * delete action remove all messages with the same ID including
> +             * the received message itself. The user will not be notified. */
> +            if (message.action === 'delete') {
> +              status = 'discarded';
> +              mdb_deleteById(transaction, message.id, error);

I may be wrong here, but I thought we can have only one message with a specific id (because you're deleting it). I agree your index is not ensuring this (and maybe it should?) but that's what your code is doing.

If I read the workflow in the spec correctly, when message.action is delete, then you should just not put the message in the store. Therefore I'd just check this condition before calling `store.put(message)` above, set `status = 'updated'` only when you "put" the message.

@@ +164,5 @@
> +        cursor.continue();
> +      }
> +    };
> +    req.onerror = function mdb_openCursorError(event) {
> +      error(event.target.errorCode);

I think it's really `event.target.error.name`, I know MDN is probably wrong here.

Could you try to confirm?

(don't bother if you end up deleting this entire function)

::: apps/wappush/js/parsed_message.js
@@ +154,5 @@
>        }
> +
> +      /* 'action' attribute, optional, string, defaults to 'signal-medium' when
> +       * not present in the incoming message, see WAP-167 7.2 */
> +      if (indicationNode.hasAttribute('action')) {

I really think you should have a whitelist here. If it's not known, either ignore the message or ignore the attribute.

(with added unit tests of course ;) )

@@ +162,5 @@
> +      }
> +
> +      /* If the message has a 'delete' action but no 'si-id' field than it's
> +       * malformed and should be immediately discarded, see WAP-167 6.2 */
> +      if ((obj.action === 'delete') && !obj.id) {

nit: you don't need these parenthesis here.

@@ +174,5 @@
>        obj.href = slNode.getAttribute('href');
> +
> +      /* 'action' attribute, optional, string, defaults to 'execute-low' when
> +       * not present in the incoming message, see WAP-168 5.2 */
> +      if (slNode.hasAttribute('action')) {

you should have a whitelist here too. Imagine you have a "delete", that would break in the next code.

::: apps/wappush/js/wappush.js
@@ +123,5 @@
>      }
>  
>      message.save(
>        (function wpm_saveSuccess(status) {
> +        if (((status !== 'new') && (status !== 'updated')) ||

checking whether `status === 'discarded'` could be enough, right ?

@@ +124,5 @@
>  
>      message.save(
>        (function wpm_saveSuccess(status) {
> +        if (((status !== 'new') && (status !== 'updated')) ||
> +            (message.action === 'signal-none')) {

Mmm do you really want to save if message.action is 'signal-none' ? I mean, the spec says "we may be doing something with this message", but currently we do nothing, and we could just discard it, right?

Maybe handle 'signal-none' and 'delete' in the same way (currently) ?

::: apps/wappush/test/unit/messagedb_test.js
@@ +61,5 @@
> +              assert.equal(message.type, messages.current.type);
> +              assert.equal(message.sender, messages.current.sender);
> +              assert.equal(message.href, messages.current.href);
> +              assert.equal(message.text, messages.current.text);
> +              done();

it's a good idea to do this instead:

function retrieveSuccess(message) {
  done(function() {
    // assertions
  });
}

This is an improvement done in the test-agent, that let mocha catch the exception if one assertion is failing, and show a correct stack trace.

Same for the other asynchronous tests of course.

::: apps/wappush/test/unit/parsed_message_test.js
@@ +74,5 @@
> +      action: {
> +        sender: '[phone]',
> +        contentType: 'text/vnd.wap.si',
> +        content: '<si>' +
> +                 '<indication action="none">' +

isn't it 'signal-none' ?

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Could be a good idea to fix the jshint warnings too! Since the app is quite
> small, it's probably not a big work to do. Doing this in another bug is fine
> to me.

I'll do that. It's not much work and it will put us in a good position going forward.

> I may be wrong here, but I thought we can have only one message with a
> specific id (because you're deleting it). I agree your index is not ensuring
> this (and maybe it should?) but that's what your code is doing.

Yes, that's correct, that's what my code is doing... and it's wrong :-( The idea is that a message with a 'si-id' field is replaced only if it has a 'created' field that shows it's newer than an existing one. Otherwise if it has only the 'si-id' field it should not be replaced. The bit doing this reads:

if (storedMessage.created && message.created &&
    (storedMessage.created < message.created)) {
  // Update existing message
} else {
  // Discard incoming message
}

But it should actually be:

if (storedMessage.created && message.created) {
  if (storedMessage.created < message.created) {
    // Update existing message
  } else {
    // Discard incoming message
  }
} else {
  // Insert incoming message
}

I'll fix it and I'll add a unit-test to cover this scenario.

> If I read the workflow in the spec correctly, when message.action is delete,
> then you should just not put the message in the store. Therefore I'd just
> check this condition before calling `store.put(message)` above, set `status
> = 'updated'` only when you "put" the message.

It's fairly subtle but the message needs to be inserted first because the delete action in the flow happens only after the steps 2 and 3 that deal with message age and replacement. So let's say you get a delete message with a 'created' date that is older than an existing message it will be discarded and the delete message won't be executed.

> @@ +164,5 @@
> > +        cursor.continue();
> > +      }
> > +    };
> > +    req.onerror = function mdb_openCursorError(event) {
> > +      error(event.target.errorCode);
> 
> I think it's really `event.target.error.name`, I know MDN is probably wrong
> here.

Yeah, I've just double-checked and that's a DOMError object so definitely event.target.error.name. I'll fix MDN before it confuses more people :-/

> ::: apps/wappush/js/parsed_message.js
> @@ +154,5 @@
> >        }
> > +
> > +      /* 'action' attribute, optional, string, defaults to 'signal-medium' when
> > +       * not present in the incoming message, see WAP-167 7.2 */
> > +      if (indicationNode.hasAttribute('action')) {
> 
> I really think you should have a whitelist here. If it's not known, either
> ignore the message or ignore the attribute.

With this patch we're handling all the different attributes that could appear in the message as per spec. The only missing part is the 'info' element which is implementation specific and can be ignored at will. For the rest we know we will never get messages with other attributes as those wouldn't pass through the DTD verification of the original message so there's no point in adding logic for that IMHO.

> ::: apps/wappush/js/wappush.js
> @@ +123,5 @@
> >      }
> >  
> >      message.save(
> >        (function wpm_saveSuccess(status) {
> > +        if (((status !== 'new') && (status !== 'updated')) ||
> 
> checking whether `status === 'discarded'` could be enough, right ?

Yeah :)

> @@ +124,5 @@
> >  
> >      message.save(
> >        (function wpm_saveSuccess(status) {
> > +        if (((status !== 'new') && (status !== 'updated')) ||
> > +            (message.action === 'signal-none')) {
> 
> Mmm do you really want to save if message.action is 'signal-none' ? I mean,
> the spec says "we may be doing something with this message", but currently
> we do nothing, and we could just discard it, right?

Yes but only under certain conditions. If it has a 'si-id' and a 'created' field then it can have the side-effect of making further events with the same 'si-id' be dropped if they have an older 'created' date. So yeah, I could check those two fields and drop those messages early if the don't have them.

> Maybe handle 'signal-none' and 'delete' in the same way (currently) ?

That wouldn't work because delete behaves differently depending on its 'created' field presence & value.

> it's a good idea to do this instead:
> 
> function retrieveSuccess(message) {
>   done(function() {
>     // assertions
>   });
> }
> 
> This is an improvement done in the test-agent, that let mocha catch the
> exception if one assertion is failing, and show a correct stack trace.
> 
> Same for the other asynchronous tests of course.

Nice, I'll do that!

> ::: apps/wappush/test/unit/parsed_message_test.js
> @@ +74,5 @@
> > +      action: {
> > +        sender: '[phone]',
> > +        contentType: 'text/vnd.wap.si',
> > +        content: '<si>' +
> > +                 '<indication action="none">' +
> 
> isn't it 'signal-none' ?

Yup, nice catch.

I'll post a revised patch soon.

Comment 10

[Julien Wajsberg [:julienw]]

(In reply to Gabriele Svelto [:gsvelto] from comment #9)

> > If I read the workflow in the spec correctly, when message.action is delete,
> > then you should just not put the message in the store. Therefore I'd just
> > check this condition before calling `store.put(message)` above, set `status
> > = 'updated'` only when you "put" the message.
> 
> It's fairly subtle but the message needs to be inserted first because the
> delete action in the flow happens only after the steps 2 and 3 that deal
> with message age and replacement. So let's say you get a delete message with
> a 'created' date that is older than an existing message it will be discarded
> and the delete message won't be executed.

My proposition was to do it inside the "created" if-block. But now that you'll modify it, I'll have a look again in the new code :)

> 
> With this patch we're handling all the different attributes that could
> appear in the message as per spec. The only missing part is the 'info'
> element which is implementation specific and can be ignored at will. For the
> rest we know we will never get messages with other attributes as those
> wouldn't pass through the DTD verification of the original message so
> there's no point in adding logic for that IMHO.

We're doing a DTD verification in Gecko?
If yes then we're probably safe, right.

> > Mmm do you really want to save if message.action is 'signal-none' ? I mean,
> > the spec says "we may be doing something with this message", but currently
> > we do nothing, and we could just discard it, right?
> 
> Yes but only under certain conditions. If it has a 'si-id' and a 'created'
> field then it can have the side-effect of making further events with the
> same 'si-id' be dropped if they have an older 'created' date. So yeah, I
> could check those two fields and drop those messages early if the don't have
> them.
> 
> > Maybe handle 'signal-none' and 'delete' in the same way (currently) ?
> 
> That wouldn't work because delete behaves differently depending on its
> 'created' field presence & value.

Oki, I get it, that makes sense, thanks!

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] Handle message actions and increase test coverage

This is the revised patch; it's grown quite big because of the jshint fixes. Those fixes are largely aesthetic and functionality has not been touched. The only functional changes are those requested in the review in comment 8 and the fix I described in comment 9 when handling messages with a 'si-id' field but no 'created' field.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 826923 [details] [diff] [review]
[PATCH v2] Handle message actions and increase test coverage

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

For next time, I think it's better to fix the jshint issues in its own bug/commit. But lets do this here this time.

Some issues here.
Please rebase your github PR and see if travis turns green.

Please request r again once you're ready :)

::: apps/wappush/js/cp_screen_helper.js
@@ +242,5 @@
>      // If the document has been already authenticated and there are no errors,
>      // show the settings we are about to store into the settings database.
>      message = storeConfirmDialog.querySelector('.message');
>      var msg = '';
> +    for (var j = 0; i < names.length; j++) {

|j < names.length|

or better: do a "var i" outside of the for loop, and reuse the "i" variable.

::: apps/wappush/js/wappush.js
@@ +135,3 @@
>      message.save(
>        (function wpm_saveSuccess(status) {
> +        if ((status === 'discarded') || (message.action === 'signal-none')) {

don't you want to delete it if action is "signal-none" ?

::: apps/wappush/test/unit/activity_picker_test.js
@@ +1,2 @@
> +/* -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- /
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */

We don't need to put these vim-mode lines. You can keep them if you want though, I don't mind.

::: apps/wappush/test/unit/wappush_test.js
@@ +8,3 @@
>  'use strict';
>  
> +mocha.setup({ ignoreLeaks: true });

you can do:

  mocha.globals(['WapPushManager']);

better than ignoring all leaks

That said, I don't get why we load this file after the mocks.

Is it because the "load" event gets sent, and therefore the `init` function is called too soon?

Maybe you'd need to move the "load" event handler out of wappush.js and put it in a startup.js file, so that in this test file you can simply call `init` when you want.

This could be for another patch though.

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v3] Handle message actions and increase test coverage

(In reply to Julien Wajsberg [:julienw] from comment #12)
> For next time, I think it's better to fix the jshint issues in its own
> bug/commit. But lets do this here this time.

Thanks for the review and sorry for making it huge, I couldn't resist :)

> Some issues here.
> Please rebase your github PR and see if travis turns green.

Yes, I did it and now travis is green (except for unrelated failures).

> or better: do a "var i" outside of the for loop, and reuse the "i" variable.

I just did that in the refreshed patch.

> ::: apps/wappush/js/wappush.js
> @@ +135,3 @@
> >      message.save(
> >        (function wpm_saveSuccess(status) {
> > +        if ((status === 'discarded') || (message.action === 'signal-none')) {
> 
> don't you want to delete it if action is "signal-none" ?

No, because of the out-of-order replacement logic. If a signal-none message made it up to that point it means it have the 'si-id' and 'created' fields set and thus "counts" as a valid message for ones that might be received later.

> ::: apps/wappush/test/unit/activity_picker_test.js
> @@ +1,2 @@
> > +/* -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- /
> > +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> 
> We don't need to put these vim-mode lines. You can keep them if you want
> though, I don't mind.

I did because some files had them and some didn't and in general it helps new contributors keeping indentation consistent.

> That said, I don't get why we load this file after the mocks.
> 
> Is it because the "load" event gets sent, and therefore the `init` function
> is called too soon?
> 
> Maybe you'd need to move the "load" event handler out of wappush.js and put
> it in a startup.js file, so that in this test file you can simply call
> `init` when you want.

That's an excellent idea actually and I did just that decoupling the WapPushManager initialization from the wappush.js file. I also noticed that I had left an unused HTML file in the app, message.html, so I got rid of that too.

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Gabriele Svelto [:gsvelto] from comment #13)
> > 
> > don't you want to delete it if action is "signal-none" ?
> 
> No, because of the out-of-order replacement logic. If a signal-none message
> made it up to that point it means it have the 'si-id' and 'created' fields
> set and thus "counts" as a valid message for ones that might be received
> later.

oki, got it completely this time, thanks!

> 
> > ::: apps/wappush/test/unit/activity_picker_test.js
> > @@ +1,2 @@
> > > +/* -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- /
> > > +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> > 
> > We don't need to put these vim-mode lines. You can keep them if you want
> > though, I don't mind.
> 
> I did because some files had them and some didn't and in general it helps
> new contributors keeping indentation consistent.

Good for me.


Reviewing the latest patch right now.

Comment 15

[Julien Wajsberg [:julienw]]

Comment on attachment 827886 [details] [diff] [review]
[PATCH v3] Handle message actions and increase test coverage

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

r=me with the jshint fixes and (but you ultimately decode) moving loadBodyHTML to the `setup` function.

The travis issue seems to happen because of a Gecko issue, I'll investigate. In the mean time you can merge.

::: apps/wappush/js/startup.js
@@ +5,5 @@
> +
> +'use strict';
> +
> +window.addEventListener('load', function callSetup(evt) {
> +  window.removeEventListener('load', callSetup);

not for this patch, more to keep this in mind: you might want to listen for "DOMContentLoaded" instead of "load".

Or rather, you might want to just load startup.js after wappush.js and just call "init" here ;) When the "defer"-ed scripts are loaded and executed, the DOM is ready.

Again, not for now, it's just to keep it in mind for a forthcoming patch.

::: apps/wappush/js/wappush.js
@@ +2,5 @@
>  /* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
>  
> +/* global CpScreenHelper, NotificationHelper, ParsedMessage, SiSlScreenHelper,
> +          Utils, WhiteList */
> +

you need to add a "/* exported WapPushManager */" for jshint correctness now ;)

::: apps/wappush/test/unit/wappush_test.js
@@ +1,4 @@
> +/* -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- /
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> +
> +/* global loadBodyHTML, mocha, MockL10n, MockMessageDB, MockNavigatormozApps,

you need to remove "mocha" from here, now

@@ +57,1 @@
>      loadBodyHTML('/index.html');

you might want to load a fresh page for each test (so in `setup` instead of `suiteSetup`)

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #15)
> not for this patch, more to keep this in mind: you might want to listen for
> "DOMContentLoaded" instead of "load".
> 
> Or rather, you might want to just load startup.js after wappush.js and just
> call "init" here ;) When the "defer"-ed scripts are loaded and executed, the
> DOM is ready.
> 
> Again, not for now, it's just to keep it in mind for a forthcoming patch.

Not being a JS expert I'm still somewhat puzzled as what would be the best way to tackle this. So just having a final startup.js file calling WapPushManager.init() would be the best approach? I really have no preference about this, I would just like to follow the established best practices whatever they are :)

> you need to add a "/* exported WapPushManager */" for jshint correctness now
> ;)
> 
> ::: apps/wappush/test/unit/wappush_test.js
> @@ +1,4 @@
> > +/* -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- /
> > +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> > +
> > +/* global loadBodyHTML, mocha, MockL10n, MockMessageDB, MockNavigatormozApps,
> 
> you need to remove "mocha" from here, now

Right, I'll fix both errors.

> @@ +57,1 @@
> >      loadBodyHTML('/index.html');
> 
> you might want to load a fresh page for each test (so in `setup` instead of
> `suiteSetup`)

Right, does that automatically drop the previous DOM tree? If so it will make the tests more robust.

BTW I've just filed bug 935509 for further refactoring in the app; something to tackle as soon as I'm rid of all the koi+ bugs.

Comment 17

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v4] Handle message actions and increase test coverage r=julienw

Updated patch with all review comments addressed. I'll be waiting for Travis to turn green before merging it:

https://travis-ci.org/mozilla-b2g/gaia/builds/13583558

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

Pushed to gaia/master 533d6263a950acf1ce62cd33dd7ecb30e6767316

I'll now adapt this for v1.2, since we've not decided if we want to uplift the CP patches this will require some additional work.

Comment 19

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v4 gaia/v1.2] Handle message actions and increase test coverage r=julienw

Uplifted patch that applies cleanly to v1.2

Comment 20

[Gabriele Svelto [:gsvelto] (PTO)]

Pushed to gaia/v1.2 2db8490d5c1d7eeab8f7383042616a0391571501

Comment 21

[Enpei]

verified 
Buri
Gaia:     590eb598aacf1e2136b2b6aca5c3124557a365ca
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/26f1e160e696
BuildID   20131107004003
Version   26.0