Closed Bug 815236 Opened 12 years ago Closed 12 years ago

SMS sent multiple times

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-basecamp:+)

Status:
VERIFIED FIXED
Milestone:
B2G C2 (20nov-10dec)
Project Flags:
blocking-basecamp +

People

(Reporter: gerard-majax, Assigned: ochameau)

Details

Attachments

(2 files)

Pull request 6834
166 bytes, text/html
Details
Pull request 6853 - smaller patch
166 bytes, text/html
borjasalguero
: review+
Details 
Somehow the 'send' button is getting click event multiple times which results in the same message being sent multiple times, i.e. the recipient gets the same SMS twice (or worst).
Component: Gaia → Gaia::SMS
Bug 815470 which I filed moments ago is similar - the undefined sender is actually a duplicate conversation from what I can see.
Who's your provider ?
Flags: needinfo?(lissyx+mozillians)
NRJ Mobile as of now, but I did reproduced it also with other providers. Anyway, I'm more
Flags: needinfo?(lissyx+mozillians)
But I think it's more a UX matter: it seems you can enable the send button multiple times while you should not be able to.
I see it using AT&T.
I would block on this. This cost money.
Alexandre pinged me on IRC and says his issue in this bug is definitely different than what I am seeing in Bug 815470, so disregard what I said in Comment 5.
blocking-basecamp: ? → +
Priority: -- → P1
Target Milestone: --- → B2G C2 (20nov-10dec)
Assignee: nobody → fbsc
With the following:
- Gaia: "6632aade33f503af089539ac06558634e8bad641"
- Gecko: "20ae4f1189670a957d0892e0462a3c9d9b6fe0d3"

It's working properly. Movistar SIM Card to Orange SIM Card. SMS App has not changed the behaviour, so it should work.
It's getting worse and worse with the number of SMS in the database.
[:gerard-majax] Could you specify the Gaia/Gecko commits in order to test the same? Thanks!
Sure.

gaia @ 7db6596
gecko @ 3768c31

And by worse, I mean that sometimes it now triggers 16 (yes) deliveries. It seems to correlate with slowliness due to the amount of SMS in the thread. What I suspect here is that the send event is triggered multiple times due to latency/lack of reactiveness.

I don't own the phone right now, it's someone else who is dogfooding daily (using this nexus s running b2g as a replacement for currently dead phone while waiting for another one).
(In reply to Alexandre LISSY :gerard-majax from comment #11)
> I don't own the phone right now, it's someone else who is dogfooding daily
> (using this nexus s running b2g as a replacement for currently dead phone
> while waiting for another one).

Are you sure you still don't want us to send you a dogfood unit :)
Still reproducing with 90f1645.
Our target device for developing and testing is the Unagi, until we cannot reproduce the bug in that device I think it should not be a blocker
blocking-basecamp: + → ?
If it happens with a Unagi/Otoro, please specify:
- SIM Card used
- Gaia/Gecko version
- Scenario (It happens clicking 'send' fast, it happens every time we send a SMS...?)
- Logs if available.
This has definitively nothing to do with the SIM card, and it's definitively the 'click' event arriving multiple times on the send button. See the following dump and corresponding 'fix', tested right now on Nexus S:

Before the "fix":
-----------------
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1135 in thui_sendMessage: sendMessage: entering
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1151 in thui_sendMessage: sendMessage: has settings
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1135 in thui_sendMessage: sendMessage: entering
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1151 in thui_sendMessage: sendMessage: has settings
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1157 in onsuccess: sendMessage: RIL status: false
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1157 in onsuccess: sendMessage: RIL status: false
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1203 in onsave: sendMessage: calling MessageManager.send()
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1203 in onsave: sendMessage: calling MessageManager.send()
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1205 in onsent: sendMessage: MessageManager.send() onsent()
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1205 in onsent: sendMessage: MessageManager.send() onsent()
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1135 in thui_sendMessage: sendMessage: entering
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1151 in thui_sendMessage: sendMessage: has settings
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1135 in thui_sendMessage: sendMessage: entering
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1151 in thui_sendMessage: sendMessage: has settings
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1157 in onsuccess: sendMessage: RIL status: false
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1157 in onsuccess: sendMessage: RIL status: false
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1203 in onsave: sendMessage: calling MessageManager.send()
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1203 in onsave: sendMessage: calling MessageManager.send()
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1205 in onsent: sendMessage: MessageManager.send() onsent()
E/GeckoConsole(  432): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1205 in onsent: sendMessage: MessageManager.send() onsent()

[...]
After the "fix":
----------------
E/GeckoConsole(  629): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1143 in thui_sendMessage: sendMessage: entering
E/GeckoConsole(  629): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1159 in thui_sendMessage: sendMessage: has settings
E/GeckoConsole(  629): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1136 in thui_sendMessage: sendMessage: already performing ...
E/GeckoConsole(  629): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1165 in onsuccess: sendMessage: RIL status: false
E/GeckoConsole(  629): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1211 in onsave: sendMessage: calling MessageManager.send()
E/GeckoConsole(  629): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1213 in onsent: sendMessage: MessageManager.send() onsent()

-------------------------------------------------------------------------------------
The "fix"
-------------------------------------------------------------------------------------

diff --git a/apps/sms/js/sms.js b/apps/sms/js/sms.js
index 52ee8bf..bacdddb 100644
--- a/apps/sms/js/sms.js
+++ b/apps/sms/js/sms.js
@@ -1129,8 +1129,18 @@ var ThreadUI = {
   },
 
   sendMessage: function thui_sendMessage(resendText) {
+    var locked = this.isSMSLocked;
+    if (locked == undefined || locked == false) {
+      this.isSMSLocked = true;
+    } else {
+      console.debug('sendMessage: already performing ...');
+      return;
+    }
+
     var settings = window.navigator.mozSettings,
         throwGeneralError;
+    
+    console.debug("sendMessage: entering");
 
     throwGeneralError = function() {
       CustomDialog.show(
@@ -1146,11 +1156,14 @@ var ThreadUI = {
     };
 
     if (settings) {
+      console.debug("sendMessage: has settings");
 
       var req = settings.createLock().get('ril.radio.disabled');
       req.addEventListener('success', (function onsuccess() {
         var status = req.result['ril.radio.disabled'];
 
+        console.debug("sendMessage: RIL status: " + status);
+
         // Retrieve num depending on hash
         var hash = window.location.hash;
         // Depending where we are, we get different num
@@ -1195,7 +1208,9 @@ var ThreadUI = {
               // XXX Once we have PhoneNumberJS in Gecko we will
               // use num directly:
               // https://bugzilla.mozilla.org/show_bug.cgi?id=809213
+              console.debug("sendMessage: calling MessageManager.send()");
               MessageManager.send(numNormalized, text, function onsent(msg) {
+                console.debug("sendMessage: MessageManager.send() onsent()");
                 var root = document.getElementById(message.timestamp.getTime());
                 if (root) {
                   //Removing delivery spinner
@@ -1216,6 +1231,7 @@ var ThreadUI = {
                     }
                 });
               }, function onerror() {
+                console.debug("sendMessage: MessageManager.send() onerror()");
                 var root = document.getElementById(message.timestamp.getTime());
                 PendingMsgManager.deleteFromMsgDB(message,
                   function ondelete(msg) {
@@ -1272,6 +1288,7 @@ var ThreadUI = {
   },
 
   resendMessage: function thui_resendMessage(message) {
+    console.debug("resendMessage: entering");
     var resendConfirmStr = _('resend-confirmation');
     var result = confirm(resendConfirmStr);
     if (result) {
@@ -1285,6 +1302,7 @@ var ThreadUI = {
         }, filter, true);
       });
     }
+    console.debug("resendMessage: exiting");
   },
 
   renderContactData: function thui_renderContactData(contact) {
With my Unagi, latest Gecko & Gaia. Same log:

E/GeckoConsole(  396): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1135 in thui_sendMessage: sendMessage: entering
E/GeckoConsole(  396): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1151 in thui_sendMessage: sendMessage: has settings
E/GeckoConsole(  396): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1155 in onsuccess: sendMessage: RIL status: false
E/GeckoConsole(  396): " {file: "app://sms.gaiamobile.org/shared/js/phoneNumberJS/PhoneNumber.js" line: 56 column: 0 source: "call to eval() or related function blocked by CSP"}]
E/GeckoConsole(  396): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1200 in onsave: sendMessage: calling MessageManager.send()
E/GeckoConsole(  396): Content JS DEBUG at app://sms.gaiamobile.org/js/sms.js:1202 in onsent: sendMessage: MessageManager.send() onsent()
blocking-basecamp: ? → +
Debugging I found the following bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=817563
blocking-basecamp: + → ?
I would like to know if this bug is reproducible with Unagi/Otoro devices. If it is, let me know Gaia & Gecko versions and I will try again to review if there is any problem here. Otherwise as Samsung Galaxy S is out of the scope this bug is not blocking basecamp.

[David] Could you try to reproduce the same?
Flags: needinfo?(dscravaglieri)
I have reproduced it on my unagi. Alexandre ochameau too. I suggest ochameau takes this issue.
blocking-basecamp: ? → +
Flags: needinfo?(dscravaglieri)
Assignee: fbsc → poirot.alex
All yours Alexandre

Next time, when reporting a bug or reproducing it, it will be good to provide both gecko and gaia tips. That will save a lot of time to QA, Managers and developers.

Just a curiosity in order to ensure same builds are behaving in the same way and we don't have an even bigger problem:

David: which gecko and gaia tips are you using?
Alexandre: are you using the same ones that David?

For instance, having a look at the logs it seems that CSP is activated in our builds and not in yours...
(In reply to Daniel Coloma:dcoloma from comment #21)
> All yours Alexandre
> 
> Next time, when reporting a bug or reproducing it, it will be good to
> provide both gecko and gaia tips. That will save a lot of time to QA,
> Managers and developers.

Which I did as soon as I could, in comment #11, because I had other things on the fire.
(In reply to Alexandre LISSY :gerard-majax from comment #22)
> (In reply to Daniel Coloma:dcoloma from comment #21)
> > All yours Alexandre
> > 
> > Next time, when reporting a bug or reproducing it, it will be good to
> > provide both gecko and gaia tips. That will save a lot of time to QA,
> > Managers and developers.
> 
> Which I did as soon as I could, in comment #11, because I had other things
> on the fire.

But you are testing in Nexus, I was asking about the tips used when reproducing it in the Unagi. We need to have exactly the same environment to check and fix.
(In reply to Daniel Coloma:dcoloma from comment #23)
> (In reply to Alexandre LISSY :gerard-majax from comment #22)
> > (In reply to Daniel Coloma:dcoloma from comment #21)
> > > All yours Alexandre
> > > 
> > > Next time, when reporting a bug or reproducing it, it will be good to
> > > provide both gecko and gaia tips. That will save a lot of time to QA,
> > > Managers and developers.
> > 
> > Which I did as soon as I could, in comment #11, because I had other things
> > on the fire.
> 
> But you are testing in Nexus, I was asking about the tips used when
> reproducing it in the Unagi. We need to have exactly the same environment to
> check and fix.

I don't have a Unagi.
(In reply to Alexandre LISSY :gerard-majax from comment #24)
> (In reply to Daniel Coloma:dcoloma from comment #23)
> > (In reply to Alexandre LISSY :gerard-majax from comment #22)
> > > (In reply to Daniel Coloma:dcoloma from comment #21)
> > > > All yours Alexandre
> > > > 
> > > > Next time, when reporting a bug or reproducing it, it will be good to
> > > > provide both gecko and gaia tips. That will save a lot of time to QA,
> > > > Managers and developers.
> > > 
> > > Which I did as soon as I could, in comment #11, because I had other things
> > > on the fire.
> > 
> > But you are testing in Nexus, I was asking about the tips used when
> > reproducing it in the Unagi. We need to have exactly the same environment to
> > check and fix.
> 
> I don't have a Unagi.

People making it bb+ should have Unagis and should provide those tips they used to reproduce it.
(In reply to Daniel Coloma:dcoloma from comment #23)
> (In reply to Alexandre LISSY :gerard-majax from comment #22)
> > (In reply to Daniel Coloma:dcoloma from comment #21)
> > > All yours Alexandre
> > > 
> > > Next time, when reporting a bug or reproducing it, it will be good to
> > > provide both gecko and gaia tips. That will save a lot of time to QA,
> > > Managers and developers.
> > 
> > Which I did as soon as I could, in comment #11, because I had other things
> > on the fire.
> 
> But you are testing in Nexus, I was asking about the tips used when
> reproducing it in the Unagi. We need to have exactly the same environment to
> check and fix.

Daniel,

Next time he would be nice to check if you're speaking with an employee or a contributor that does not have an access to a Unagi. It will save volunteer a lot of time to not have to say: I don't have a Unagi. :)

2 other persons has reproduced it with an Unagi.

Steps to reproduce:
 - Go to the sms app and create a new message
 - Hit quickly the send button a few times

Actual results:
 - multiple sms are sent

Expected result:
 - only one sms is sent

This does not seems to happens all the time though and I believe David is running the stable branch on his phone.
(In reply to Vivien Nicolas (:vingtetun) from comment #26)
> (In reply to Daniel Coloma:dcoloma from comment #23)
> > (In reply to Alexandre LISSY :gerard-majax from comment #22)
> > > (In reply to Daniel Coloma:dcoloma from comment #21)
> > > > All yours Alexandre
> > > > 
> > > > Next time, when reporting a bug or reproducing it, it will be good to
> > > > provide both gecko and gaia tips. That will save a lot of time to QA,
> > > > Managers and developers.
> > > 
> > > Which I did as soon as I could, in comment #11, because I had other things
> > > on the fire.
> > 
> > But you are testing in Nexus, I was asking about the tips used when
> > reproducing it in the Unagi. We need to have exactly the same environment to
> > check and fix.
> 
> Daniel,
> 
> Next time he would be nice to check if you're speaking with an employee or a
> contributor that does not have an access to a Unagi. It will save volunteer
> a lot of time to not have to say: I don't have a Unagi. :)

I think it was obvious, but I was asking dscravaglieri or ochameau for the tips, as they were the ones reproducing it on Unagis. 

My question in comment 21:

"David: which gecko and gaia tips are you using?
Alexandre: are you using the same ones that David?"

With Alexandre was referring to ochameau... I did not notice they were two Alexander in the loop, my bad.

Sorry gerard-majax about that and making you spending 1 minute (I am guessing how fast you type :-)) you did not have an Unagi. 

> 
> 2 other persons has reproduced it with an Unagi.
> 
> Steps to reproduce:
>  - Go to the sms app and create a new message
>  - Hit quickly the send button a few times
> 
> Actual results:
>  - multiple sms are sent
> 
> Expected result:
>  - only one sms is sent
> 

Yes, this was pretty much clear since comment 1. 

> This does not seems to happens all the time though and I believe David is
> running the stable branch on his phone.

Now, the important point I was trying to make (it seems unsuccessfully) since comment 21 and for which I do not have answer yet. So, let me repeat it, in order to avoid confusions:

Alexandre Ochameau, David Scravaglieri, can you provide us the Gaia and Gecko tips to reproduce it in the Unagis?

Although you may believe it is kind of funny (yes, I know it is good to have some humour but today is Monday and it is harder for me), not being able to reproduce in our side might mean we are not building the images in the same way and hence we may see different behaviours not only in this regard but in many others. I am afraid of that and that is why I am insisting in this topic that much.
Flags: needinfo?(dscravaglieri)
Here they are:

gecko-beta: 4077b03b8c26
gaia: 92c343c3b376ec8b6bee3c55c8bbb6fb7acd4d25
Flags: needinfo?(dscravaglieri)
(In reply to Daniel Coloma:dcoloma from comment #27)
> 
> I think it was obvious, but I was asking dscravaglieri or ochameau for the
> tips, as they were the ones reproducing it on Unagis. 
>

It was not :)
 
> My question in comment 21:
> 
> "David: which gecko and gaia tips are you using?
> Alexandre: are you using the same ones that David?"
> 
> With Alexandre was referring to ochameau... I did not notice they were two
> Alexander in the loop, my bad.
> 
> Sorry gerard-majax about that and making you spending 1 minute (I am
> guessing how fast you type :-)) you did not have an Unagi. 
>

I feel like he spent much more time than that on IRC trying to raise the issue.
 
> > 
> > 2 other persons has reproduced it with an Unagi.
> > 
> > Steps to reproduce:
> >  - Go to the sms app and create a new message
> >  - Hit quickly the send button a few times
> > 
> > Actual results:
> >  - multiple sms are sent
> > 
> > Expected result:
> >  - only one sms is sent
> > 
> 
> Yes, this was pretty much clear since comment 1. 
> 

My bad I misread comment 25.

> 
> Although you may believe it is kind of funny (yes, I know it is good to have
> some humour but today is Monday and it is harder for me), not being able to
> reproduce in our side might mean we are not building the images in the same
> way and hence we may see different behaviours not only in this regard but in
> many others. I am afraid of that and that is why I am insisting in this
> topic that much.

Not really funny as you said. Do you have a list of other issues where you can not reproduce and we can? If there are plenty it would be good to find the root cause of this.


And btw, thanks for beeing pragmatic.
> > 
> > Although you may believe it is kind of funny (yes, I know it is good to have
> > some humour but today is Monday and it is harder for me), not being able to
> > reproduce in our side might mean we are not building the images in the same
> > way and hence we may see different behaviours not only in this regard but in
> > many others. I am afraid of that and that is why I am insisting in this
> > topic that much.
> 
> Not really funny as you said. Do you have a list of other issues where you
> can not reproduce and we can? If there are plenty it would be good to find
> the root cause of this.
> 

I heard a couple of other topics this morning and I panic then. 

It seems that the problem is easier to be fixed. I'll follow-up over e-mail.
Attached file Pull request 6834 — 
So. I had to break sendMessage in multiple sub-functions.
That's mainly to ease readability of this code and to better control the state of it. Like ensuring that we don't send messages multiple times, nor completely lock sending messages.

I removed setTimeout calls in pendingDBUtils:saveToMsgDB that were wrong and even if they were correct, would most likely introduce infinite loops.

There is two safety locks here:
* The input is disabled via `ThreadUI.input.disabled` ASAP and enabled again right after the call to `ThreadUI.cleanFields()` that empty the input content.
That will prevent duplicated messages being sent. I'm reproducing this error quite easily by double tapping on Send button.
* I added another lock. The `sent` variable ensures that we will try to send the message only once. I wasn't able to reproduce such error, but it doesn't cost much. As we have lots of asynchronous code with lots of chained callbacks, there is always a risk of calling one of these callbacks more than once...

I extensively tested it with two phones by sending messages and trying to send many of them in burst. I tested the loss of network and the resend feature.
Attachment #688845 - Flags: review?(fernando.campo)
[:ochameu] I was checking the code, but I would like to know where was the bug previously and if you could tell us in which version of Gecko & Gaia is easy to reproduce.

We have to refactor all this code once we test PhoneNumberJS functionality in Gecko, so despite your code looks nice we are going to remove the majority of the functions related with your patch (saving in PendingDB, modifying the state to 'sending', error mechanism...) once all bugs tracked here https://bugzilla.mozilla.org/show_bug.cgi?id=806592 will be ready.

I would prefer a small patch only for fixing the bug. Thanks!
(In reply to Borja Salguero [:borjasalguero] from comment #32)
> [:ochameu] I was checking the code, but I would like to know where was the
> bug previously and if you could tell us in which version of Gecko & Gaia is
> easy to reproduce.

There is no particular need of any very specific version as we are used to reproduce it over time.
  gaia: 8783efcdb98f7fd77ebc2e4269c2272a7553dc4f
  gecko: 4857665ccf07afc0af1ead3952a7dac1037329dd
The important thing to reproduce it is to stress the phone with either having a big message history and/or trying to send multiple messages.

> 
> We have to refactor all this code once we test PhoneNumberJS functionality
> in Gecko, so despite your code looks nice we are going to remove the
> majority of the functions related with your patch (saving in PendingDB,
> modifying the state to 'sending', error mechanism...) once all bugs tracked
> here https://bugzilla.mozilla.org/show_bug.cgi?id=806592 will be ready.

Can you give me non-meta tracking bug, where such patches are going to be submitted ?
As there is a majority of platform bugs and bb- gaia bugs, I'm bit sceptical about such refactoring, are we really going to do such refactoring after C2?

In any case, I'll take a look again at old code and see if I can provide a smaller patch.
Well, main bugs are bb+ and the majority have a patch being reviewed/tested. It could be great if the patch is only related with the 'lock' you mentioned before. Thanks!
(In reply to Borja Salguero [:borjasalguero] (limited access to email until 10th December) from comment #34)
> Well, main bugs are bb+ and the majority have a patch being reviewed/tested.
> It could be great if the patch is only related with the 'lock' you mentioned
> before. Thanks!

There is not reasons to block on a non-c2 bugs here. Does the patch here actually fix the issue? Does it have any regressions? Does the code looks good?

If Yes. No. Yes. then let's land it.
(In reply to Vivien Nicolas (:vingtetun) from comment #35)
> (In reply to Borja Salguero [:borjasalguero] (limited access to email until
> 10th December) from comment #34)
> > Well, main bugs are bb+ and the majority have a patch being reviewed/tested.
> > It could be great if the patch is only related with the 'lock' you mentioned
> > before. Thanks!
> 
> There is not reasons to block on a non-c2 bugs here.

The other issues are C2:

https://bugzilla.mozilla.org/show_bug.cgi?id=811538 
https://bugzilla.mozilla.org/show_bug.cgi?id=811539 

> Does the patch here
> actually fix the issue? Does it have any regressions? Does the code looks
> good?
> 
> If Yes. No. Yes. then let's land it.

Can you create a patch that simply solves what this bug is about and create follow-up bugs for the other issues this patch is trying to solve? 

What Borja is requesting is what has been requested to be done in other bugs: trying to solve in every patch what the bug is about and no other issues.
(In reply to Vivien Nicolas (:vingtetun) from comment #35)
> (In reply to Borja Salguero [:borjasalguero] (limited access to email until
> 10th December) from comment #34)
> > Well, main bugs are bb+ and the majority have a patch being reviewed/tested.
> > It could be great if the patch is only related with the 'lock' you mentioned
> > before. Thanks!
> 
> There is not reasons to block on a non-c2 bugs here. Does the patch here
> actually fix the issue? Does it have any regressions? Does the code looks
> good?
> 
> If Yes. No. Yes. then let's land it.

As I commented in GitHub, No, It's not ready for landing. I would like to have only the code for fixing the bug, if the bug is there. We should be consistent with our review process, small patches for small bugs. One thing is removing a listener or creating a lock, other is refactor the whole functionality of sending a sms.
(In reply to Daniel Coloma:dcoloma from comment #36)
> (In reply to Vivien Nicolas (:vingtetun) from comment #35)
> > (In reply to Borja Salguero [:borjasalguero] (limited access to email until
> > 10th December) from comment #34)
> > > Well, main bugs are bb+ and the majority have a patch being reviewed/tested.
> > > It could be great if the patch is only related with the 'lock' you mentioned
> > > before. Thanks!
> > 
> > There is not reasons to block on a non-c2 bugs here.
> 
> The other issues are C2:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=811538 
> https://bugzilla.mozilla.org/show_bug.cgi?id=811539 
>

If the code that will be removed by those bugs is change here that even better and there is no reason to spend too much time on it.
 
> > Does the patch here
> > actually fix the issue? Does it have any regressions? Does the code looks
> > good?
> > 
> > If Yes. No. Yes. then let's land it.
> 
> Can you create a patch that simply solves what this bug is about and create
> follow-up bugs for the other issues this patch is trying to solve? 
> 
> What Borja is requesting is what has been requested to be done in other
> bugs: trying to solve in every patch what the bug is about and no other
> issues.

Sure then it will be good to set the right flag on the patch so it is obvious that a decision has been taken and then ask for changes. If the bug does not fit with 'does the code looks good' there is no reason to land it as I said previously.

(looking at the patch it there are probably some unneeded changes but there is mostly code dance and the removals of some obvious mistakes about how to use setTimout)
Here is an alternative patch, smaller but not better. It fixes issues exposed in comment 31 without addressing the original issue of sendMessage: its readability.
It is now way harder to tell if `unlock()` is called in all scenarios.
That's the main reason why I moved code in sub-functions in previous pull request. As Vivien just said, I haven't changed much more code. The additional noise in its diff is due to the move of code in functions.
Attachment #688845 - Flags: review?(lissyx+mozillians)
Attachment #688845 - Flags: review?(fernando.campo)
Attachment #688845 - Flags: review?(fbsc)
Attachment #688845 - Flags: review?(lissyx+mozillians)
Attachment #688845 - Flags: review?(fbsc)
Attachment #689153 - Flags: review?(lissyx+mozillians)
Attachment #689153 - Flags: review?(fbsc)
Please squash all commits into one. I would like, before merging (because for me was impossible to reproduce this bug), that [:gerard-majax] will test the patch, that's why I've added him as reviewer. The code for me looks nice.
Attachment #689153 - Flags: review?(fbsc) → review+
Comment on attachment 689153 [details]
Pull request 6853 - smaller patch

Let's not ask too many things to our contributor, he has other things to do in his other life. Let's land that and gerard-majax please re-open if you can reproduce :)
Attachment #689153 - Flags: review?(lissyx+mozillians)
Device back into my hands, code updated, no more issue. Thanks to all of you for fixing this hard-to-find race conditon.
Reviewed and verified on "Unagi" device,
buildID:20130103070201
User  able to click on "Send" button only once and only one SMS sent
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: