B2G SMS: Support Replace Short Message Type

RESOLVED FIXED in 1.3 Sprint 5 - 11/22

Status

--
minor
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

({feature})

unspecified
1.3 Sprint 5 - 11/22
ARM
Gonk (Firefox OS)
feature
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Blocks: 736710
(Assignee)

Comment 1

7 years ago
This feature requires an additional field `protocolId` in database. Any opinions or suggestions?
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... :(
Version: unspecified → Trunk
(Assignee)

Comment 3

7 years ago
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
(Assignee)

Comment 4

7 years ago
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(...);
  }
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?
(Assignee)

Comment 6

7 years ago
(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?
(Assignee)

Comment 7

7 years ago
BTW, above stored fields list in Android SMS database is available in packages/apps/Mms/src/com/android/mms/transaction/SmsReceiverService.java#extractContentValues().
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?
(Assignee)

Comment 9

7 years ago
(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.
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?
(Assignee)

Comment 11

7 years ago
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`.
(Assignee)

Comment 12

7 years ago
(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.
(Assignee)

Updated

7 years ago
Assignee: nobody → vyang
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.
(Assignee)

Comment 15

7 years ago
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
(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.
(Assignee)

Updated

7 years ago
Depends on: 736697
(Assignee)

Comment 17

7 years ago
Created attachment 610834 [details] [diff] [review]
Part 1: IDL change
Attachment #607888 - Attachment is obsolete: true
(Assignee)

Comment 18

7 years ago
Remove blocking flag for b2g-sms for milestone 3 temporarily.
No longer blocks: 709564
No longer blocks: 736710
(Assignee)

Updated

6 years ago
Keywords: feature
(Assignee)

Updated

6 years ago
Assignee: vyang → nobody
(Assignee)

Updated

6 years ago
Blocks: 853809
(Assignee)

Updated

5 years ago
No longer blocks: 853809
Duplicate of this bug: 853809
(Assignee)

Updated

5 years ago
Blocks: 891754
(Assignee)

Updated

5 years ago
Blocks: 869681

Comment 20

5 years ago
will this feature be supported on V1.1?
blocking-b2g: --- → hd?
(Assignee)

Comment 21

5 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → vyang
koi? for discussion in comms team backlog discussion
blocking-b2g: hd? → koi?
blocking-b2g: koi? → 1.3?

Comment 23

5 years ago
hi beatriz:
Can you accept the conclusion that this feature would not implemented on V1.1HD?
Thank you very much!
Flags: needinfo?(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.
Flags: needinfo?(brg)
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.
If Comms team can accept to have it in v1.4, let's try to push it to v1.4.
blocking-b2g: 1.3? → 1.4?
(Assignee)

Comment 27

5 years ago
Created attachment 828154 [details] [diff] [review]
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.
Attachment #610834 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Created attachment 828160 [details] [diff] [review]
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.
(Assignee)

Comment 29

5 years ago
Part 3, test cases, to come.
(Assignee)

Updated

5 years ago
Attachment #828154 - Flags: review?(gene.lian)
(Assignee)

Comment 30

5 years ago
Created attachment 828164 [details] [diff] [review]
part 2/3: implementation

Just a diff-friendly version.  No change.
Attachment #828160 - Attachment is obsolete: true
Attachment #828164 - Flags: review?(gene.lian)
(Assignee)

Updated

5 years ago
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Version: Trunk → unspecified
(Assignee)

Comment 31

5 years ago
Created attachment 828663 [details] [diff] [review]
part 1/3: don't return next message id : v2

re-gen patch from hg
Attachment #828154 - Attachment is obsolete: true
Attachment #828154 - Flags: review?(gene.lian)
Attachment #828663 - Flags: review?(gene.lian)
(Assignee)

Comment 32

5 years ago
Created attachment 828664 [details] [diff] [review]
part 2/3: implementation : v2

1. re-gen patch from hg
2. rebase to m-c tip
Attachment #828164 - Attachment is obsolete: true
Attachment #828164 - Flags: review?(gene.lian)
Attachment #828664 - Flags: review?(gene.lian)
(Assignee)

Comment 33

5 years ago
Created attachment 828666 [details] [diff] [review]
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
Attachment #828666 - Flags: review?(gene.lian)
Sorry for the delay. I'll take a look at least by today. Thanks!
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.
Attachment #828663 - Flags: review?(gene.lian) → review+
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.
Attachment #828664 - Flags: review?(gene.lian) → review+
Attachment #828666 - Flags: review?(gene.lian) → review+
(Assignee)

Comment 37

5 years ago
(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.
(Assignee)

Comment 38

5 years ago
Created attachment 8334486 [details] [diff] [review]
part 1/3: don't return next message id : v3

rebase only
Attachment #828663 - Attachment is obsolete: true
Attachment #8334486 - Flags: review+
(Assignee)

Comment 39

5 years ago
Created attachment 8334487 [details] [diff] [review]
part 2/3: implementation : v3

Address comment 36.
Attachment #828664 - Attachment is obsolete: true
Attachment #8334487 - Flags: review+
(Assignee)

Comment 40

5 years ago
Created attachment 8334488 [details] [diff] [review]
part 3/3: test cases : v2

rebase only.
Attachment #828666 - Attachment is obsolete: true
Attachment #8334488 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5b4b95bde952
https://hg.mozilla.org/mozilla-central/rev/ed0ffb6a9c8f
https://hg.mozilla.org/mozilla-central/rev/62a986812642
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 5 - 11/22
(Assignee)

Updated

5 years ago
blocking-b2g: 1.4? → ---
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. :(
(Assignee)

Comment 45

5 years ago
(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!
Depends on: 962421
No longer depends on: 962421
You need to log in before you can comment on or make changes to this bug.