Closed Bug 880563 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

()

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

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 file, 7 obsolete files)

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: nobody → gene.lian
Attached patch Patch (obsolete) — Splinter Review
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.
Attached patch patch - vicamo yet another impl. (obsolete) — Splinter Review
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.
Attachment #760848 - Flags: feedback?(ctai)
I'm trying to solve the timer issue based on Vicamo's patch.
Attached patch Gene's Patch based on Vicamo's (obsolete) — Splinter Review
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)
Attached patch WIP Patch (obsolete) — Splinter Review
Need more tests.
Attachment #761516 - Attachment is obsolete: true
Attachment #761974 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Attachment #764683 - Attachment is obsolete: true
Attachment #765293 - Flags: review?(vyang)
Attachment #765293 - Flags: review?(ctai)
Attached patch Patch V1.1 (obsolete) — Splinter Review
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.
Attached patch Patch, V2 (obsolete) — Splinter Review
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+
Attached patch Patch, V2.1Splinter Review
Addressing the nits. r=vicamo,ctai a=leo+
Attachment #767158 - Attachment is obsolete: true
Attachment #767727 - Flags: review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Priority: -- → P1
Blocks: 949801
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: