1067731 - [dolphin][flame] Process wap push message differently according to its type and priority level

Status: RESOLVED FIXED

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

Description

[Peipei Cheng (needinfo if you need my action)]

STR
----------------------------------------------------------------------------
1. send different types and different levels of wap push to device (SI/SL,none/low/medium/high/delete)
   --> For all types/levels, FFOS device behaves the same: get wap psh notification and allow user to open from notification bar and then remove wap push from device.

[expected results] It would be better if we can process wap push message differently according to its type and level.


Following is how android phone works. Just for reference.
---------------------------------------------------------------------------------
Type SI:
      None  ：User could not see any notification on device. The message can only be seen from the log.
      Low   ：No notification is shown. User can see new message from SMS inbox.
      Medium：There is push notification. User can open the links in the message.
      High： Message is displayed once arrive device.
      delete：Message is deleted as well as other wap push with the same ID.

Type SL：
       High：Direct open the webpage once message arrives.
       Low：There is push notification, and use can open the links.
       cache：No notification, but browser will cache the url.

Comment 1

[Gabriele Svelto [:gsvelto]]

(In reply to Peipei Cheng from comment #0)
>    --> For all types/levels, FFOS device behaves the same: get wap psh
> notification and allow user to open from notification bar and then remove
> wap push from device.

We would need some radically new UX for this, like moving the WAP Push functionality to the SMS app. I'll add a few comments below to explain current behavior.

> [expected results] It would be better if we can process wap push message
> differently according to its type and level.
> 
> 
> Following is how android phone works. Just for reference.
> -----------------------------------------------------------------------------
> ----
> Type SI:
>       None  ：User could not see any notification on device. The message can
> only be seen from the log.
>       Low   ：No notification is shown. User can see new message from SMS
> inbox.

We display both None and Low messages as notifications because there's no inbox for WAP Push messages currently.

>       Medium：There is push notification. User can open the links in the
> message.

This is the current behavior.

>       High： Message is displayed once arrive device.

This could be implemented already.

>       delete：Message is deleted as well as other wap push with the same ID.

Bug 942064, I hope to fix it soon.

> Type SL：
>        High：Direct open the webpage once message arrives.

I would consider opening a page without the user consent a security risk.

>        Low：There is push notification, and use can open the links.

This is the current behavior.

>        cache：No notification, but browser will cache the url.

We don't really have a way to do this right now and it could also pose a security risk. In general I wouldn't do any actions that involve opening a link without the user knowing or being able to intervene.

Comment 2

[jinghua.xing]

hello Gabriele

I want to know how does this issue going on as this issue still can be reproduced on v2.1. And you say it is implemented alreadyfor display the SI message whose action is high once it arrive device, but I can't find where to implement it in v2.1 and I think it is not implemented in v2.1.

Thank you.

Comment 3

[Gabriele Svelto [:gsvelto]]

(In reply to jinghua.xing from comment #2)
> I want to know how does this issue going on as this issue still can be
> reproduced on v2.1. And you say it is implemented alreadyfor display the SI
> message whose action is high once it arrive device, but I can't find where
> to implement it in v2.1 and I think it is not implemented in v2.1.

Displaying a message with high priority directly is not implemented yet, but it could be implemented rather quickly. I'll open a bug specifically for that.

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Display immediately WAP Push signal indication messages with the signal-high action set

This patch launches the application and displays immediately messages with the 'signal-high' action set. This is consistent with the WAP Push specification which recommends to make such messages more 'visible' than ones with lower priority. It may need review from UX though.

Note that I've moved the code that launches the app and clears the automatic close timeout into the wpm_displayWapPushMessage() function because it made sense to factor it there rather than duplicating it for all the callers. After all wpm_displayWapPushMessage() is always the first function called when displaying the application so this turns it into its main entrypoint.

Note that this patch also applies cleanly to v2.1 so if partners want to pick it up it won't require modifications to work there.

Comment 5

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Display immediately WAP Push signal indication messages with the signal-high action set

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8536577 [details] [diff] [review]
[PATCH] Display immediately WAP Push signal indication messages with the signal-high action set

r=me

but I'm not comfortable with calling wpm_finish() after launching the app. If this works I think it can be racy. So please try to do otherwise before landing.

You can ask review again if you want another look after your change.

Comment 7

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Display immediately WAP Push signal indication messages with the signal-high action set

Thanks for the review, the wpm_finish() invocation is definitely racy so I've moved it around. I've also extended the unit-tests to ensure that what happens after the message is received is what we expect to happen. The PR was also updated.

Comment 8

[jinghua.xing]

For the SI message with "signal-none" action, as the WAP-167 says
>If the received SI’s action attribute equals “signal-none”, the SI MUST NOT:
>• be presented (see section 6.3) to the end-user
>• be postponed (see section 6.3.1)
>• imply that a service is being executed without user intervention
>However, the client MAY use the information carried in the info element to perform certain tasks. For 
>information about how the info element is used, see section 5.2.2.

I think we can discard messages with "signal-none" action when we receive it on gaia side. What's your opinion about this?
Thank you

Comment 9

[jinghua.xing]

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

Gsvelto, I think the SL message with "execute-high“ action should also behavior as the SI message with "signal-high" does. How do your think about this?

Thank you.

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to jinghua.xing from comment #8)
> I think we can discard messages with "signal-none" action when we receive it
> on gaia side. What's your opinion about this?

We've decided to send notifications for signal-none messages until we have an inbox, see bug 981521 for more details.

(In reply to jinghua.xing from comment #9)
> Gsvelto, I think the SL message with "execute-high“ action should also
> behavior as the SI message with "signal-high" does. How do your think about
> this?

We did not implement the execute options for SL links because those would force us to open the link in the message. Opening the link without user intervention is not a good idea security-wise so instead we just send a notification for the message.

Comment 11

[jinghua.xing]

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

> We've decided to send notifications for signal-none messages until we have
> an inbox, see bug 981521 for more details.

In the protocol，it says the message with “signal-none” action MUST NOT be be presented or postponed, so I think we should just discard it as android does.


> We did not implement the execute options for SL links because those would
> force us to open the link in the message. Opening the link without user
> intervention is not a good idea security-wise so instead we just send a
> notification for the message.

For the message with 'execute-high' message, I didn't mean open the link without user intervention. But I think when receive this message, we can directly launch the wappush app and display this message.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8536754 [details] [diff] [review]
[PATCH v2] Display immediately WAP Push signal indication messages with the signal-high action set

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

r=me, please fix this nits and do a full try run before merging :)

::: apps/wappush/test/unit/wappush_test.js
@@ +709,5 @@
> +      this.sinon.spy(window, 'close');
> +    });
> +
> +    teardown(function() {
> +      clock.restore();

nit: you don't need to restore, because you use this.sinon :)

@@ +725,5 @@
> +          sinon.assert.calledOnce(window.close);
> +          assert.isFalse(MockNavigatormozApps.mAppWasLaunched);
> +        });
> +      }, done);
> +    });

nit: I prefer to use this form when I test using promises:

  WapPushManager.....then(function() {
    do assertions
  }).then(done, done);

It helps keeping indentations low, and it's more readable. In the end, assertions are throwing when failing, thus rejecting the promise, and the second "done" is called with the exception as parameter. Simple yet effective :)

@@ +733,5 @@
> +        done(function checks() {
> +          clock.tick(100);
> +          sinon.assert.notCalled(Notification);
> +          sinon.assert.notCalled(window.close);
> +          assert.isTrue(MockNavigatormozApps.mAppWasLaunched);

wondering if you can't assert something more, like asserting it's displayed?

Comment 13

[Julien Wajsberg [:julienw]]

If you want to address Jinghua's concern, please request review again.

Comment 14

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Display immediately WAP Push signal indication messages with the signal-high action set

Thanks for the review, I've addressed all the review comments and taken Jinghua's suggestion to do the same for SL messages with action="execute-high" set. If you look at the PR I've posted my changes as a separate patch on top of the previous one for ease of review.

Comment 15

[Gabriele Svelto [:gsvelto]]

Just one last note, I've also properly chained the promises when displaying the message directly. I wasn't doing it before and I noticed it when I've updated the unit-tests to check if the screen was really getting populated.

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8539309 [details] [diff] [review]
[PATCH v3] Display immediately WAP Push signal indication messages with the signal-high action set

r=me, but wondering if you wanted to handle signal-none in a special way as well? Could be a separate bug :)

Comment 17

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> r=me, but wondering if you wanted to handle signal-none in a special way as
> well? Could be a separate bug :)

We already did in the past but I ultimately had to disable that code due to bug 981521. We'll probably handle it again when we have some form of inbox in the future.

Comment 18

[Gabriele Svelto [:gsvelto]]

Try is green: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=482cfd5faa20

Pushed to gaia/master db92ae26d66e8a8e1faed4f65009d2da00ad066a

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

Comment 19

[jinghua.xing]

Can you uplift this to v2.1? Thank you.

Comment 20

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8539309 [details] [diff] [review]
[PATCH v3] Display immediately WAP Push signal indication messages with the signal-high action set

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Missing functionality requested by partner, see comment 0 and comment 19
[User impact] if declined: WAP Push messages with the execute-high/signal-high attribute are not immediately shown
[Testing completed]: Tested using the desktop build with mocked calls and covered with unit tests
[Risk to taking this patch] (and alternatives if risky): WAP Push functionality might be affected though the changes are relatively small
[String changes made]: None

Comment 21

[jinghua.xing]

When we received a push message,we first use pendingMessages++ to add 1 to pendingMessages. But after saving the message to database, we did not use pendingMessages-- to reduce it. When we try to close this message, because of the code:

 >   closeTimeout = window.setTimeout(function wpm_delayedClose() {
 >     if (pendingMessages > 0) {
 >       // Pending messages were received since we set this timer
 >       closeTimeout = window.setTimeout(wpm_delayedClose, 100);
 >        return;
 >     }

it will going to an endless loop and we cannot close the message. Please help to confirm  it. Thank you.

Comment 22

[Gabriele Svelto [:gsvelto]]

(In reply to jinghua.xing from comment #21)
> it will going to an endless loop and we cannot close the message. Please
> help to confirm  it. Thank you.

Confirmed, it's a bug. I'll back out the patch and revise it to avoid this problem.

Comment 23

[Gabriele Svelto [:gsvelto]]

The patch was backed out, I'll post a new one ASAP.

Comment 24

[Julien Wajsberg [:julienw]]

Ooops right, sorry about that.
Gabriele, can you post the revert hash?

Comment 25

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] (PTO 12/25 -> 12/29) from comment #24)
> Ooops right, sorry about that.
> Gabriele, can you post the revert hash?

Yes, sorry about that, the patch was reverted in a50281231318efb2da39d9e0844e3a2b2662ee25

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

Comment 26

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v4] Display immediately WAP Push signal indication messages with the signal-high action set

This is a one-line change that always calls wpm_finish() when we're done saving a message to the database. This works for both cases because wpm_sendNotification() is synchronous so the app won't be closed before it finishes executing anyway and wpm_displayWapPushMessage() explicitly cancels the closing procedure started by wpm_finish().

For ease of review I've reopened the PR and pushed the change on top of the previous ones.

Comment 27

[Julien Wajsberg [:julienw]]

Comment on attachment 8541146 [details] [diff] [review]
[PATCH v4] Display immediately WAP Push signal indication messages with the signal-high action set

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

I'm not very fond of this solution, but please land it if you think it's better like this, because it should work after all.

Otherwise you can follow my advice :)

::: apps/wappush/js/wappush.js
@@ +244,1 @@
>        wpm_finish();

I'd rather not call this and only decrement pendingMessages in the first block of the if condition.

I understand that clearing the timeout in displayMessage should work, but it feels quite hacky to undo something we just did. Better not do it in the first place.

Comment 28

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] (PTO 12/25 -> 12/29) from comment #27)
> I understand that clearing the timeout in displayMessage should work, but it
> feels quite hacky to undo something we just did. Better not do it in the
> first place.

You've got a point, let's make it explicit, implicit dependencies stuff usually become broken when somebody touches the code without knowing about them. I also hope to get rid of the whole close-on-a-timer hack soon.

Comment 29

[Gabriele Svelto [:gsvelto]]

Try is green: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=093fdcb4ef0c

Pushed to gaia/master 1509b2cedfd7d19c20a48a1901296e529470645d

https://github.com/mozilla-b2g/gaia/commit/1509b2cedfd7d19c20a48a1901296e529470645d

Comment 30

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8541146 [details] [diff] [review]
[PATCH v4] Display immediately WAP Push signal indication messages with the signal-high action set

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Missing functionality requested by partner, see comment 0 and comment 19
[User impact] if declined: WAP Push messages with the execute-high/signal-high attribute are not immediately shown
[Testing completed]: Tested using the desktop build with mocked calls and covered with unit tests
[Risk to taking this patch] (and alternatives if risky): Viewing WAP Push messages might be affected, a previous version of this patch was backed out already, see comment 21
[String changes made]: None

Comment 31

[bhavana bajaj [:bajaj]]

Given the risk here and that this is basically implementing a feature, I am concerned about landing this in 2.1 at this point. Ni steven yang to confirm the partner requirement here and help push this to 2.2 instead or provide other ideas to mitigate risk here?

Comment 32

[Steven Yang [:styang]]

it was issued by our partner before, let's check if we can put it into 2.1S. ni? seinlin and vincent to help. thanks.

Comment 33

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

The current patch can be uplift into v2.1/v2.1s, only comment in wappush_test.js got conflict.

Comment 34

[bhavana bajaj [:bajaj]]

Comment on attachment 8541146 [details] [diff] [review]
[PATCH v4] Display immediately WAP Push signal indication messages with the signal-high action set

Looks like steven yang is going to land it on desired partner branch

Comment 35

[Vincent Liu[:vliu]]

v2.1s: https://github.com/mozilla-b2g/gaia/commit/b28d114c933840840612a2640b9bfa9403525602