Changeset gecko-b2g18: 47823629795e gaia: ff7472af0fe27f4cda0e42f301026b167ebaafe1 Steps to reproduce: 1- Open SMS 2- tap message 3- Send message Expected: Message appears in message thread with throbber, then throbber disappear when message is sent. Actual: 2 Messages appears in message thread, on stays with throbber, even if you close the App or reboot the phone

There is no issue with gecko-beta: bacea371020a gaia: ff7472af0fe27f4cda0e42f301026b167ebaafe1

Here is some more information as I helped David narrowing down this issue. It is a gecko regression as it works with the current gaia master (ff7472af0fe27f4cda0e42f301026b167ebaafe1) and last working smoketest gecko version (bacea371020a). I seems to be due to bug 774621. As it fails when I cherry pick this bug patches on top of the changesets I just described. (note that you have to execute `make-reset-gaia` after updating gecko) Vicamo, I have no idea what is going on there, it may just be matter of updating gaia code due to new platform behavior?? Here is the gaia code that interact with SMS API: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/sms.js#L1247-L1291 Especially the code that handle pending messages, as it seems to do what you just implemented on platform side, isn't?

With the new change in Gecko, current Gaia code should work as usual. In the 'performance' bug, one of the task it's remove 'IndexedDB' for pending messages and rely on the platform only, due to 'error' and 'sending' messages are handle by Gecko now. Otherwise, we could ask Vicamo if there is any other change that should be implemented in Gaia. Vicamo, Do you know what it's happening? Thanks!

This test case runs correctly on gecko trunk. No duplicated message found. Still verifying b2g18 branch.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #6) > This test case runs correctly on gecko trunk. No duplicated message found. > Still verifying b2g18 branch. Passed! Building GUI emulator locally ....

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7) > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #6) > > This test case runs correctly on gecko trunk. No duplicated message found. > > Still verifying b2g18 branch. > > Passed! Building GUI emulator locally .... Can't reproduce with current revisions: Gecko: mozillaorg/gecko-18 (Bug 817730 - Abort connect if rilproxy is not ready.) Gaia: ff7472af0fe27f4cda0e42f301026b167ebaafe1

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10) > Can't reproduce with current revisions: > Gecko: mozillaorg/gecko-18 (Bug 817730 - Abort connect if Sorry, it's https://github.com/mozilla/mozilla-central/tree/b2g18 > rilproxy is not > ready.) > Gaia: ff7472af0fe27f4cda0e42f301026b167ebaafe1

I reproduce Bug 822255 with same gecko and gaia 74f6107ad865102abdecd7d130be891298dec42a (so more recent).

If this is a platform problem, we need to more information about which platform call is returning unexpected results. Borja: Can you detail which function in the SMS API isn't returning the results that the gaia code is expecting.

I cant reproduce it with Today's build & Latest Gaia... Tomorrow I will try with new Build from Manifest and I will come back with more feedback

borja> this is very simple to reproduce. For example, you can try to send a message to a name that you don't have as a contact. I could also reproduce it sending a message to a working number, waiting that the message is sent, wait a few seconds more, and then going back to the message list, and going to the conversation page again. Both are reproducing the "throbber not disappearing" problem. The duplicated message is very easily reproducible without a SIM card. Just send a SMS to whatever name (even a name that is not in your contact list), there is an error (of course), then go back to the conversation list, then go again to the conversation page. current gaia master, current b2g18.

(In reply to Julien Wajsberg [:julienw] from comment #15) > borja> this is very simple to reproduce. I've tried to reproduce this with the gecko revisions David provided within emulator, just works as normal.

I can take a stab at this. (In reply to Julien Wajsberg [:julienw] from comment #15) > borja> this is very simple to reproduce. For example, you can try to send a > message to a name that you don't have as a contact. What exactly do you mean? Precise STRs please! > I could also reproduce it sending a message to a working number, waiting > that the message is sent, wait a few seconds more, and then going back to > the message list, and going to the conversation page again. > > Both are reproducing the "throbber not disappearing" problem. I can't reproduce this problem at all. > The duplicated message is very easily reproducible without a SIM card. Just > send a SMS to whatever name (even a name that is not in your contact list), > there is an error (of course), then go back to the conversation list, then > go again to the conversation page. I can definitely reproduce this.

The first two testcases need a SIM card. 1st testcase's STR: * open SMS app * tap the "+" button to create a new SMS * enter "jouiofguofigu" as destination * enter "pdsidispdiposdi" as text * press the send button => IIRC an error is displayed (Sorry I don't have a phone here) which is correct but there is a throbber that doesn't stop which is not correct. 2nd testcase's STR: see Bug 822255, comment 0 I'm pretty sure

(In reply to Julien Wajsberg [:julienw] from comment #18) > The first two testcases need a SIM card. Yep. > 1st testcase's STR: > * open SMS app > * tap the "+" button to create a new SMS > * enter "jouiofguofigu" as destination > * enter "pdsidispdiposdi" as text > * press the send button > => IIRC an error is displayed (Sorry I don't have a phone here) which is > correct but there is a throbber that doesn't stop which is not correct. Yes, the error is displayed, but the throbber disappears as expected. > 2nd testcase's STR: see Bug 822255, comment 0 I can't reproduce this either. So far I can only see the duplicate message appearing when you try to send a text without a SIM card.

Ok, I will try to produce more precise STR today then.

This issue is reproducible with today build: gecko-b2g18: 6f2f48519b62 gaia: 7c95cecd8bf049c103e626c51de5714b7aefb7dd You need a SIM card and you must send a message to another phone, if you send a message to yourself, it works but it's not the real usecase.

In addition, to see the throbber, from the thread, go back to the message list then reopen the thread.

I've tested with: gaia:"7c95cecd8bf049c103e626c51de5714b7aefb7dd" (same Gaia as you) gecko:"5bad6f1d595945a4b4acc1125447ec0669bf6474" And with this version it's impossible to reproduce. Im gonna try to test with your version (Manifest Build it's building yet...), but it seems that it's something related with Gecko (because same Gaia it's working for me). I've tried the following scenarios: -Send a SMS with SIM Card to a valid phone number. -Send a SMS with SIM Card to a valid phone number in 'Flight mode'. -Send a SMS without SIM Card to a valid phone number. I will come back with more feedback once I have the build with your version.

borja> don't forget you have to go back to the "message list" page, and then come back at the "navigation" page, to see this behaviour. And also, I saw that if you wait a few seconds after the SMS was sent, and THEN you go back to the message list and then come back to the navigation page, it is happening more consistentely. I'll try again with a new fresh build as soon as I arrive at the office.

new fresh build. gecko b2g18, |hg summary| result : parent: 122365:a7a4704fd1ef tip Bug 816452 - Don't fire mozbrowsererror when loading page is stopped. r=jlebar a=blocking-basecamp gaia master, |git log -n 1 --oneline| result: 7c95cec Merge pull request #7057 from asutherland/email-bcc-fix Can reproduce the "throbber stays here" bug with this STR : * have a SIM card insered, with the PIN entered * open sms app * tap "+" * put your own number as the recipient * add any message * tap "send" => the message appears in the thread view, with a throbber => when the message is sent, the throbber disappear => we get a notification that a new message appeared (BTW, it doesn't show up automatically in the opened conversation which is another bug imho) * wait a minute * tap "<" to go back * tap on the number you sent the mail (which should be your number) => the throbber is back beside your sent message I can reproduce this every time. So this is very strange that you don't and we might have something stranger here. I have no helpful log.

[:julienw] It's only happening when you send a SMS to yourself? or it's happening in every case?

So I added some logs in the pile of spaghetti that is the Sms app, and I found that the mozSms API returns messages in the "sending" state, which is wrong. This happens for both sending to myself and sending to another number.

Digging deeper, I'm wondering if the problem doesn't come from the delivery message. That would explain why I have to wait a few seconds/minutes before doing the final actions to make the bug appear. And that would also explain maybe why we don't all see this, maybe some networks don't send the delivery message. Since I'm not familiar at all with this, could someone have a look at [1] and check if this seems fine ? Who could check this ? [1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1450

Hi all. After reviewing deeply I've found some problems of sync. between Gaia & Gecko code, and it's the root of this problem and others related with SMS, so we should move fast on it. After landing https://bugzilla.mozilla.org/show_bug.cgi?id=774621 'sending' messages are handled by Gecko, but also (due to indexedDB for 'sending/error' messages is still in the app), this SMS is shown twice! (one from Gecko, one from Gaia). ¿Could we fix it in Gaia side? I have a patch ready, but I've discovered that Gecko is not handling 'failed' messages properly, so messages sent when 'flight mode' & 'No SIM Card' scenarios are not stored in Gecko! So removing the local indexedDB will produce a regression in the functionality :(. And furthermore I cant remove `partially` this indexedDB due to it's tied to PhoneNumberJS, and as you know this library should be out of Gaia code asap. ¿How could we fix this issue? There are two ways for doing it (AFAIK): - One should be creating a 'followup' bug where [:vicamo] should add a patch for the 'failed' messages, and I would add the patch for Gaia. ¿Problem? Until these patches will be ready and landed SMS App is not going to work properly! - On the other hand we could backout this changes in Gecko, add the 'failed' messages management and Gaia patch, and merge all this code once it will be ready and reviewed. With this approach SMS App would be working while we create the patch. Wdyt?

My view is that Sms is working fine enough currently and you should just move forward in creating the new patches. Could you please upload your wip patch so that the work is not done twice (I'm thinking to :philikon here :) ) ?

I'm in charge of the Gaia part, and I would need the patch in Gecko side for the 'failed' sms for testing it and check that everything works as expected (currently there are scenarios that it's impossible to test :( ). That's why I would need who is gonna be in charge of this in Gecko side for sync. in this work and deliver it asap.

More info yet :(. Change to 'sending' status should dispatch an event, but this event it's never dispatched. So it's not only adding 'failed' in the scenarions missing, I would need to fix this bug as well.

(In reply to David Scravaglieri [:scravag] from comment #21) > This issue is reproducible with today build: Please be specific. There are three separate issues being discussed in this bug. (In the future, I suggest filing individual bugs for these.) (In reply to Julien Wajsberg [:julienw] from comment #27) > So I added some logs in the pile of spaghetti that is the Sms app, and I > found that the mozSms API returns messages in the "sending" state, which is > wrong. This happens for both sending to myself and sending to another number. Can you please upload this logging patch? (In reply to Borja Salguero [:borjasalguero] from comment #29) > After landing https://bugzilla.mozilla.org/show_bug.cgi?id=774621 'sending' > messages are handled by Gecko, but also (due to indexedDB for > 'sending/error' messages is still in the app), this SMS is shown twice! (one > from Gecko, one from Gaia). This would explain the duplicate message when there's no SIM card. > ¿Could we fix it in Gaia side? > I have a patch ready, but I've discovered that Gecko is not handling > 'failed' messages properly, Please be more specific. How is Gecko not handling failed messages "properly"? What is the "proper" behaviour and what is the current behaviour? If there's a Gecko bug, let's file it and refer to it here. > so messages sent when 'flight mode' & 'No SIM > Card' scenarios are not stored in Gecko! So removing the local indexedDB > will produce a regression in the functionality :(. It already isn't working. See bug 822468. > ¿How could we fix this issue? > There are two ways for doing it (AFAIK): > - One should be creating a 'followup' bug where [:vicamo] should add a patch > for the 'failed' messages, and I would add the patch for Gaia. ¿Problem? > Until these patches will be ready and landed SMS App is not going to work > properly! It already isn't working properly. Can we please outline what the Gecko bug is?

(In reply to Philipp von Weitershausen [:philikon] from comment #33) > > so messages sent when 'flight mode' & 'No SIM > > Card' scenarios are not stored in Gecko! So removing the local indexedDB > > will produce a regression in the functionality :(. > > It already isn't working. See bug 822468. Never mind, I can't reproduce this bug with today's build.

> > ¿Could we fix it in Gaia side? > > I have a patch ready, but I've discovered that Gecko is not handling > > 'failed' messages properly, > > Please be more specific. How is Gecko not handling failed messages > "properly"? What is the "proper" behaviour and what is the current > behaviour? If there's a Gecko bug, let's file it and refer to it here. > Currently Gecko is not handling 'failed' messages in following scenarios: - Send a SMS without SIM Card - Send a SMS in flight mode Also is not handling some 'status' changes: - There is no event when 'sending' > > so messages sent when 'flight mode' & 'No SIM > > Card' scenarios are not stored in Gecko! So removing the local indexedDB > > will produce a regression in the functionality :(. > > It already isn't working. See bug 822468. > It's working due to Gaia has it own IndexedDB where store these messages. However, it should be stored by Gecko, due to having this management in Gaia level is tied to PhoneNumberJS, and it's well known that one of the goals it's remove this library in Gaia.

per :philikon request

(In reply to Borja Salguero [:borjasalguero] from comment #35) > Currently Gecko is not handling 'failed' messages in following scenarios: > - Send a SMS without SIM Card > - Send a SMS in flight mode Please define "handling". Are you saying it's not notifying a 'failed' event when trying to send an SMS in these scenarios? > Also is not handling some 'status' changes: > - There is no event when 'sending' That's expected. 'sending' is the initial status when you're sending a message. You *know* it's 'sending' because you just asked the system to send the message. So there's no need to dispatch an event. It's like mozTelephony doesn't dispatch a "dialing" event when you dial a new phone call. Because that's the initial status.

Here's a patch with a bit more debugging. I see all events and database records working as expected. With PhoneNumberJS already integrated in Gecko's SmsDatabaseService, why can't we just rip out the pending SMS database in Gaia?

We are going to remove this indexedDB in Gaia (in fact this is the patch that I've almost ready), but for doing it I need the issues explained before. Furthermore, in Gecko SMS states are not working properly! If after send some sms to some destination you call 'getAllMessages', al messages are marked as 'sending'.... Im testing with production build of today.

borja, could you please attach your work in progress ? We'd like to try and fix this during the following week and it would be nice to not begin from scratch. Thanks ! About the fact that all messages are marked 'sending', I'm quite sure of this too, and it happens in [1], maybe because the envelope is not updated in handleSmsSent above. [1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1468

Scenario easy to reproduce: - Send a SMS to phone number A. - Come back to 'Thead List' - Click again in 'Phone number A' in order to see all sms from/to this phone number. Log: E/GeckoConsole( 458): Content JS LOG at app://sms.gaiamobile.org/js/sms.js:513 in thlui_renderThreads: Threads: Message ID 1 status sending Expected: Message ID 1 should be 'SENT' due to it was sent properly Currently: All messages are marked as 'sending', so SMS App is completely broken.

In the patch I've removed the messages from local indexedDB, so all messages retrieved come directly from Gecko. I've added some logs as well in order to check what it's happening.

Patch in Bug 824465 fixes the "throbber is still there" bug for me. For the "duplicate message" bug I think that getting rid of the IndexedDB database will just work. I'll try this now.

Remove the pending IndexedDB and minor refactoring + lots of logs I know I have still some bugs left: - deleting multiple threads - we don't keep the error mark when going back on a thread But it seems to work better than ever ! I hope to have some more time later today, would be cool to have this bug fixed app for Christmas ;-)

Julien - I haven't had time to go over your patch yet, but the work I did for #824552 fixes the deletion, and loading marks when revisiting a thread. Not sure if it's helpful at all, but here is the pull request: https://github.com/mozilla-b2g/gaia/pull/7186

(In reply to Kevin Grandon from comment #48) > Julien - I haven't had time to go over your patch yet, but the work I did > for #824552 fixes the deletion, and loading marks when revisiting a thread. > Not sure if it's helpful at all, but here is the pull request: > https://github.com/mozilla-b2g/gaia/pull/7186 Thanks Kevin ! The loading marks problem is a gecko bug (patch in bug 824465 that landed on inbound today so it should come to other branches later). Your patch could work (I didn't try it) but it adds up a hack on top of another hack, and I'm not comfortable with this, especially for the Sms app. I think that I'll finish my patch today which removes a lot of now-unnecessary things from the sms app. Would you like to review this patch once it's ready ?

Hi Julien! Probably we are working in parallel on the same thing:S... I was working in https://bugzilla.mozilla.org/show_bug.cgi?id=811539 , that basically it's based on removing IndexedDB for Pending (due to is tied to PhoneNumberJS, and the bug is about removing this phone number code) and removing the 'hacks' in 'Contacts' request. As I've seen that we are working on the same, we should make it step by step and try to not 'overlap' our code. In this case (if you agree) your patch would be removing this 'indexedDB' and 'sending' management, and I will be in charge of removing the other parts of code where 'PhoneNumberJS' is used and the 'optimization' code, that it's ready in one of my branches. Wdyt? Please put me as reviewer of your patch and I will test&review it asap! Thanks ;)

Borja, I'm ok with this, I should have a working patch later today. Thanks !

Remove the pending IndexedDB and minor refactoring in sending, resending, and deletion of messages and threads. This fixes the duplicated message in thread. Fixed some other bugs in the process : - duplicated message when we were resending a failed message - the received SMS was not displayed when I sent it to myself - when I was sending the first SMS to a contact, and when I was hiding the Sms app (using the home button) and then going back to the Sms app, then the thread was empty. This happened only when the phone number was not normalized yet in the hash. - now we try to send the SMS even if we don't have mozSettings - fix notification was appearing when we were already in the good thread, when the num was not normalized yet

I forgot: now we store in gecko a SMS that the user tried to send when airplane mode was set.

changes: reset MessageManager.currentNum when necessary + fixed most gjslint and jshint warnings.

Julien, i was reviewing your code and I have some concerns about 'currentNum' due to we should not work with 'phone number' because we are going to lost this capability once PhoneNumberJS will be out of SMS App (I mean, currentNum could be not internationalized, so probably we are gonna have a lot of issues about it). Otherwise I would like to test a simple approach (Im trying to develop it and I would like to share with you and comment this idea) because I think that it could be more simpler! I will come back soon with this info and the comments with a deep review of your code. Thanks for all your support!

Borja, let's take care of this in Bug 809213 if this turns out to be a problem. I tried to take care to always have a internationalized number in currentNum but I may have failed. If you find a place in my code where this is not the case, I can fix that. I don't want a simple approach, I want a approach that works and fixes all the bugs I found in the process, and removing the IndexedDB that we don't want anymore. I'm not sure you could find a simpler approach doing this. I took great care to not change how the whole application worked, I kept all the functions, so I think this should be ok for you. I'm looking forward to your review so that we can have a working Sms app ! :)

Just to be clear, my process has been : * first, remove the Pending DB everywhere it was used * then, fix any bug that I found that was related to the DB being removed. * tested a lot, with or without a SIM card, with or without airplane mode, sending SMS to myself, to other person, receiving SMS when I was in the app, receiving SMS when I was in the good thread, receiving SMS when I was elsewhere, resending a failed SMS, canceling the resend and then trying to resend again, a failed resend, trying to resend when going back to a thread...

For sure! I have the same goal with SMS App ;). Im gonna try to upload the approach for sharing with you and analyze if it's feasible and know your opinion and I will review your code as well. Thanks!

Julien! Here you have the proposal. https://github.com/mozilla-b2g/gaia/pull/7242 Please take a look and let me know what do you think! Im reviewing your code now, I will come back with feedback. Thanks!

I understand that you want to put the handlers directly on navigator.mozSMS. I agree that this is interesting and will lead to cleaner code. However I'm concerned because this is a very big change and it may take some time to work correctly (your code doesn't work at all for now)... and we must ship in 2 weeks. I'd suggest that we keep this for post-v1 and we merge my code that I tested a lot for v1. Let's file a new bug for this and let's move forward for v1 ? And then let's fix other bugs ;) What do you think ?

Just FYI I spent nearly 3 full days on this, with half of it only for testing.

(Reviewing...!)

Julien, could you create the patch as a PR in order to add the comments easily? Thanks!

Here you are : https://github.com/mozilla-b2g/gaia/pull/7244 However, you know that you can add comments in the bugzilla "splinter review" view by double-clicking on one line ?

https://github.com/mozilla-b2g/gaia/pull/7244#issuecomment-11743938 Wdyt?

new patch implementing Borja's proposition. The code is a lot cleaner now. There is a lot of things that work differently now, both in terms of code or of behaviour. Also see PR at https://github.com/mozilla-b2g/gaia/pull/7244

Awesome! This code looks so nice! Tomorrow I will review it in more detail and I will test it in my Unagi. Otherwise thanks a lot for your effort! SMS App is gonna work better than ever with these changes ;). If I find something to change I will add a comment and I will try to make you a PR to your branch (only if it's necessary) in order to save time and merge it asap. Merçi! Gracias!

I forgot to remove the last bit of now-useless "pendingAction" :) already pushed to the PR

Borja, take care because I rewrote the commit so if you checked out yesterday, you should do it again :-)

Hehehe, dont worry! Im checking the code again and testing it in my Unagi, I've added to the PR only a small comment. The rest of the code looks nice, Im going to use it during today as my personal one trying to generate all scenarios and test it again and I will come back with the final review asap. By the way I think that this code will be merge on time! :D

I've fixed the error you reported, thanks !

I've seen one or two unrelated bugs, I'll file new bugs as soon as this one is landed.

R+ from my side! I've tested this code a lot and it's working as expected. Please squash your changes and that's all!. Thanks a lot for your support and your effort during these days! This code will be merged on time ;). Last review of the year, so ... je te souhaite une bonne année 2013! Feliz año!

Verified in 2013-1-1 unagi pvt build fixed.