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)
Tracking
()
People
(Reporter: airpingu, Assigned: airpingu)
References
Details
(Whiteboard: [fixed-in-birch])
Attachments
(1 file, 7 obsolete files)
35.60 KB,
patch
|
airpingu
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
Assignee: nobody → gene.lian
Assignee | ||
Comment 2•12 years ago
|
||
I haven't done the retrieving process and the SMS sending process.
Attachment #760848 -
Flags: feedback?(vyang)
Attachment #760848 -
Flags: feedback?(ctai)
Comment 3•12 years ago
|
||
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)
Comment 4•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
Not yet tested. Just want to address my own comment.
Comment 7•12 years ago
|
||
(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 :)
Assignee | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #760848 -
Flags: feedback?(ctai)
Assignee | ||
Comment 10•12 years ago
|
||
I'm trying to solve the timer issue based on Vicamo's patch.
Assignee | ||
Comment 11•12 years ago
|
||
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)
Assignee | ||
Comment 12•12 years ago
|
||
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)
Assignee | ||
Comment 13•12 years ago
|
||
Need more tests.
Attachment #761516 -
Attachment is obsolete: true
Attachment #761974 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #764683 -
Attachment is obsolete: true
Attachment #765293 -
Flags: review?(vyang)
Attachment #765293 -
Flags: review?(ctai)
Assignee | ||
Comment 16•12 years ago
|
||
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 17•12 years ago
|
||
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 18•12 years ago
|
||
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)
Assignee | ||
Comment 19•12 years ago
|
||
(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.
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #766571 -
Attachment is obsolete: true
Attachment #767158 -
Flags: review?(vyang)
Attachment #767158 -
Flags: review?(ctai)
Assignee | ||
Comment 21•12 years ago
|
||
Note: this bug needs to be landed before Thursday morning.
Assignee | ||
Comment 22•12 years ago
|
||
Btw, I'll land this by myself after getting review+.
Comment 23•12 years ago
|
||
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+
Assignee | ||
Comment 24•12 years ago
|
||
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.
Assignee | ||
Comment 25•12 years ago
|
||
(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•12 years ago
|
||
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+
Assignee | ||
Comment 27•12 years ago
|
||
Addressing the nits. r=vicamo,ctai a=leo+
Attachment #767158 -
Attachment is obsolete: true
Attachment #767727 -
Flags: review+
Assignee | ||
Comment 28•12 years ago
|
||
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]
Comment 29•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 30•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•