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

Status: RESOLVED FIXED

Description

[Gene Lian (I already quit Mozilla)]

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.

Comment 1

[Dietrich Ayala (:dietrich)]

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

Comment 2

[Gene Lian (I already quit Mozilla)]

Created attachment 760848 [details] [diff] [review]
Patch

I haven't done the retrieving process and the SMS sending process.

Comment 3

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

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?

Comment 4

[Julien Wajsberg [:julienw]]

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 :)

Comment 5

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

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

Comment 6

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

Created attachment 761516 [details] [diff] [review]
patch - vicamo yet another impl.

Not yet tested.  Just want to address my own comment.

Comment 7

[Julien Wajsberg [:julienw]]

(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 :)

Comment 8

[Gene Lian (I already quit Mozilla)]

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 9

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

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.

Comment 10

[Gene Lian (I already quit Mozilla)]

I'm trying to solve the timer issue based on Vicamo's patch.

Comment 11

[Gene Lian (I already quit Mozilla)]

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) {...}|.

Comment 12

[Gene Lian (I already quit Mozilla)]

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.

Comment 13

[Gene Lian (I already quit Mozilla)]

Created attachment 764683 [details] [diff] [review]
WIP Patch

Need more tests.

Comment 14

[Gene Lian (I already quit Mozilla)]

Created attachment 765293 [details] [diff] [review]
Patch

Comment 16

[Gene Lian (I already quit Mozilla)]

Created attachment 766571 [details] [diff] [review]
Patch V1.1

Rebased.

Comment 17

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

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;
  }

Comment 18

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

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;

Comment 19

[Gene Lian (I already quit Mozilla)]

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

Comment 20

[Gene Lian (I already quit Mozilla)]

Created attachment 767158 [details] [diff] [review]
Patch, V2

Comment 21

[Gene Lian (I already quit Mozilla)]

Note: this bug needs to be landed before Thursday morning.

Comment 22

[Gene Lian (I already quit Mozilla)]

Btw, I'll land this by myself after getting review+.

Comment 23

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

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

Comment 24

[Gene Lian (I already quit Mozilla)]

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.

Comment 25

[Gene Lian (I already quit Mozilla)]

(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 26

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

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

Comment 27

[Gene Lian (I already quit Mozilla)]

Created attachment 767727 [details] [diff] [review]
Patch, V2.1

Addressing the nits. r=vicamo,ctai a=leo+

Comment 28

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/projects/birch/rev/8c464cf6e5ca
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e3456aec01cf

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/e3456aec01cf