Closed Bug 877064 Opened 8 years ago Closed 8 years ago

B2G MMS: Retry sending MMS fail.

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: ctai, Assigned: ctai)

References

Details

(Whiteboard: RN6/7)

Attachments

(1 file, 5 obsolete files)

Looks like something on the retrying send MMS.
Block an leo+ bug.
blocking-b2g: --- → leo?
Summary: B2G MMS: Fix retring send MMS. → B2G MMS: Retry sending MMS fail.
Attached patch Patch v1.0 (obsolete) — Splinter Review
Assignee: nobody → ctai
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #755209 - Attachment is obsolete: true
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #755212 - Attachment is obsolete: true
Attachment #755213 - Flags: feedback?(vyang)
Whiteboard: RN5/29
Whiteboard: RN5/29 → RN6/7
Attached patch Patch v1.3 (obsolete) — Splinter Review
Attachment #755213 - Attachment is obsolete: true
Attachment #755213 - Flags: feedback?(vyang)
Attachment #755860 - Flags: feedback?(vyang)
Root cause:
Users of instances of nsITimer should keep a reference to the timer until it is no longer needed in order to assure the timer is fired.

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsITimer
Comment on attachment 755860 [details] [diff] [review]
Patch v1.3

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

::: dom/mobilemessage/src/ril/MmsService.js
@@ +625,5 @@
>  function RetrieveTransaction(contentLocation) {
>    this.contentLocation = contentLocation;
> +
> +  // We need to keep a reference to the timer to assure the timer is fired.
> +  this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

Please only create a timer when necessary.

@@ +748,5 @@
>      msg.headers["content-type"] = contentType;
>    }
>  
> +  // We need to keep a reference to the timer to assure the timer is fired.
> +  this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

ditto

::: modules/libpref/src/init/all.js
@@ +4207,5 @@
>  // never: Never retrieval mode.
>  pref("dom.mms.retrieval_mode", "manual");
>  
>  pref("dom.mms.sendRetryCount", 3);
> +pref("dom.mms.sendRetryInterval", 180000);

Any reason to shorten the retry interval here?
Attachment #755860 - Flags: feedback?(vyang)
Attached patch Patch v1.4 (obsolete) — Splinter Review
Attachment #755860 - Attachment is obsolete: true
Attachment #756467 - Flags: review?(vyang)
Comment on attachment 756467 [details] [diff] [review]
Patch v1.4

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

::: dom/mobilemessage/src/ril/MmsService.js
@@ +625,5 @@
>  function RetrieveTransaction(contentLocation) {
>    this.contentLocation = contentLocation;
> +
> +  // We need to keep a reference to the timer to assure the timer is fired.
> +  this.timer = null;

nit: you can simply move this line into |RetrieveTransaction.prototype|.

@@ +639,5 @@
>      let that = this;
>      this.retrieve((function retryCallback(mmsStatus, msg) {
>        if (MMS.MMS_PDU_STATUS_DEFERRED == mmsStatus &&
>            that.retryCount < PREF_RETRIEVAL_RETRY_COUNT) {
> +        if (that.retryCount === 0) {

nit: please check nullity of |that.timer| instead.  We don't need an extra pre-condition "timer is null when retryCount is zero" even it's true.

@@ +752,5 @@
>      msg.headers["content-type"] = contentType;
>    }
>  
> +  // We need to keep a reference to the timer to assure the timer is fired.
> +  this.timer = null;

ditto

@@ +836,5 @@
>      let retryCallback = (function (mmsStatus, msg) {
>        if ((MMS.MMS_PDU_ERROR_TRANSIENT_FAILURE == mmsStatus ||
>              MMS.MMS_PDU_ERROR_PERMANENT_FAILURE == mmsStatus) &&
>            this.retryCount < PREF_SEND_RETRY_COUNT) {
> +        if (this.retryCount === 0) {

ditto.
Attachment #756467 - Flags: review?(vyang) → review+
Can you mark this bug as leo+?
This bug will cause the sending icon spin forever.
Thanks.
Flags: needinfo?(dietrich)
Attached patch Patch v1.5Splinter Review
Attachment #756467 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f8a0aa9350c9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
blocking-b2g: leo? → leo+
Flags: needinfo?(dietrich)
Flags: in-moztrap-
Duplicate of this bug: 875491
You need to log in before you can comment on or make changes to this bug.