922580 - B2G MMS: should stop downloading MMS immediately when airplane mode is on

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 #904993 +++

Bug 904993 solves one problem but we would still have another tricky case as below:

1. Switch MMS settings to manual download.
2. Send an MMS to FFOS from other device.
3. Tap on the download button.
4. Turn on airplane mode *immediately*.
5. The downloading circle will spin after 4 times retries: totally 2560 seconds later [1].

A better behaviour for step #5 should be directly stop retries and mark the spinning circle to be the exclamation mark immediately.

Note: this bug happens on V1.1 so nominate this for leo+.

[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.

Comment 1

[Preeti Raghunath(:Preeti)]

Seems like a feature enhancement for 1.1 or 1.2

Comment 2

[Gene Lian (I already quit Mozilla)]

Hi Bevis, this looks like a good first bug for the newbie. Are you interested? If you're willing to take this one, I can guide you to work it out. :)

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Gene,

Sure! I am glad to have chance to start some contribution to our product.

Bevis

Comment 4

[Kai-Chih Hu [:khu]]

Confirmed with RIL team and we planed and expected to land it to 1.3.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Fix the UI problem of the downloading indication when user turn on airplane mode right after request a manual MMS download.

The fix of this bug is to
1. Add an observer to the ril.radio.disabled preference to allow the transaction to be canceled when this preference is toggled to TRUE.
2. minor change to re-factor the observed preference keys used in this module.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Update the test results in FET network with unagi:
1. Regression Test is positive for
   - Send MMS.
   - Receive MMS with both auto/manaual download.
2. Airplane mode turn on right after sending receiving MMS
   - the progress bar will be changed to a 'Warning' icon instead which fix the scenario mentioned in this bug.

Comment 7

[Gene Lian (I already quit Mozilla)]

Thanks Bevis! I'll take a look by today. :)

Comment 8

[Gene Lian (I already quit Mozilla)]

Comment on attachment 820776 [details] [diff] [review]
Part 1: Fix the UI problem of the downloading indication when user turn on airplane mode right after request a manual MMS download.

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +86,5 @@
>  
> +const OBSERVED_PREF_KEY_MMSC             = "ril.mms.mmsc";
> +const OBSERVED_PREF_KEY_MMSPROXY         = "ril.mms.mmsproxy";
> +const OBSERVED_PREF_KEY_MMSPORT          = "ril.mms.mmsport";
> +const OBSERVED_PREF_KEY_RADIO_DISABLED   = "ril.radio.disabled";

Nice to clean this up. :) But I'd prefer using the following naming:

const kPrefRilMmsc          = "ril.mms.mmsc";
const kPrefRilMmsProxy      = "ril.mms.mmsproxy";
const kPrefRilMmsPort       = "ril.mms.mmsport";
const kPrefRilRadioDisabled = "ril.radio.disabled";

Also, since you make these changes please replace all the preference keys by the new consts within this file.

@@ +160,5 @@
>      proxyInfo: null,
> +    settings: [OBSERVED_PREF_KEY_MMSC,
> +               OBSERVED_PREF_KEY_MMSPROXY,
> +               OBSERVED_PREF_KEY_MMSPORT,
> +               OBSERVED_PREF_KEY_RADIO_DISABLED],

Ditto.

@@ +375,5 @@
>            this.flushPendingCallbacks(_HTTP_STATUS_ACQUIRE_CONNECTION_SUCCESS)
>            break;
>          }
>          case kPrefenceChangedObserverTopic: {
> +          if (data == OBSERVED_PREF_KEY_RADIO_DISABLED) {

Ditto.

@@ +380,2 @@
>              try {
> +              this.radioDisabled = Services.prefs.getBoolPref(OBSERVED_PREF_KEY_RADIO_DISABLED);

Ditto.

@@ +763,5 @@
>    registerRunCallback: function registerRunCallback(callback) {
>      if (!this.isObserversAdded) {
>        Services.obs.addObserver(this, kXpcomShutdownObserverTopic, false);
>        Services.obs.addObserver(this, kMobileMessageDeletedObserverTopic, false);
> +      Services.prefs.addObserver(OBSERVED_PREF_KEY_RADIO_DISABLED, this, false);

Ditto.

@@ +775,5 @@
>    removeObservers: function removeObservers() {
>      if (this.isObserversAdded) {
>        Services.obs.removeObserver(this, kXpcomShutdownObserverTopic);
>        Services.obs.removeObserver(this, kMobileMessageDeletedObserverTopic);
> +      Services.prefs.removeObserver(OBSERVED_PREF_KEY_RADIO_DISABLED, this);

Ditto.

@@ +831,5 @@
>          this.cancelRunning();
>          break;
>        }
> +      case kPrefenceChangedObserverTopic: {
> +        if (data == OBSERVED_PREF_KEY_RADIO_DISABLED) {

Ditto.

@@ +835,5 @@
> +        if (data == OBSERVED_PREF_KEY_RADIO_DISABLED) {
> +          try {
> +            let radioDisabled = Services.prefs.getBoolPref(OBSERVED_PREF_KEY_RADIO_DISABLED);
> +            if (radioDisabled) {
> +              this.runCallbackIfValid(_MMS_ERROR_RADIO_DISABLED, null);

You don't need to call this line.

@@ +836,5 @@
> +          try {
> +            let radioDisabled = Services.prefs.getBoolPref(OBSERVED_PREF_KEY_RADIO_DISABLED);
> +            if (radioDisabled) {
> +              this.runCallbackIfValid(_MMS_ERROR_RADIO_DISABLED, null);
> +              this.cancelRunning();

Please add a parameters for .cancelRunning(mmsStatus) and calls this.cancelRunning(_MMS_ERROR_RADIO_DISABLED);

  cancelRunning: function cancelRunning(mmsStatus) {
    this.isCancelled = true;

    if (this.timer) {
      ...
      this.runCallbackIfValid(mmsStatus, null);
      return;
    }

    ...
  },

To do so,

  observe: function observe(subject, topic, data) {
    switch (topic) {
      case kXpcomShutdownObserverTopic: {
        this.cancelRunning(_MMS_ERROR_SHUTDOWN);
        break;
      }
      case kMobileMessageDeletedObserverTopic: {
        data = JSON.parse(data);
        if (data.id != this.cancellableId) {
          return;
        }

        this.cancelRunning(_MMS_ERROR_MESSAGE_DELETED);
        break;
      }
    }
  }

where _MMS_ERROR_SHUTDOWN is a new one and we need to handle that properly. This one is not your fault but we can clean it up as well. :)

Btw, please help correct:

const _HTTP_STATUS_ACQUIRE_TIMEOUT = -4;

@@ +841,5 @@
> +            }
> +          } catch (e) {
> +            if (DEBUG) debug("Failed to get preference of 'ril.radio.disabled'.");
> +          }
> +         } /* if (data == OBSERVED_PREF_KEY_RADIO_DISABLED) */

Don't need this comment in general. Also, please align the "}".

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Fix the UI problem of the downloading indication when user turn on airplane mode right after request a manual MMS download. V2

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Fix the UI problem of the downloading indication when user turn on airplane mode right after request a manual MMS download. V3

Revice the patch with:
1. Apply correct naming convention of the constant declaration.
2. Add additional mmsStatus parameter in function cancelRunning().
3. Always invoke runCallbackIfValid in cancelRunning(). The reasons are:
   - It is more precise to notify upper layer because the mmsStatus is specified and the task is cancel. (No need to wait the callback implicited from HTTP request response.
   - after reviewing the implementation, it is safe to invoke it here because runCallback is only invoked in runCallbackIfValid(), and runCallbackIfValid() will only be invoked once the send/receive request is done.

Comment 11

[Gene Lian (I already quit Mozilla)]

Comment on attachment 821584 [details] [diff] [review]
Part 1: Fix the UI problem of the downloading indication when user turn on airplane mode right after request a manual MMS download. V3

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +776,5 @@
>    removeObservers: function removeObservers() {
>      if (this.isObserversAdded) {
>        Services.obs.removeObserver(this, kXpcomShutdownObserverTopic);
>        Services.obs.removeObserver(this, kMobileMessageDeletedObserverTopic);
> +      Services.prefs.removeObserver(kPrefRilRadioDisabled, this);

You don't need to call this. The Services.prefs will internally remove observers when shutting down.

@@ +799,3 @@
>      this.isCancelled = true;
>  
> +    this.runCallbackIfValid(mmsStatus, null);

I'd prefer not moving this to here. It sounds a bit weird to me because you call runCallbackIfValid in advance but it still has extra processes in the this.cancellable.cancel(). We should let cancellable runs runCallbackIfValid eventually due to the _HTTP_STATUS_USER_CANCELLED, which is a complete process. To do this, you need the following changes:

1. Keep the mmsStatus as a |this.cancelledReason| in the CancellableTransaction before running the this.cancellable.cancel();.

2. Change translateHttpStatusToMmsStatus to eat the above-mentioned |this.cancelledReason|. When httpStatus == _HTTP_STATUS_USER_CANCELLED, return that cancelledReason. To do this, you might need to .bind(this) for sendRequest(..., function() {...}.bind(this)).

3. You need to properly handle _MMS_ERROR_SHUTDOWN like:

  if (aMmsStatus == _MMS_ERROR_MESSAGE_DELETED) {
    errorCode = Ci.nsIMobileMessageCallback.NOT_FOUND_ERROR;
  }

4. In SendTransaction:

  if (this.isCancelled) {
    this.runCallbackIfValid(this.cancelledReason, null);
  }

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Fix the UI problem of the downloading indication when user turn on airplane mode right after request a manual MMS download. V4

Revise the patch again as followed:
1. Apply naming rules of the constant definition.
2. Add new Observer for the change of the preference |ril.radio.disabled| to cancel the transactions accordingly.
   - Services.prefs.removeObserver(kPrefRilRadioDisabled, this) is still needed because the observer is unregistered when transaction is done istead of the shutdown case.
3. accept Gene's suggestion to apply the original runCallback flow when HTTP request is sent.
4. refactor cancelRunning() with |reason| parameter cached in the |cancelledReason| of each CancellableTransaction instance.
5. Translate the |_HTTP_STATUS_USER_CANCELLED| to the cached |cancelledReason| if available in gMmsTransactionHelper.translateHttpStatusToMmsStatus().
6. 2 new internal MMS status definition are created:
    - const _MMS_ERROR_SHUTDOWN                      = -4;
    - const _MMS_ERROR_USER_CANCELLED_WITHOUT_REASON = -5;
      - This is a placeholder if |cancelledReason| is not specified when the transaction is cancel. 
      - Currently, the |cancelledReason| is always specified.
    - These 2 status will be mapped implicitedly to |nsIMobileMessageCallback.INTERNAL_ERROR| in current implementation of MmsService.retrieve()/MmsService.send().

Comment 13

[Gene Lian (I already quit Mozilla)]

Comment on attachment 823234 [details] [diff] [review]
Part 1: Fix the UI problem of the downloading indication when user turn on airplane mode right after request a manual MMS download. V4

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +57,5 @@
> +const _MMS_ERROR_MESSAGE_DELETED               = -1;
> +const _MMS_ERROR_RADIO_DISABLED                = -2;
> +const _MMS_ERROR_NO_SIM_CARD                   = -3;
> +const _MMS_ERROR_SHUTDOWN                      = -4;
> +const _MMS_ERROR_USER_CANCELLED_WITHOUT_REASON = -5;

s/_MMS_ERROR_USER_CANCELLED_WITHOUT_REASON/_MMS_ERROR_USER_CANCELLED_NO_REASON which is simpler.

@@ +679,5 @@
>      },
>  
> +    translateHttpStatusToMmsStatus: function translateHttpStatusToMmsStatus(httpStatus,
> +                                                                            defaultStatus,
> +                                                                            cancelledReason) {

Could you please move cancelledReason to the second parameter?

@@ +764,5 @@
>    runCallback: null,
>  
>    isObserversAdded: false,
>  
> +  cancelledReason: _MMS_ERROR_USER_CANCELLED_WITHOUT_REASON,

s/_MMS_ERROR_USER_CANCELLED_WITHOUT_REASON/_MMS_ERROR_USER_CANCELLED_NO_REASON

@@ +918,5 @@
>                                            (function (httpStatus, data) {
>          let mmsStatus = gMmsTransactionHelper
>                          .translateHttpStatusToMmsStatus(httpStatus,
> +                                                        MMS.MMS_PDU_STATUS_DEFERRED,
> +                                                        this.cancelledReason);

Move to the second param.

@@ +1147,5 @@
>  
>        this.cancellable =
>          gMmsTransactionHelper.sendRequest("POST", gMmsConnection.mmsc,
>                                            this.istream,
>                                            function (httpStatus, data) {

You have to bind(this) for this callback function. Please see how the retrieve works. ;)

Directly give you review+ but please make sure fix this and give it a test before landing.

@@ +1151,5 @@
>                                            function (httpStatus, data) {
>          let mmsStatus = gMmsTransactionHelper.
>                            translateHttpStatusToMmsStatus(httpStatus,
> +                            MMS.MMS_PDU_ERROR_TRANSIENT_FAILURE,
> +                            this.cancelledReason);

This won't work. Please see above.

Also, please move to the second param.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Cancel the transaction when user toggle airplane mode to ON. V5 . r=gene

1. rename _MMS_ERROR_USER_CANCELLED_NO_REASON.
2. move cancelledReason in translateHttpStatusToMmsStatus() to 2nd parameter.
3. Fix the missing 'bind(this)' inside SendTransaction.send() when assigning callback.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Cancel the transaction when user toggle airplane mode to ON. V6 . r=gene

Re-fine function.bind(this) coding style.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server link:
https://tbpl.mozilla.org/?tree=Try&rev=8244bf0c30ec

Patch has also been verified locally in the following conditions:
1. Send/Receiving with Auto/Manually Download.
2. Msg will be stopped correctly if user toggles airplane one when MMS transaction is ongoing.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a0d1499d5a0