787370 - B2G SMS: 'sent' and 'delivered' events not triggered

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Salvador de la Puente González [:salva]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.19 (KHTML, like Gecko) Ubuntu/12.04 Chromium/18.0.1025.168 Chrome/18.0.1025.168 Safari/535.19

Steps to reproduce:

In Gaia:
From APP1, attach callbacks to "onsent" and "ondelivered" members from mozSms.
From SMS application, send a message.
From APP1, send a message.


Actual results:

Any event received: nor from APP1, nor from SMS app.


Expected results:

An event should be sent to the callbacks "whenever an SMS is sent" as said in the documentation:
https://developer.mozilla.org/en-US/docs/DOM/SmsManager

Comment 1

[Philipp von Weitershausen [:philikon]]

Hsinyi, could you take a look at this bug and see if we can confirm it? Whether or not we can reproduce it, it would be good to have Marionette tests. :)

Comment 2

[Johnny Stenback (:jst)]

Assuming this gets confirmed, I think we should block on this.

Comment 3

[Salvador de la Puente González [:salva]]

I would like to add that a handler when a SMS is deleted (ondeleted) would be convenient as well.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Assigned to Vicamo since he told me that he's going to test this.

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> Assigned to Vicamo since he told me that he's going to test this.

Confirmed, but note "ondelivered" events are not always emitted due to per-carrier operational policy.

(In reply to Salvador de la Puente González [:salva] from comment #3)
> I would like to add that a handler when a SMS is deleted (ondeleted) would
> be convenient as well.

Please have a discuss on mozilla-webapi mailing list and fire a bug ;)

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> Confirmed, but note "ondelivered" events are not always emitted due to
> per-carrier operational policy.

To be more cleared, there is indeed a bug in onsent event but ondelivered works fine for me. I'll upload a patch for onsent bug and a patch for more descriptive debug messages for SMS-STATUS-REPORT PDU.

Comment 7

[Salvador de la Puente González [:salva]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> > Assigned to Vicamo since he told me that he's going to test this.
> 
> Confirmed, but note "ondelivered" events are not always emitted due to
> per-carrier operational policy.

Ok, thank you. I'll take in count.

>
> (In reply to Salvador de la Puente González [:salva] from comment #3)
> > I would like to add that a handler when a SMS is deleted (ondeleted) would
> > be convenient as well.
> 
> Please have a discuss on mozilla-webapi mailing list and fire a bug ;)

Ok, thank you. I'll start the discussion there.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: notify missed SmsManager.onsent event

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: add debug messages for SMS-STATUS-REPORT

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: test scripts

Comment 11

[Marshall Culpepper [:marshall_law]]

Comment on attachment 658032 [details] [diff] [review]
Part 1: notify missed SmsManager.onsent event

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +962,5 @@
>      }
>  
>      gSmsRequestManager.notifySmsSent(options.requestId, sms);
> +
> +    Services.obs.notifyObservers(options.sms, kSmsSentObserverTopic, null);

Maybe a follow up for another bug, but it looks like a notifyObservers call for "sms-send-failed" is also missing around line 994:

http://dxr.mozilla.org/mozilla-central/dom/system/gonk/RadioInterfaceLayer.js.html#l994

Comment 12

[Marshall Culpepper [:marshall_law]]

Comment on attachment 658034 [details] [diff] [review]
Part 3: test scripts

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

Looks good, check my follow up below

::: dom/sms/tests/marionette/manifest.ini
@@ +6,5 @@
>  [test_between_emulators.py]
>  [test_incoming.js]
> +
> +[test_outgoing.js]
> +b2g = false

Does marionette run non-b2g tests in the emulator? if not, this seems like a useless flag :)

If you intend for this test to run in non-b2g environments, would this test be better served as an xpcshell test or a mochitest instead?

::: dom/sms/tests/marionette/test_outgoing.js
@@ +10,5 @@
> +let receiver = "5555552368";
> +let body = "Hello SMS world!";
> +let now = Date.now();
> +
> +sms.onsent = function onsent(event) {

It's sad we didn't have any tests for onsent yet :(

I haven't seen any tests for ondelivered either, can you make sure to file a follow up so we can get more coverage?

@@ +30,5 @@
> +
> +  SpecialPowers.removePermission("sms", document);
> +  finish();
> +};
> +

If you end up fixing the notifyObserver call for sms-send-failed, is there a way for send-failed to bubble up to DOM? We might want to make sure an API for that exists in a follow up (probably outside the scope of this test though)

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Marshall Culpepper [:marshall_law] from comment #11)
> Maybe a follow up for another bug, but it looks like a notifyObservers call
> for "sms-send-failed" is also missing around line 994:
> 
> http://dxr.mozilla.org/mozilla-central/dom/system/gonk/RadioInterfaceLayer.
> js.html#l994

No, we don't have a onsendfailed event in nsIDOMSmsManager, neither ondeliverfailed. For `ondeliverfailed`, Mounir mentioned it once in bug 777251, but the latest update we may have during the San Paulo work week is having a discussion on what's missing and required for V2.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Marshall Culpepper [:marshall_law] from comment #12)
> ::: dom/sms/tests/marionette/manifest.ini
> @@ +6,5 @@
> >  [test_between_emulators.py]
> >  [test_incoming.js]
> > +
> > +[test_outgoing.js]
> > +b2g = false
> 
> Does marionette run non-b2g tests in the emulator? if not, this seems like a
> useless flag :)
> If you intend for this test to run in non-b2g environments, would this test
> be better served as an xpcshell test or a mochitest instead?

I was considering about billing problem. I don't want to send SMS message out in physical device. After some research on ManifestParser, I think it's already safe enough with default tags (qemu=true) above. I'll remove the redundant flag.

> I haven't seen any tests for ondelivered either, can you make sure to file a
> follow up so we can get more coverage?

There is no `ondelivered` event support in emulator. ondelivered event depends on a PDU sent by SMSC, which doesn't exist in emulator. We can, and should, bring it in.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> There is no `ondelivered` event support in emulator. ondelivered event
> depends on a PDU sent by SMSC, which doesn't exist in emulator. We can, and
> should, bring it in.

Open as bug 788928.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: notify missed SmsManager.onsent event : V2

rebase only

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: test scripts : v2

1. Address review comment #12 about useless flag in manifest.ini
2. Cover both SmsManager.onsent and SmsRequest.onsuccess, single and multiple receipients, and do more semantic tests as possible. Therefore another review is required IMO.

TODO: SmsManager.ondelivered(bug 788928) and SmsRequest.onerror.

Comment 18

[Marshall Culpepper [:marshall_law]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> There is no `ondelivered` event support in emulator. ondelivered event
> depends on a PDU sent by SMSC, which doesn't exist in emulator. We can, and
> should, bring it in.

The emulator supports forwarding along arbitrary PDUs. I take advantage of this in my voicemail tests:

http://dxr.mozilla.org/mozilla-central/dom/telephony/test/marionette/pdu_builder.js.html
http://dxr.mozilla.org/mozilla-central/dom/telephony/test/marionette/test_voicemail_statuschanged.js.html#l10

Comment 19

[Marshall Culpepper [:marshall_law]]

Comment on attachment 659105 [details] [diff] [review]
Part 3: test scripts : v2

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

Wow, this is great! Good work :) r+ with just a few comments below..

::: dom/sms/tests/marionette/test_outgoing.js
@@ +33,5 @@
> +function checkSameSentMessage(message, now) {
> +  checkSentMessage(message, now);
> +
> +  if (!sentMessages[message.id]) {
> +    sentMessages[message.id] = message;

nit: it would be probably be better to do:

let sentMessage = sentMessages[message.id];
if (!sentMessage) {
  sentMessages[message.id] = message;
} else {
  ...
  ok(sentMessage.timestamp..)
}

@@ +71,5 @@
> +  function onRequestSuccess(event) {
> +    log("SmsRequest.onsuccess event received.");
> +    event.target.removeEventListener("success", onRequestSuccess);
> +
> +    ok(event.target instanceof MozSmsRequest,

should this check happen before removing the event listener above?

Comment 20

[Marshall Culpepper [:marshall_law]]

(In reply to Marshall Culpepper [:marshall_law] from comment #19)

> nit: it would be probably be better to do:
> 
> let sentMessage = sentMessages[message.id];
> if (!sentMessage) {
>   sentMessages[message.id] = message;
> } else {
>   ...
>   ok(sentMessage.timestamp..)
> }

Or, even better still, remove the "else" and use early return semantics:

let sentMessage = sentMessages[message.id];
if (!sentMessage) {
  sentMessages[message.id] = message;
  return;
}

// Everything in "else" here..

Comment 21

[Philipp von Weitershausen [:philikon]]

bail out early <3

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Marshall Culpepper [:marshall_law] from comment #18)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> > There is no `ondelivered` event support in emulator. ondelivered event
> > depends on a PDU sent by SMSC, which doesn't exist in emulator. We can, and
> > should, bring it in.
> 
> The emulator supports forwarding along arbitrary PDUs. I take advantage of
> this in my voicemail tests:
> 
> http://dxr.mozilla.org/mozilla-central/dom/telephony/test/marionette/
> pdu_builder.js.html
> http://dxr.mozilla.org/mozilla-central/dom/telephony/test/marionette/
> test_voicemail_statuschanged.js.html#l10

Well, SMS-STATUS-REPORT contains a `messageRef` field that must be identical with the one previously sent to ril_worker in the response of REQUEST_SEND_SMS. That's something hidden from DOM API or even RadioInterfaceLayer. I would like to leave ondelivered event untested in this issue and move following discussion to bug 788928.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: test scripts : v3

address review comment #19, #20

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db7ae51b909d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb1c4456082
https://hg.mozilla.org/integration/mozilla-inbound/rev/21244f4a873b

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/db7ae51b909d
https://hg.mozilla.org/mozilla-central/rev/3eb1c4456082
https://hg.mozilla.org/mozilla-central/rev/21244f4a873b