839436 - B2G MMS: make DB be able to save MMS messages

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

We need to make MobileMessageDatabaseService.js be able to save the MMS messages and also keep the backward-compatibility for the existing SMS DB functions at the same time.

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, provide nsIDOMMozMmsMessage (WIP)

Comment 2

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, provide APIs to save/mark MMS messages (WIP)

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Vicamo,

Eventually, I didn't pass the message object into the functions because of some experiences learned from my previous reviewers:

1. It's not good to wrap all the parameters into an object, which makes us hard to know the needed inputs by looking at the API definition.

2. It's not good to save everything into the DB until we really need it, so that the DB space for saving messages can be estimated and under control.

Comment 4

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Fix some nits. Please see comment #3 for the summary.

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3

Comment 6

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.1

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.2

Comment 8

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

Comment on attachment 716394 [details] [diff] [review]
Patch, V3.2

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

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +54,5 @@
>  
>  const GLOBAL_SCOPE = this;
>  
> +function getNumbersFromRecord(aRecord) {
> +  return aRecord.delivery == DELIVERY_RECEIVED ? [aRecord.sender] : aRecord.receivers;

That's still something we still have problems in bug 833291 :(

@@ +447,5 @@
> +                                          aRecord.body,
> +                                          aRecord.messageClass,
> +                                          aRecord.timestamp,
> +                                          aRecord.read);
> +    } else if (aRecord.type == "mms") {

You don't need |else| or even |if (aRecord.type == "mms")| here. We should ensure the database has only two types of messages.

@@ +527,5 @@
> +        // TODO: we skip the current message we got if it's not
> +        // a SMS one. Restores the waiting request and calls the
> +        // function again to get the next one when not processing.
> +        // Needs to find a way to handle MMS message in the future.
> +        aMessageList.requestWaiting = smsRequest;

Reset the request back to requestWaiting may result in racing condition. The onsuccess callback is executed asynchronously. So there is a small time window between:

  let smsRequest = aMessageList.requestWaiting;
  aMessageList.requestWaiting = null;
  ...
  getRequest.onsuccess = function onsuccess(event) {
    let record = event.target.result;
    if (record.type != "sms") {
      aMessageList.requestWaiting = smsRequest;

You should probably take smsRequest when that step is assured. But, why bother? We have no official MMS support yet. I'd rather kill the data flow in WapPushManager for now, and leave the database code here as it should be.

@@ +733,5 @@
> +      // Next update the other objectStore.
> +      let numbers = getNumbersFromRecord(aRecord);
> +      for (let i = 0; i < numbers.length; i++) {
> +        let number = numbers[i];
> +        stores[1].get(number).onsuccess = function onsuccess(event) {

Update mostRecenStore here relates to how do we define a thread. So, bug 833291 :(

@@ +778,3 @@
>  
> +    let receivers = aReceivers;
> +    if (!receivers) {

Why? I can't understand :(

Comment 9

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.3

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.4

Comment 11

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.5

Comment 12

[Gene Lian (I already quit Mozilla)]

Something is still wrong in the RadioInterfaceLayer.js. I'll try to fix them tomorrow...

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V4

Hi Vicamo,

This patch is working well without bugs. Please take a review when you have a chance. Please feel free to let me know if any details can be improved. Thanks!

Run the try at the same time: https://tbpl.mozilla.org/?tree=Try&rev=422dd864ecc9

Comment 14

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

Comment on attachment 717483 [details] [diff] [review]
Patch, V4

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1460,5 @@
>      }
>  
> +    let messageSender = message.sender || null;
> +    let messageReceiver = message.receiver || null;
> +    let messageFullBody = message.fullBody || null;

Per offline discussion, the reason you have these saved variables is to fill the message object carried by system messenger. However, we should keep the objects sent via different channels the same. I mean, the original code is wrong, let's fix it here.

@@ +1499,5 @@
>            delivery: DOM_SMS_DELIVERY_RECEIVED,
>            deliveryStatus: RIL.GECKO_SMS_DELIVERY_STATUS_SUCCESS,
> +          sender: messageSender,
> +          receiver: messageReceiver,
> +          body: messageFullBody,

ditto

@@ +1510,5 @@
>  
> +    message.type = "sms";
> +    message.sender = messageSender;
> +    message.receiver = messageReceiver;
> +    message.body = messageFullBody;

ditto

@@ +2580,5 @@
>      let timestamp = Date.now();
>      let deliveryStatus = options.requestStatusReport
>                         ? RIL.GECKO_SMS_DELIVERY_STATUS_PENDING
>                         : RIL.GECKO_SMS_DELIVERY_STATUS_NOT_APPLICABLE;
> +    let id = gMobileMessageDatabaseService.saveSendingMessage({

please have |let record = {...}| instead.

Comment 15

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V4.1

Comment 16

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V4.2

Comment 17

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V4.3

Comment 18

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

Comment on attachment 717824 [details] [diff] [review]
Patch, V4.3

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

Thank you :)

Comment 19

[Gene Lian (I already quit Mozilla)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=304eb1a15722

Comment 20

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd5d8525ace

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bd5d8525ace

Comment 22

[Gene Lian (I already quit Mozilla)]

This bug relates to MMS features and needs to be tagged as leo+ so that we can uplift it into the b2g-18 branch.

Comment 23

[Dylan Oliver [:doliver]]

Leo triage: leo+ for MMS blockers

Comment 24

[Gene Lian (I already quit Mozilla)]

Attachment: b2g18-specific-patch

Due to the conflict at Bug 830164 (s/icc/iccInfo is only applied in m-c but not b2g18), we need to prepare a b2g18-specific patch for this bug.

Comment 25

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8feb4680772f

Comment 26

[Ryan VanderMeulen [:RyanVM]]

The entire set of clian's pushes was backed out for multiple reasons.
https://tbpl.mozilla.org/?tree=Mozilla-B2g18&rev=a0b06192f882

1.) Not every patch you pushed had leo+, tracking+, or approval+ to land. Every patch that lands needs these approvals whether they block another one or not.
2.) The tree rules are clear that you are not to land on top of bustage. At the time you pushed, both B2G Mn and B2G xpcshell had bustage from prior commits that hadn't been backed out yet.
3.) The tree rules are also clear that you are to watch your pushes for any bustage and handle them accordingly. mozilla-inbound is the ONLY tree where this rule does not apply.
4.) Even after the earlier bustage was backed out, something in one of your many pushes was causing further B2G Mn failures as shown in the log below.
https://tbpl.mozilla.org/php/getParsedLog.php?id=20424173&tree=Mozilla-B2g18
5.) This isn't cause for backout by itself, but it is also strongly preferred to not push each commit individually as our build and testing resources are limited and doing so stretches them even thinner. Please limit your number of pushes as much as possible unless you have good reason for keeping them separate.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Ignore #1, my bad. The rest still stands.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/04732631cf91

We'll see what happens with Marionette...

Comment 29

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/04732631cf91
> 
> We'll see what happens with Marionette...

We'll take care of it.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/04732631cf91
> 
> We'll see what happens with Marionette...

Yep, looks like this is the patch that caused the Marionette failures noted in comment 26. Backed out.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ad36b4b684c7

https://tbpl.mozilla.org/php/getParsedLog.php?id=20567378&tree=Mozilla-B2g18

Comment 31

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

Attachment: b2g18-specific-patch : v2

s/iccInfo/icc/

Comment 32

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #31)
> Created attachment 723968 [details] [diff] [review]
> b2g18-specific-patch : v2

Verified locally. Will push after last tbpl commit turns (nearly) all green.

Comment 33

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

Attachment: b2g18-specific-patch : v2

Re-gen with hg. Landing ...

Comment 34

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6c767edf8f72

Comment 35

[Gene Lian (I already quit Mozilla)]

Added [NO_UPLIFT][RIL_INTERFACE] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out.

------------------------------
If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible:

Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643)
Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages
Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS
Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments.
Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741).
Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS
Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix)
Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix)
Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event
Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead().
Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete().
Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage(). 
Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
Bug 845643 - B2G MMS: Save retrieved MMS into database.
Bug 792321 - Check max values of MMS parameters in sendRequest.
Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS
Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS
Bug 839436 - B2G MMS: make DB be able to save MMS messages.

Comment 36

[Gene Lian (I already quit Mozilla)]

Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.

Comment 37

[Gene Lian (I already quit Mozilla)]

Per off-line discussion with Michael, we decided not to back out the MMS bugs that have already been in mozilla-b2g18. Removing [NO_UPLIFT] to make the check-in status sync'ed.