B2G MMS: deleting an MMS during the sending or retrieving process doesn't work properly

RESOLVED FIXED in Firefox 25

Status

()

P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: airpingu, Assigned: airpingu)

Tracking

Trunk
mozilla25
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 attachment, 7 obsolete attachments)

This is inspired by Steve. If the user attempts to delete an MMS when that MMS is still under the sending or retrieving process, bad things would happen in our current logic. We need to have proper returns and better DB synchronization to deal with this kind of scenario.

This might be a general user scenario. For example, if the user is trying to send "I love you" to his or her ex-lover, then he or she realizes this is wrong and is in a hurry to delete that when the sending circle is still spinning, the current logic cannot handle that properly.
Regardless of how many ex-lovers the typical user in our target market has, we should at least protect against this scenario in the UI by disabling the delete ability while send/receive is occurring to avoid bad things. Blocking+.
blocking-b2g: leo? → leo+
(Assignee)

Updated

6 years ago
Assignee: nobody → gene.lian
Created attachment 760848 [details] [diff] [review]
Patch

I haven't done the retrieving process and the SMS sending process.
Attachment #760848 - Flags: feedback?(vyang)
Attachment #760848 - Flags: feedback?(ctai)
Comment on attachment 760848 [details] [diff] [review]
Patch

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

::: dom/mobilemessage/src/ril/MmsService.js
@@ +824,5 @@
>     *        A callback function that takes two arguments: one for
>     *        X-Mms-Response-Status, the other for the parsed M-Send.conf message.
>     */
>    run: function run(callback) {
> +    this.isCancelled = false;

As you see in following code segment, |this.run| might be run the second time right after blobs are loaded.  This line should be moved into ctor, or, since you have initialize it in prototype, just remove it here.

@@ +905,5 @@
> +   */
> +  cancel: function cancel() {
> +    this.isCancelled = true;
> +    if (this.timer) {
> +      this.timer.cancel();

If we don't cancel |gMmsTransactionHelper.sendRequest()| as well, we have:

  1. disable data connection
  2. send MMS
  3. delete MMS (therefore SendTransaction is cancelled, error returned to Gaia)
  4. turn on data connection
  5. that MMS is still sent out.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1535,5 @@
>                if (DEBUG) debug("Message id " + messageId + " deleted");
>                deleted[messageIndex] = true;
>  
> +              Services.obs.notifyObservers(self.createDomMessageFromRecord(messageRecord),
> +                                           "mobile-message-deleted", null);

most of the time, that observer event has no receiver but we still create an expensive MmsMessage DOM object here.  Maybe carry |messageRecord.id| only?
Attachment #760848 - Flags: feedback?(vyang)
Quite often, I find myself wanting to cancel a send, not necessarily because I sent my message to an ex-lover ;) but I agree this is a new feature and a new API, and this should probably not be done by deleting the message (more probably, we'd want to tap on the spinning icon to cancel the sending).

But clearly gecko should protect against this :)
(In reply to Julien Wajsberg [:julienw] from comment #4)
> Quite often, I find myself wanting to cancel a send, not necessarily because
> I sent my message to an ex-lover ;) but I agree this is a new feature and a
> new API, and this should probably not be done by deleting the message (more
> probably, we'd want to tap on the spinning icon to cancel the sending).
> 
> But clearly gecko should protect against this :)

The "cancel" API seems to be yet another thing for me.  (To clarify, MMS also has an operation called "CANCELLING", which is issued by MMS-Proxy to ask the client to cancel a already retrieved MMS message.)  DOMRequest have no cancel() by nature.
Created attachment 761516 [details] [diff] [review]
patch - vicamo yet another impl.

Not yet tested.  Just want to address my own comment.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> (In reply to Julien Wajsberg [:julienw] from comment #4)
> > Quite often, I find myself wanting to cancel a send, not necessarily because
> > I sent my message to an ex-lover ;) but I agree this is a new feature and a
> > new API, and this should probably not be done by deleting the message (more
> > probably, we'd want to tap on the spinning icon to cancel the sending).
> > 
> > But clearly gecko should protect against this :)
> 
> The "cancel" API seems to be yet another thing for me.  (To clarify, MMS
> also has an operation called "CANCELLING", which is issued by MMS-Proxy to
> ask the client to cancel a already retrieved MMS message.)  DOMRequest have
> no cancel() by nature.

So many overloaded terms. We'll need to find good names :)
Thanks for you guys feedback! Especially for Vicamo's patch. :) After having the first glance on this patch, I had some concerns:

1. I'm not quite sure if it's good or not to save a DB ID in the SendTransaction structure which shouldn't be aware of the presence of DB. If you're OK, then I'm fine with that.

2. What's happening if we're wanting to cancel the SendTransaction when it is waiting to do the second retry by the timer? You don't really take care of that. Right? Or this patch is still under construction actually?

3. Regarding why I want to notify observers with the whole MmsMessage DOM object is because we need to do:

  Services.obs.notifyObservers(domMessage, kSmsFailedObserverTopic, null);
Comment on attachment 760848 [details] [diff] [review]
Patch

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

::: dom/mobilemessage/src/ril/MmsService.js
@@ +772,5 @@
>    timer: null,
>  
>    istreamComposed: false,
>  
> +  isCancelled: false,

I am not sure is it a good idea to use a flag to do this. It might cause some race condition.

@@ +824,5 @@
>     *        A callback function that takes two arguments: one for
>     *        X-Mms-Response-Status, the other for the parsed M-Send.conf message.
>     */
>    run: function run(callback) {
> +    this.isCancelled = false;

Agree with Vicamo.

@@ +959,5 @@
>  function MmsService() {
>    // TODO: bug 810084 - support application identifier
> +
> +  Services.obs.addObserver(this, kXpcomShutdownObserverTopic, false);
> +  Services.obs.addObserver(this, "mobile-message-deleted", false);

Something like kMobileMessageDeletedTopic?

@@ +1665,5 @@
> +        Services.obs.removeObserver(this, kXpcomShutdownObserverTopic);
> +        Services.obs.removeObserver(this, "mobile-message-deleted");
> +        break;
> +      }
> +      case "mobile-message-deleted": {

ditto.
I'm trying to solve the timer issue based on Vicamo's patch.
Created attachment 761974 [details] [diff] [review]
Gene's Patch based on Vicamo's

Summary:

1. Fixed the timer issue. That is, we can also cancel the sending/retrieving processes when they're waiting for the next retry managed by the timer.

2. I don't really like maintaining a |cancellables| in the |gMmsTransactionHelper|, because:

  A. Since the |SendTransaction| and |RetrieveTransaction| also needs to listen for the "mobile-message-deleted" to cancel the timer. Let's do it just at one place.

  B. It seems very weird to me that |gMmsTransactionHelper| has to be aware of the database status. It should only be in charge of handling the HTTP transaction and nothing else.

  C. What's happening if |SendTransaction| and |RetrieveTransaction| are destructed? The |gMmsTransactionHelper| has no way to remove the their elements from |.cancellables|.

3. Fixed some nits and made up some missing codes when refactorizing like: |xhr.onerror| and |catch (e) {...}|.
Attachment #760848 - Attachment is obsolete: true
Attachment #761974 - Flags: feedback?(vyang)
Attachment #761974 - Flags: feedback?(ctai)
Comment on attachment 761974 [details] [diff] [review]
Gene's Patch based on Vicamo's

I'll come back with a patch after ensuring everything is working.
Attachment #761974 - Flags: feedback?(vyang)
Attachment #761974 - Flags: feedback?(ctai)
Created attachment 764683 [details] [diff] [review]
WIP Patch

Need more tests.
Attachment #761516 - Attachment is obsolete: true
Attachment #761974 - Attachment is obsolete: true
Created attachment 765293 [details] [diff] [review]
Patch
Attachment #764683 - Attachment is obsolete: true
Attachment #765293 - Flags: review?(vyang)
Attachment #765293 - Flags: review?(ctai)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 885271
Created attachment 766571 [details] [diff] [review]
Patch V1.1

Rebased.
Attachment #765293 - Attachment is obsolete: true
Attachment #765293 - Flags: review?(vyang)
Attachment #765293 - Flags: review?(ctai)
Attachment #766571 - Flags: review?(vyang)
Attachment #766571 - Flags: review?(ctai)
Comment on attachment 766571 [details] [diff] [review]
Patch V1.1

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

I think that's almost done, just still a few things to improve before landing.

::: dom/mobilemessage/src/ril/MmsService.js
@@ +469,5 @@
> +        }
> +      };
> +
> +      cancellable.isAcquiringConn =
> +        !gMmsConnection.acquire((function (cancellable, connected) {

We can skip binding 'cancellable' here.

@@ +473,5 @@
> +        !gMmsConnection.acquire((function (cancellable, connected) {
> +
> +        cancellable.isAcquiringConn = false;
> +
> +        if (!connected || cancellable.isDone || cancellable.isCancelled) {

After some discussion, |cancellable.isDone| is only true when |cancellable.isCancelled| is true.  So we can remove it here.

@@ +502,5 @@
> +        // Always release the MMS network connection before callback.
> +        gMmsConnection.release();
> +        if (callback) {
> +          callback(httpStatus, data);
> +        }

We can skip nullity check here because 'callback' is always "cancellable.done.bind(cancellable)" here.

@@ +532,5 @@
> +
> +        // Setup event listeners
> +        xhr.onerror = function () {
> +        if (DEBUG) debug("xhr error, response headers: " +
> +                         xhr.getAllResponseHeaders());

nit: indent

@@ +756,5 @@
> +
> +  // nsIObserver
> +
> +  observe: function observe(subject, topic, data) {
> +    let cancelRunning = (function () {

Please move this function out as a (private) member function because `observe` may be called several times but all with a message id that differs from ours.

@@ +763,5 @@
> +      if (this.timer) {
> +        // The sending or retrieving process is waiting for the next retry.
> +        // What we only need to do is to cancel the timer.
> +        this.timer.cancel();
> +        this.timer = null;

this.runCallbackIfValid(_MMS_ERROR_MESSAGE_DELETED, null);

@@ +772,5 @@
> +        this.cancellable = null;
> +      }
> +
> +      // Fall through to run the callback and inform clients of the status.
> +      this.runCallbackIfValid(_MMS_ERROR_MESSAGE_DELETED, null);

We can have following in all gMmsTransactionHelper.sendRequest callback functions:

  if (httpStatus == _HTTP_STATUS_USER_CANCELLED) {
    callback(_MMS_ERROR_MESSAGE_DELETED, null);
    return;
  }

This way, we don't create another call path branch.

@@ +848,5 @@
> +      gMmsTransactionHelper.sendRequest("GET", this.contentLocation, null,
> +                                        (function (httpStatus, data) {
> +      if (httpStatus == _HTTP_STATUS_USER_CANCELLED) {
> +        // The transaction has been cancelled. No callback is required.
> +        return;

callback(_MMS_ERROR_MESSAGE_DELETED, null);
return;

@@ +946,5 @@
>    this.msg = msg;
>  }
> +SendTransaction.prototype = Object.create(CancellableTransaction.prototype, {
> +  istreamComposed: { value: false,
> +    enumerable: true, configurable: true, writable: true },

nit: one line per attribute.

@@ +1006,5 @@
>          this.istream = MMS.PduHelper.compose(null, this.msg);
>          this.istreamComposed = true;
> +        if (!this.isCancelled) {
> +          this.run(callback);
> +        }

if (this.isCancelled) {
  this.runCallbackIfValid(_MMS_ERROR_MESSAGE_DELETED, null);
} else {
  this.run(callback);
}

@@ +1046,5 @@
>     *        A callback function that takes two arguments: one for
>     *        X-Mms-Response-Status, the other for the parsed M-Send.conf message.
>     */
> +  send: { value: function send(callback) {
> +    this.timer = null;    

nit: trailing ws.

@@ +1054,5 @@
> +                                        this.istream,
> +                                        function (httpStatus, data) {
> +      if (httpStatus == _HTTP_STATUS_USER_CANCELLED) {
> +        // The transaction has been cancelled. No callback is required.
> +        return;

callback(_MMS_ERROR_MESSAGE_DELETED, null);
return;

@@ +1702,5 @@
> +        if (aMmsStatus == _MMS_ERROR_MESSAGE_DELETED) {
> +          errorCode = Ci.nsIMobileMessageCallback.NOT_FOUND_ERROR;
> +        } else if (aMmsStatus != MMS.MMS_PDU_ERROR_OK) {
> +          errorCode = Ci.nsIMobileMessageCallback.INTERNAL_ERROR;
> +        }

nit: I prefer follow style to skip one possible redundant assignment:

  let errorCode;
  if (foo) {
    errorCode = 1;
  } else if (bar) {
    errorCode = 2;
  } else {
    errorCode = 3;
  }
Attachment #766571 - Flags: review?(vyang)
Comment on attachment 766571 [details] [diff] [review]
Patch V1.1

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

::: dom/mobilemessage/src/ril/MmsService.js
@@ +469,5 @@
> +        }
> +      };
> +
> +      cancellable.isAcquiringConn =
> +        !gMmsConnection.acquire((function (cancellable, connected) {

Looks like the argument connected will always be true?

@@ +478,5 @@
>            gMmsConnection.release();
> +
> +          if (!cancellable.isDone) {
> +            cancellable.done(cancellable.isCancelled ?
> +                             _HTTP_STATUS_USER_CANCELLED : 0, null);

What is 0 mean? We should not use magic number right here.

@@ +762,5 @@
> +
> +      if (this.timer) {
> +        // The sending or retrieving process is waiting for the next retry.
> +        // What we only need to do is to cancel the timer.
> +        this.timer.cancel();

Any chance of race condition occurs right here?

@@ +873,3 @@
>            (retrieveStatus != MMS.MMS_PDU_ERROR_OK)) {
>          callback(MMS.translatePduErrorToStatus(retrieveStatus),
>                          retrieved);

Can we put retrieved in the same line?

@@ +1046,5 @@
>     *        A callback function that takes two arguments: one for
>     *        X-Mms-Response-Status, the other for the parsed M-Send.conf message.
>     */
> +  send: { value: function send(callback) {
> +    this.timer = null;    

nit. Space behind null;
Attachment #766571 - Flags: review?(ctai)
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #18)
> Comment on attachment 766571 [details] [diff] [review]
> Patch V1.1
> 
> Review of attachment 766571 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/ril/MmsService.js
> @@ +469,5 @@
> > +        }
> > +      };
> > +
> > +      cancellable.isAcquiringConn =
> > +        !gMmsConnection.acquire((function (cancellable, connected) {
> 
> Looks like the argument connected will always be true?

No, it could be false when timeout.

> 
> @@ +478,5 @@
> >            gMmsConnection.release();
> > +
> > +          if (!cancellable.isDone) {
> > +            cancellable.done(cancellable.isCancelled ?
> > +                             _HTTP_STATUS_USER_CANCELLED : 0, null);
> 
> What is 0 mean? We should not use magic number right here.

The original logic is using it to specify the fail of http request in the error catching. Please see:

  releaseMmsConnectionAndCallback(0, null);

I'd prefer filing another bug to fix this. It's too close to the deadline to have a new mechanism, which is error-prone.

> 
> @@ +762,5 @@
> > +
> > +      if (this.timer) {
> > +        // The sending or retrieving process is waiting for the next retry.
> > +        // What we only need to do is to cancel the timer.
> > +        this.timer.cancel();
> 
> Any chance of race condition occurs right here?

No, as discussed in person. All the tasks should be run in the main thread.

> 
> @@ +873,3 @@
> >            (retrieveStatus != MMS.MMS_PDU_ERROR_OK)) {
> >          callback(MMS.translatePduErrorToStatus(retrieveStatus),
> >                          retrieved);
> 
> Can we put retrieved in the same line?

OK.

> 
> @@ +1046,5 @@
> >     *        A callback function that takes two arguments: one for
> >     *        X-Mms-Response-Status, the other for the parsed M-Send.conf message.
> >     */
> > +  send: { value: function send(callback) {
> > +    this.timer = null;    
> 
> nit. Space behind null;

OK.
Created attachment 767158 [details] [diff] [review]
Patch, V2
Attachment #766571 - Attachment is obsolete: true
Attachment #767158 - Flags: review?(vyang)
Attachment #767158 - Flags: review?(ctai)
Note: this bug needs to be landed before Thursday morning.
Btw, I'll land this by myself after getting review+.
Comment on attachment 767158 [details] [diff] [review]
Patch, V2

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

Great job. Looks good to me.

::: dom/mobilemessage/src/ril/MmsService.js
@@ +874,5 @@
> +      }).bind(this));
> +    },
> +    enumerable: true,
> +    configurable: true,
> +    writable: true 

nit:spcae...
Attachment #767158 - Flags: review?(ctai) → review+
I got a Gaia error when deleting the MMS during the sending process:

E/GeckoConsole( 5835): Content JS LOG at app://sms.gaiamobile.org/js/message_manager.js:442 in onError: Error Sending: undefined

Maybe we should also have some Gaia changes as well.
(In reply to Gene Lian [:gene] from comment #24)
> I got a Gaia error when deleting the MMS during the sending process:
> 
> E/GeckoConsole( 5835): Content JS LOG at
> app://sms.gaiamobile.org/js/message_manager.js:442 in onError: Error
> Sending: undefined
> 
> Maybe we should also have some Gaia changes as well.

Oh, it's just a normal log. I doesn't matter. The Gecko fix should be sufficient.
Comment on attachment 767158 [details] [diff] [review]
Patch, V2

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

::: dom/mobilemessage/src/ril/MmsService.js
@@ +519,5 @@
> +
> +        // Setup event listeners
> +        xhr.onerror = function () {
> +        if (DEBUG) debug("xhr error, response headers: " +
> +                         xhr.getAllResponseHeaders());

nit: indentation
Attachment #767158 - Flags: review?(vyang) → review+
Created attachment 767727 [details] [diff] [review]
Patch, V2.1

Addressing the nits. r=vicamo,ctai a=leo+
Attachment #767158 - Attachment is obsolete: true
Attachment #767727 - Flags: review+
https://hg.mozilla.org/projects/birch/rev/8c464cf6e5ca
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e3456aec01cf
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/8c464cf6e5ca
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/e3456aec01cf
status-b2g-v1.1hd: affected → fixed
status-firefox23: --- → wontfix
status-firefox24: --- → wontfix
status-firefox25: --- → fixed

Updated

6 years ago
Priority: -- → P1
(Assignee)

Updated

5 years ago
Blocks: 949801
You need to log in before you can comment on or make changes to this bug.