865157 - B2G MMS: Receiving an MMS creates a redundant thread different from the one containing SMS (from the same sender)

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Borja Salguero [:borjasalguero]]

STR:
- Receive a MMS from '[phone]'

EXPECTED:
- 1 notification and One thread it's created with the MMS received from '[phone]'

CURRENTLY:
- 2 notifications and 2 threads have been created with the MMS received from '[phone]'

Comment 1

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

Does this duplicates bug 853752?

Comment 2

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

MMS always comes with a notification first, and we should store this notification into database so that you can retrieve it later in manual retrieval mode, so there must be two notifications.

Comment 3

[Borja Salguero [:borjasalguero]]

In this case I've received one MMS twice (not a notification + MMS) :S

Comment 4

[Gene Lian (I already quit Mozilla)]

Just having a quick discussion with Steve. Bug 853752 only solves partial problem. This issue is happening when attempting to receive an SMS and an MMS continuously. Let me look into this.

Comment 5

[Steve Chung [:steveck]]

2 notifications is correct in current flow, I will discuss with UX about disabling first notification for automatic download mode. The issue in this bug should be focused on one more thread will be created after receiving a mms message, these 2 threads got different ID and contained same messages.

Comment 6

[Gene Lian (I already quit Mozilla)]

Re-describe the title based on the comment #5.

This bug blocks Bug 840055 which is a leo+ MMS user story (MMS Thread View). Nominating for leo+.

Comment 7

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

blocking a leo+ user story, leo+

Comment 8

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Mounir,

This patch touches DOM change. Need your super review. The MMS notification indication doesn't carry any receivers. Originally, we should add our phone number to receivers. However, some SIM cards don't allow us to retrieve the phone number info. Under this circumstance, we still hope to add a NULL receiver to let the Gaia know the receiver is not available (the current SMS codes do the same thing). To do so, we need to loose the restriction of checking string type. Thanks!

Comment 9

[Anshul]

Doesn't seem like this bug touches any interface, at least not the current patch. Okay to uplift.

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 742335 [details] [diff] [review]
Patch

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

r- because I do not understand why you seem to be only reading the last value of the array. I am likely missing something though.

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +175,3 @@
>        return NS_ERROR_INVALID_ARG;
>      }
> +    if (receiverJsVal.isString()) {

I'm not sure I understand what you are doing here. Isn't receiverJsVal the last value of the array?

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1008,5 @@
>        // phone number, thus setting the SMS' receiver to be null.
>        aMessage.receiver = self;
>      } else if (aMessage.type == "mms") {
>        let receivers = aMessage.receivers;
>        if (!receivers.length) {

nit: I prefer explicit == 0.

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 742335 [details] [diff] [review]
Patch

Keeping the request open is probably better actually.

Comment 12

[Gene Lian (I already quit Mozilla)]

Comment on attachment 742335 [details] [diff] [review]
Patch

I'll answer Mounir's question and refine the patch later.

Comment 13

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) from comment #10)
> ::: dom/mobilemessage/src/MmsMessage.cpp
> @@ +175,3 @@
> >        return NS_ERROR_INVALID_ARG;
> >      }
> > +    if (receiverJsVal.isString()) {
> 
> I'm not sure I understand what you are doing here. Isn't receiverJsVal the
> last value of the array?

No, the receiverJsVal is for each element value in this array, not just the last value. Anyway, I'm going to refine the patch with another different thought. ;)

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Hi reviewers,

After some thoughts, I decided to loose the restriction of checking empty receivers for some reasons:

1. The MMS notification indication doesn't carry any receivers. This kind of MMS message should have no receivers in nature so I decided to expose an empty array to the Gaia end as it is, which sounds more consistent with the MMS spec. The Gaia end can recognize this kind of MMS message with |.delivery == "not-downloaded"|.

2. I originally thought to add the user's phone number into the receivers array. However, sometimes the phone number is not available for some SIM cards, which means we might need to return something like [""] or [null]. After some thoughts, I think this is not a very formal way to tell Gaia site the lack of receiver is due to a SIM card issue (or not) from the API point of view.

So let's directly return an empty array for this kind of |.delivery == "not-downloaded"| MMS message. After retrieving MMS from server later, the receivers array must not be empty for a |.delivery == "received"| MMS message.

Comment 15

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

Comment on attachment 742937 [details] [diff] [review]
Patch, V2

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

As defined in MMS-ENC, "from" attribute in M-Notification.ind PDU is optional, so indeed we could have empty receiver array at the time.

Comment 16

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/projects/birch/rev/d28a41f49785

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d28a41f49785

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/499c3627e89a