736708 - B2G SMS: Support Replace Short Message Type

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

According to 3GPP 23.040 section 9.2.3.9, "The Replace Short Message feature is optional ... If such a code is present, then the MS shall check the originating address and replace any existing stored message having the same Protocol Identifier code and originating address with the new short message and other parameter values."

This feature is effective when TP-PID is set to 0x41..0x47 in a SMS-DELIVER pdu.

Comment 1

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

This feature requires an additional field `protocolId` in database. Any opinions or suggestions?

Comment 2

[Mounir Lamouri (:mounir)]

Vicamo, could you explain this problem in understandable terms so I could have an opinion? I really have no idea of what you are speaking about... :(

Comment 3

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

My fault. Let me try again.

On receipt of a short message with its TP-PID field set to one of the Replace Short Message Types, the MS(Mobile Station, or mobile device, phone, whatever) shall replace any existing stored message having the same TP-PID and originating address with the new short message. It says we shall search whether we have stored messages that have the same sender address and TP-PID, and replace these messages with the new one, if the TP-PID falls in 0x41..0x47. If a Replace Short Message Type code is not present, then the MS shall store the message in the normal way.

There is no much information about when does this feature is actually applied. Android has it by the way.

References:
1) http://tiliman.wordpress.com/2008/12/20/replacing-sms-on-a-phone-using-kannel-sms-gateway/ , document of a sms gateway
2) http://smstools3.kekekasvi.com/index.php?p=fileformat , document of a sms tool

Comment 4

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

Something like:

  saveReceivedMessage: function saveReceivedMessage(sender, pid, body, date) {
    if (pid in 0x41..0x47) {
      let messages = getMessages(pid, sender);
      if (messages) {
        replaceMessagesInDatabase(...);
        return;
      }
      // fallback to normal procedures.
    }
    storeMessageInDatabase(...);
  }

Comment 5

[Mounir Lamouri (:mounir)]

Can't we do that check _before_ calling saveReceivedMessage? That way, we don't have to change our internal API in a way that will only match B2G backend needs?

Comment 6

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

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #5)
> Can't we do that check _before_ calling saveReceivedMessage?

Because the previous message might have already been stored in database a few days, months, years ago. We can't assure the newer message arrives before the victim message is stored. Besides, messages are not processed concurrently. Their full journey begins in parcel read and ends in getting stored in database or just discarded. You can't at all predict whether current processing message is to be replaced by the next one, or the next tenth one, or not.

> That way, we don't have to change our internal API in a way that will only
> match B2G backend needs?

I'm not really know how android backend or any other backends save these SMS messages. But I'm quite curious that Android stores a) originating address, b) date, c) pid, d) read, e) seen, f) subject, g) reply-path-present, h) smsc, i) body, how could the android backend save messages without them?

Comment 7

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

BTW, above stored fields list in Android SMS database is available in packages/apps/Mms/src/com/android/mms/transaction/SmsReceiverService.java#extractContentValues().

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not sure I really understand what this bug is about.

Is this simply something that lets the network say "hey, that message that I sent a week ago, it had the wrong contents, it should really contain the following message: '...'"?

If so, is this something that we really need to expose in the API? Couldn't we simply replace the message that we have in the database with the new message contents?

Comment 9

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

(In reply to Jonas Sicking (:sicking) from comment #8)
> I'm not sure I really understand what this bug is about.
> 
> Is this simply something that lets the network say "hey, that message that I
> sent a week ago, it had the wrong contents, it should really contain the
> following message: '...'"?
> 
> If so, is this something that we really need to expose in the API? Couldn't
> we simply replace the message that we have in the database with the new
> message contents?

Yep, so there must be a way to discover there is a message of the same pid and sender address pair to be replaced. RadioInterfaceLayer.saveReceivedMessage() accepts `number(sender address)`, `body`, `timestamp` for now. An additional `pid` is therefore required for both database & API to implement this feature.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, so we can do that using internal APIs? I.e. we don't need to expose anything more than we already do to web pages?

Comment 11

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

Something like:

   interface nsISmsDatabaseService : nsISupports
   {
   ...
  -  long saveReceivedMessage(in DOMString aSender, in DOMString aBody, in unsigned long long aDate);
  +  long saveReceivedMessage(in DOMString aSender, in unsigned long aProtocolId, in DOMString aBody, in unsigned long long aDate);
   ...
   };

But please note another issue, bug 736701, requires another two new fields `ReplyPathPresent` and `SMSCAddress`.

Comment 12

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

(In reply to Vicamo Yang [:vicamo] from comment #6)
> I'm not really know how android backend or any other backends save these SMS
> messages. But I'm quite curious that Android stores a) originating address,
> b) date, c) pid, d) read, e) seen, f) subject, g) reply-path-present, h)
> smsc, i) body, how could the android backend save messages without them?

The answer is, they're not saved in b2g android backend. SMS messages in this case are saved in MMS application as any normal Android platform does.

Comment 13

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

Attachment: WIP

Comment 14

[Mounir Lamouri (:mounir)]

Actually, I realize we just don't call |saveReceivedMessage| in the Android backend. We let the stock SMS app do that. So, I guess we could just change that internal API to match the needs.

Comment 15

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

Comment on attachment 607888 [details] [diff] [review]
WIP

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

::: dom/sms/interfaces/nsISmsDatabaseService.idl
@@ +49,4 @@
>  interface nsISmsDatabaseService : nsISupports
>  {
>    // Takes some information required to save the message and returns its id.
> +  long saveReceivedMessage(in DOMString aSender, in unsigned long aProtocolId, in DOMString aBody, in unsigned long long aDate);

aProtocolId should be an octet

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to Vicamo Yang [:vicamo] from comment #15)
> Comment on attachment 607888 [details] [diff] [review]
> WIP
> 
> Review of attachment 607888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/sms/interfaces/nsISmsDatabaseService.idl
> @@ +49,4 @@
> >  interface nsISmsDatabaseService : nsISupports
> >  {
> >    // Takes some information required to save the message and returns its id.
> > +  long saveReceivedMessage(in DOMString aSender, in unsigned long aProtocolId, in DOMString aBody, in unsigned long long aDate);
> 
> aProtocolId should be an octet

We don't have |octet| in our IDL format.

Comment 17

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

Attachment: Part 1: IDL change

Comment 18

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

Remove blocking flag for b2g-sms for milestone 3 temporarily.

Comment 20

[lecky]

will this feature be supported on V1.1?

Comment 21

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

(In reply to lecky from comment #20)
> will this feature be supported on V1.1?

It won't.  And it's not listed in v1.2, either.

Comment 22

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

koi? for discussion in comms team backlog discussion

Comment 23

[lecky]

hi beatriz:
Can you accept the conclusion that this feature would not implemented on V1.1HD?
Thank you very much!

Comment 24

[Beatriz Rodríguez [:brg]]

(In reply to lecky from comment #23)
> hi beatriz:
> Can you accept the conclusion that this feature would not implemented on
> V1.1HD?
> Thank you very much!
Yes. We do not ask for new features that are out of scope of v1.1.

Comment 25

[Wilfred Mathanaraj [:WDM]]

this is not in v1.2 nor currently considered for v1.3 - I have added this to the comms backlog document @ https://docs.google.com/a/mozilla.com/spreadsheet/ccc?key=0AtVT90hlMtdSdEd4TVVjWXNfU3ctMlVhWFRrWkpweVE#gid=23 for future consideration.

Comment 26

[Kai-Chih Hu [:khu]]

If Comms team can accept to have it in v1.4, let's try to push it to v1.4.

Comment 27

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

Attachment: part 1/3: don't return next message id.

Replace Short Message Type is a feature that allows SMSC to send a message that overwrites previous one.  That means when we're going to save a new incoming SMS message, we have to search the database first, pick up that matching message and replace it.  So we can no more tell a message is overriding or not by the presence of its |aMessageRecord.id| attribute, neither can we determine whether that message deserves a new message id.  As a result, returning the id of that to-be-stored message is impossible.

Comment 28

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

Attachment: part 2/3: implementation

Basically divide original |saveRecord()| into three parts.  The first one prepares transaction callbacks, the second one locates the message to be replaced when necessary, and the third one, almost everything in original |saveRecord()|, performs real replacing work.

Comment 29

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

Part 3, test cases, to come.

Comment 30

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

Attachment: part 2/3: implementation

Just a diff-friendly version.  No change.

Comment 31

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

Attachment: part 1/3: don't return next message id : v2

re-gen patch from hg

Comment 32

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

Attachment: part 2/3: implementation : v2

1. re-gen patch from hg
2. rebase to m-c tip

Comment 33

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

Attachment: part 3/3: test cases

For the 7 Replace Short Message Type PIDs, verify that message replacing is actually happening when message PID falls into the seven ones and the originating address matches.

full try: https://tbpl.mozilla.org/?tree=Try&rev=ae5ac40c9af6

Comment 34

[Gene Lian (I already quit Mozilla)]

Sorry for the delay. I'll take a look at least by today. Thanks!

Comment 35

[Gene Lian (I already quit Mozilla)]

Comment on attachment 828663 [details] [diff] [review]
part 1/3: don't return next message id : v2

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ -1447,5 @@
>      });
>    },
>  
>    saveRecord: function saveRecord(aMessageRecord, aAddresses, aCallback) {
> -    let isOverriding = (aMessageRecord.id !== undefined);

I'd prefer keeping this line in the beginning of this function, which emphasizes the saveRecord() doesn't only save new record but also update the old one.

@@ -1457,1 @@
>      if (DEBUG) debug("Going to store " + JSON.stringify(aMessageRecord));

That would be nice to dump the |isOverriding| in the log.

@@ +1467,2 @@
>        txn.oncomplete = function oncomplete(event) {
> +        if (aMessageRecord.id > self.lastMessageId) {

I'd prefer using:

  if (!isOverriding)

Comparing the number looks implicit about why we need to do so.

@@ +1488,4 @@
>            return;
>          }
>  
> +        let isOverriding = (aMessageRecord.id !== undefined);

Drop this line.

Comment 36

[Gene Lian (I already quit Mozilla)]

Comment on attachment 828664 [details] [diff] [review]
part 2/3: implementation : v2

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

Looks good to me.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1511,5 @@
> +                               aMessageRecord, aAddresses);
> +    }, [MESSAGE_STORE_NAME, PARTICIPANT_STORE_NAME, THREAD_STORE_NAME]);
> +  },
> +
> +  replaceShortMessage: function replaceShortMessage(aTransaction, aMessageStore,

s/replaceShortMessage/replaceShortMessageOnSave

Does it sound better?

@@ +1576,5 @@
> +  realSaveRecord: function realSaveRecord(aTransaction, aMessageStore,
> +                                          aParticipantStore, aThreadStore,
> +                                          aMessageRecord, aAddresses) {
> +    let self = this;
> +      this.findThreadRecordByParticipants(aThreadStore, aParticipantStore,

nit: align the codes before landing.

@@ +1998,5 @@
>      if (aMessage.type == "sms") {
>        aMessage.delivery = DELIVERY_RECEIVED;
>        aMessage.deliveryStatus = DELIVERY_STATUS_SUCCESS;
>  
> +      if (aMessage.pid == null) {

Let's use "== undefined" to have a more consistent style within this function.

Comment 37

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #35)
> part 1/3: don't return next message id : v2
> -----------------------------------------------------------------
> ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
> @@ -1447,5 @@
> >      });
> >    },
> >  
> >    saveRecord: function saveRecord(aMessageRecord, aAddresses, aCallback) {
> > -    let isOverriding = (aMessageRecord.id !== undefined);
> 
> I'd prefer keeping this line in the beginning of this function, which
> emphasizes the saveRecord() doesn't only save new record but also update the
> old one.

We're going to replace an existing message with a just received one, remember?  So we cannot sure whether this message is going to override a certain message or not before checking it's pid and finding out a replacable message.  The action "find" follows that |isOverriding| variable must be inside the transaction body.

Comment 38

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

Attachment: part 1/3: don't return next message id : v3

rebase only

Comment 39

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

Attachment: part 2/3: implementation : v3

Address comment 36.

Comment 40

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

Attachment: part 3/3: test cases : v2

rebase only.

Comment 41

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

full try before landing: https://tbpl.mozilla.org/?tree=Try&rev=9cb51f9729f8

Comment 42

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

https://hg.mozilla.org/integration/b2g-inbound/rev/5b4b95bde952
https://hg.mozilla.org/integration/b2g-inbound/rev/ed0ffb6a9c8f
https://hg.mozilla.org/integration/b2g-inbound/rev/62a986812642

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b4b95bde952
https://hg.mozilla.org/mozilla-central/rev/ed0ffb6a9c8f
https://hg.mozilla.org/mozilla-central/rev/62a986812642

Comment 44

[Jonathan Griffin (:jgriffin)]

The test test_replace_short_message_type.js is a very frequent intermittent on TBPL since landing; see bug 940881.  If this isn't fixed soon, I'm going to disable the test. :(

Comment 45

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

(In reply to Jonathan Griffin (:jgriffin) from comment #44)
> The test test_replace_short_message_type.js is a very frequent intermittent
> on TBPL since landing; see bug 940881.  If this isn't fixed soon, I'm going
> to disable the test. :(

Fix committed.  Thank you!