904993 - B2G MMS: Return proper error codes when failing to retrieve MMS due to any network reasons.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

+++ This bug was initially created as a clone of Bug #880561 +++

This is symmetric to bug 880561. I believe we need to do the same thing for retrieving MMS.

In the current logic, users are still allowed to manually download MMS even if the airplane mode is on, causing the downloading circle spinning forever, which is obviously wrong. The expected behaviour should be to pop up a warning prompt to the user to stop them downloading MMS in advance when the airplane mode is on, SIM card is not installed or any other reasons that imply the current network is not available.

Comment 1

[Gene Lian (I already quit Mozilla)]

Hi Ctai, are you interested in taking this?

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-904993.patch v1.0

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-904993.patch v1.0

Comment 4

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

Comment on attachment 790644 [details] [diff] [review]
bug-904993.patch v1.0

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

Let's try to bury |gMmsConnection.radioDisabled|, |gRadioInterface.rilContext.cardState| calls deep into |gMmsConnection.acquire|, keep MmsService neutral to all platforms as possible.

Comment 5

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-904993.patch-v1.1

Comment 6

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

Comment on attachment 792117 [details] [diff] [review]
bug-904993.patch-v1.1

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +46,5 @@
>  const _HTTP_STATUS_USER_CANCELLED = -1;
> +const _HTTP_STATUS_ACQUIRE_CONNECTION_SUCCESS = 0;
> +const _HTTP_STATUS_RADIO_DISABLED = 1;
> +const _HTTP_STATUS_NO_SIM_CARD = 2;
> +const _HTTP_STATUS_ACQUIRE_TIMEOUT = 3;

Keep them of negative values.

@@ +51,5 @@
>  
>  // Non-standard MMS status for internal use.
>  const _MMS_ERROR_MESSAGE_DELETED = -1;
> +const _MMS_ERROR_RADIO_DISABLED = 1;
> +const _MMS_ERROR_NO_SIM_CARD = 2;

ditto.

@@ +257,5 @@
> +      if (gRadioInterface.rilContext.cardState != "ready") {
> +        if (DEBUG) debug("Error! SIM card is not ready when sending MMS.");
> +        callback(false, _HTTP_STATUS_NO_SIM_CARD);
> +        return true;
> +      }

We should have the two checks inside |if (!this.connected)| block.  That means we only check these state before we're going to call |setupDataCallByType()|.  Besides, in the cases that we're going to return false, we should also fire |onConnectTimerTimeout| as well.  And since we're going to reuse |onConnectTimerTimeout|, rename it to |flushPendingCallbacks| will be nice.

As a whole, you may probably have:

  flushPendingCallbacks: function flushPendingCallbacks(status) {
    while (this.pendingCallbacks.length) {
      let callback = this.pendingCallbacks.shift();
      callback(false, status);
    }
  },

  acquire: function acquire(callback) {
    this.connectTimer.cancel();

    if (!this.connected) {
      this.pendingCallbacks.push(callback);

      let errorStatus;
      if (this.radioDisabled) {
        errorStatus = _HTTP_STATUS_RADIO_DISABLED;
      } else if (gRadioInterface.rilContext.cardState != "ready") {
        errorStatus = _HTTP_STATUS_NO_SIM_CARD;
      }

      if (errorStatus != null) {
        flushPendingCallbacks(errorStatus);
        return true;
      }

      gRadioInterface.setupDataCallByType("mms");

      this.connectTimer.initWithCallback(this.flushPendingCallbacks
                                             .bind(this, _HTTP_STATUS_ACQUIRE_TIMEOUT),
                                         TIME_TO_BUFFER_MMS_REQUESTS,
                                         Ci.nsITimer.TYPE_ONE_SHOT);
      return false;
    }
    ....

@@ +365,1 @@
>            }

Reuse |flushPendingCallbacks(_HTTP_STATUS_ACQUIRE_CONNECTION_SUCCESS)|

@@ +884,5 @@
> +        }
> +
> +        if (httpStatus == _HTTP_STATUS_NO_SIM_CARD) {
> +          callback(_MMS_ERROR_NO_SIM_CARD, null);
> +          return;

Let's have a |translateHttpStatusToMmsStatus|:

  function translateHttpStatusToMmsStatus(httpStatus, defaultStatus) {
    switch(httpStatus) {
      case _HTTP_STATUS_USER_CANCELLED:
        return _MMS_ERROR_MESSAGE_DELETED;
      case _HTTP_STATUS_RADIO_DISABLED:
        return _MMS_ERROR_RADIO_DISABLED;
      case _HTTP_STATUS_NO_SIM_CARD:
        return _MMS_ERROR_NO_SIM_CARD;
      case HTTP_STATUS_OK:
        return MMS.MMS_PDU_ERROR_OK;
      default:
        return defaultStatus;
    }
  }

Then we have shorter code here:

  let mmsStatus = translateHttpStatusToMmsStatus(httpStatus, MMS.MMS_PDU_STATUS_DEFERRED);
  if (mmsStatus != MMS.MMS_PDU_ERROR_OK) {
    callback(mmsStatus, null);
    return;
  }

  if (!data) {
    callback(MMS.MMS_PDU_STATUS_DEFERRED, null);
    return;
  }

@@ +1121,5 @@
> +
> +        if (httpStatus == _HTTP_STATUS_NO_SIM_CARD) {
> +          callback(_MMS_ERROR_NO_SIM_CARD, null);
> +          return;
> +        }

And here:

  let mmsStatus = translateHttpStatusToMmsStatus(httpStatus, MMS.MMS_PDU_ERROR_TRANSIENT_FAILURE);

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

We will block sending to email via MMS in gecko part until we support it.

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Wrong duplicate.

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-904993.patch-v1.2

Comment 10

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

Comment on attachment 795295 [details] [diff] [review]
bug-904993.patch-v1.2

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

Still some nits to fix.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +180,5 @@
>          let callback = this.pendingCallbacks.shift();
> +        let connected = false;
> +        if (status == _HTTP_STATUS_ACQUIRE_CONNECTION_SUCCESS) {
> +          connected = true;
> +        }

nit: |let connected = (status == _HTTP_STATUS_ACQUIRE_CONNECTION_SUCCES);|

@@ +341,5 @@
>        this.settings.forEach(function(name) {
>          Services.prefs.removeObserver(name, this);
>        }, this);
>        this.connectTimer.cancel();
> +      this.flushPendingCallbacks();

pass _HTTP_STATUS_RADIO_DISABLED.

@@ +891,5 @@
>          gMmsTransactionHelper.sendRequest("GET", this.contentLocation, null,
>                                            (function (httpStatus, data) {
> +        let mmsStatus = gMmsTransactionHelper.
> +                          translateHttpStatusToMmsStatus(httpStatus,
> +                            MMS.MMS_PDU_STATUS_DEFERRED);

nit:

  let mmsStatus = gMmsTransactionHelper
                  .translateHttpStatusToMmsStatus(httpStatus,
                                                  MMS.MMS_PDU_STATUS_DEFERRED);

This has still more than 80 chars per line, but that's just nothing more we can do.

@@ +1128,1 @@
>  

nit: redundant empty line

@@ +1424,5 @@
>                                                              mmsStatus,
>                                                              retrievedMessage) {
> +    if (mmsStatus == _MMS_ERROR_RADIO_DISABLED ||
> +        mmsStatus == _MMS_ERROR_NO_SIM_CARD) {
> +      mmsStatus = MMS.MMS_PDU_STATUS_DEFERRED;

In these cases, we can't even send a NotifyResponse out.  So instead of re-assigning them to |MMS.MMS_PDU_STATUS_DEFERRED|, we should have:

  if (MMS.MMS_PDU_STATUS_RETRIEVED !== mmsStatus) {
    if (mmsStatus != _MMS_ERROR_RADIO_DISABLED &&
        mmsStatus != _MMS_ERROR_NO_SIM_CARD) {
      let transaction = new NotifyResponseTransaction(...);
      transaction.run();
    }
    ...
    return;
  }

Besides, please also help correct the line that assigns |transactionId|.  The |retrievedMessage| may be null in some cases, so we should have following code instead:

  let transactionId = savableMessage.headers["x-mms-transaction-id"];

@@ +1985,5 @@
>            aRequest.notifyGetMessageFailed(Ci.nsIMobileMessageCallback.NOT_FOUND_ERROR);
>            return;
>          }
>  
> +        let errorCode = Ci.nsIMobileMessageCallback.INTERNAL_ERROR;

Move into another |else if (mmsStatus != MMS.MMS_PDU_STATUS_RETRIEVED)| and let errorCode be undefined by default.  This way, we don't accidentally find errorCode has some value while the message has been successfully retrieved.

@@ +1990,5 @@
> +        if (mmsStatus == _MMS_ERROR_RADIO_DISABLED) {
> +          errorCode = Ci.nsIMobileMessageCallback.RADIO_DISABLED_ERROR;
> +        }
> +
> +        if (mmsStatus == _MMS_ERROR_NO_SIM_CARD) {

nit: |else if|

@@ +1998,4 @@
>          // If the mmsStatus is still MMS_PDU_STATUS_DEFERRED after retry,
>          // we should not store it into database and update its delivery
>          // status to 'error'.
>          if (MMS.MMS_PDU_STATUS_RETRIEVED !== mmsStatus) {

|if (errorCode)|

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-904993.patch v1.3

Comment 12

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

Comment on attachment 796497 [details] [diff] [review]
bug-904993.patch v1.3

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

Great job!

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +280,5 @@
>        }
>  
>        this.refCount++;
>  
> +      callback(true, Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR);

nit: _HTTP_STATUS_ACQUIRE_CONNECTION_SUCCESS

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-904993.patch v1.4

Fix nit. Vicamo, thanks for your great review.

Comment 14

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

https://hg.mozilla.org/integration/b2g-inbound/rev/b541479a5bf3

Comment 15

[Ed Morley [:emorley]]

Backed out for mochitest-5 failures (surprised this managed to cause these, but retriggers seem to leave no other alternative! :-s):
https://tbpl.mozilla.org/php/getParsedLog.php?id=27156301&tree=B2g-Inbound

https://hg.mozilla.org/integration/b2g-inbound/rev/e6f813b9c46d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

The *reftest*-5 failures were not caused by this. Re-landed.

https://hg.mozilla.org/integration/b2g-inbound/rev/682b93724feb

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/682b93724feb

Comment 18

[Gene Lian (I already quit Mozilla)]

Nominating for leo+ since bug 921945 is a potential leo+ bug.

Comment 19

[Preeti Raghunath(:Preeti)]

Gene,

It is too late for leo+. Please specify user impact of living with this bug for another 4 months.

Comment 20

[Gene Lian (I already quit Mozilla)]

This bug happens when:

1. Switch MMS settings to manual download.
2. Send an MMS to FFOS from other device.
3. Turn on airplane mode.
4. Tap the download button.
5. The downloading circle spins forever (until reboot the device) (KO).

The expected behaviour at step #5 should be immediately pop up a prompt to stop users downloading MMS because airplane mode is on.

I'm fine with koi+ or 1.3+ if leo vendors can tolerate that.

Comment 21

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #20)
> This bug happens when:
> 
> 1. Switch MMS settings to manual download.
> 2. Send an MMS to FFOS from other device.
> 3. Turn on airplane mode.
> 4. Tap the download button.
> 5. The downloading circle spins forever (until reboot the device) (KO).
                                  ^^^^^^^
Sorry, I'm wrong. It will internally go into the retry mechanism. After 4 times retries (totally 2560 seconds later [1]), the spinning circle will still be able to become the exclamation mark. However, it's still bad to take such a long time.

With this patch, it will become the exclamation mark immediately without doing more retries. Again, marking leo+ or not depends on what you guys think how bad it is.

[1] The platform has an automatic retry mechanism internally, which will try at most 4 times until the MMS is successfully downloaded. The time interval between each retry is 60, 300, 600, 1800 seconds respectively.

> 
> The expected behaviour at step #5 should be immediately pop up a prompt to
> stop users downloading MMS because airplane mode is on.
> 
> I'm fine with koi+ or 1.3+ if leo vendors can tolerate that.

Comment 22

[lecky]

(In reply to Gene Lian [:gene] (Summit + PTOs 10/3 ~ 10/14) from comment #20)
> This bug happens when:

1. Switch MMS settings to manual download.
2. Send
> an MMS to FFOS from other device.
3. Turn on airplane mode.
4. Tap the
> download button.
5. The downloading circle spins forever (until reboot the
> device) (KO).

The expected behaviour at step #5 should be immediately pop
> up a prompt to stop users downloading MMS because airplane mode is on.

I'm
> fine with koi+ or 1.3+ if leo vendors can tolerate that.


Hi Gene&Wayne:
We don't think this issue could be accepted for terminal user from our historic experience on V1.1HD. Could you help us to merge it into V1.1HD?

HI Beatriz:
What's your opinion? Do you think this issue would block V1.1HD? Thanks!

Comment 23

[Beatriz Rodríguez [:brg]]

(In reply to lecky from comment #22)
> HI Beatriz:
> What's your opinion? Do you think this issue would block V1.1HD? Thanks!
We do not need this in v1.1. We need it landing in v1.2 as a possible certification blocker. If Mozilla agrees, we can move it to koi?

Comment 24

[Gene Lian (I already quit Mozilla)]

It's PM's call. I'm fine with either decision.

Comment 25

[Wayne Chang [:wchang]]

With comment 21, this recovers itself (although with a rather long delay ~42mins) rather than being stuck forever. 

This is probably sufficient for now to not be blocking.

Moving this to koi? with this and comment 23.

Thanks Gene for the update.

Comment 26

[Andrew Overholt [:overholt]]

I'm fine with a potential cert blocker + already landed elsewhere code getting uplifted so koi+.

John, are you doing uplifts?

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

Ryan, can you take this Gecko uplift?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

This landed on m-c when it was still 26.