Closed Bug 1079824 Opened 6 years ago Closed 6 years ago
[Messages] Draft saved from activity is duplicated
STR: 1. Open Contacts app and choose any contact with phone number; 3. Tap on "Send message" button; 4. Once Messages app is opened, tap on "X" to close app; 5. Choose save draft; 6. Open Messages from Homescreen and observe newly added drafts. Expected result: only one new draft should appear in the list; Actual result: there are two new identical drafts in the list.
There is a chance that it's broken by bug 994190 (that hopefully will be fixed by patch from bug 1075302), here is a push log I was able to get with pvt builds , but having more precise regression window would be beneficial.  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d66436af432&tochange=7c24470b6b3a
Hi Oleg - due to the time (Man-hours) it takes to do a regression window it is customary that we do not do them on non-blocking issues.
Hmm, okay, thanks for clarifications! Ni? myself to look closer
bug 1075302 seems to be fixed so maybe we should just look at it in next build.
I'm just worried that bug 1075302 description sounds too unrelated. But since nobody complains yet, let's wait :)
Okay, the issue is still reproducible with the latest mozilla-central Gecko, will dig into to find culprit.
So, I can confirm that after patch for bug 994190 duplicated drafts appeared. But something bad also happens on our side, I see that we call Drafts.store 3(!) times on activity close: 1. when user confirms saving a draft; 2. onVisibilityChange -> ThreadUI.saveDraft (there is implicit Drafts.store call inside) call; 3. onVisibilityChange -> explicit Drafts.store call.
triage: identifiable regression
blocking-b2g: 2.2? → 2.2+
Can you please test again now that bug 1079546 has landed?
NI myself because Oleg is in PTO.
Seems fixed by bug 1079546. Filed bug 1085411 for the issue you've seen in comment 7. Thanks Ben !
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1079546
Hi David, this feature apparently broke even though TBPL remained completely green. Can you find resources to get integration tests added for this so that we don't break you again?
Julien, apparently this is causing issues even if TBPL is green, could you please sync with bent ?
Filed bug 1088595
We're observing the same issue again, but only when we have two active Messages app instances at the same time. ni? myself to check if it's somehow caused by patch for bug 1058459.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
QA: please find a regression window. The STR in comment 1 needs to have as first step: 0. launch the Messages app, then press home and as 6th and 7th steps: 6. Forcibly close Messages from task manager 7. Open Messages from Homescreen and observe newly added drafts. Thanks!
Nope, patch for bug 1058459 isn't the culprit. Issue from comment 16 is still reproduced even if the patch is reverted with the master Gecko. So let's wait for regression window.
---------------------------------------------------- Mozilla-Inbound Regression Window (Shallow Flash) ---------------------------------------------------- Last Working: Device: Flame 2.2 Master BuildID: 20140926154139 Gaia: e30d373eb5ed0514bce08a3b3d80d71b05a4dc32 Gecko: 627e848b2bf3 Version: 35.0a1 (2.2 Master) Firmware: V188 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 First Broken: Device: Flame 2.2 Master BuildID: 20140926162338 Gaia: e30d373eb5ed0514bce08a3b3d80d71b05a4dc32 Gecko: 8892214038df Version: 35.0a1 (2.2 Master) Firmware: V188 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 Last Working Gaia First Broken Gecko: Issue DOES reproduce Gaia: e30d373eb5ed0514bce08a3b3d80d71b05a4dc32 Gecko: 8892214038df First Broken Gaia Last Working Gecko: Issue does NOT reproduce Gaia: e30d373eb5ed0514bce08a3b3d80d71b05a4dc32 Gecko: 627e848b2bf3 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=627e848b2bf3&tochange=8892214038df Caused by Bug 994190
QA Whiteboard: [QAnalyst-Triage?]
Caused by Bug 994190 - Ben - can you take a look
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
QA Contact: aalldredge
Moving to IndexedDB component, but Ben, we're here if you need anything.
Component: Gaia::SMS → DOM: IndexedDB
Product: Firefox OS → Core
I'll try to can write an integration test to help the resolution and prevent that it comes back.
as requested on IRC
Attachment #8516100 - Attachment mime type: text/x-log → text/plain
This log shows a transaction that got aborted due to page navigation (an expected behavior) and nothing else... I'm thinking that this is actually just a race in the messages code somewhere. The app isn't waiting for a transaction to complete and internal state is getting messed up? We'll need another log with additional sms app logging turned on I think.
OK, we save actually twice: * once when we click on "save" * once when we get the visibilitychange event when the activity finishes There are IMO 2 possible ways: * properly call cleanFields in "close" * add an option to saveDraft ("clean") that would also call cleanFields. 1st solution is likely easier and less error-prone. Both solutions need: * in cleanFields, set "this.recipients.length = 0" if "this.recipients" exists
Ok, I'll move this back to Messages. I have some logging patches that I can give you if you want to see more detail about what IDB is doing with this use case.
Component: DOM: IndexedDB → Gaia::SMS
Product: Core → Firefox OS
No it's ok, thanks Ben, and sorry, I should have looked at it before.
Steve is working on this
Assignee: nobody → schung
Hi Julien, I choose another way to handle this: simply remove the visibility event listener and we could avoid unnecessary visibility change checking. Another reason is in some place the cleanFiled and recipients reset is separated, I'm not sure if it's safe to do both in cleanFiled. Or maybe we could put recipients.length = 0 and toggleRecipientSuggestions into another function like cleanRecipients. Maybe we could do both, any thought?
Attachment #8518079 - Flags: feedback?(felash)
Comment on attachment 8518079 [details] [review] Link to github Yes, it likely works. It sounds more a hack than if we properly clean up the composer, but in the same time it's likely more efficient. I'd still prefer my first solution, especially because we're not targeting a branch so I don't mind doing a little more work if it's cleaner. Let's ask Oleg's advice too :)
Attachment #8518079 - Flags: feedback?(felash) → feedback?(azasypkin)
Comment on attachment 8518079 [details] [review] Link to github IMO calling "cleanFields" that clears recipients input in addition to message input sounds more correct. Also, not related to the issue, but looks like we can get rid of explicit "Drafts.store()" call from "onVisibilityChange" handler, as it's already called implicitly: "ThreadUI.saveDraft -> Drafts.Add -> Drafts.store"
Attachment #8518079 - Flags: feedback?(azasypkin)
Whiteboard: [sms-sprint-2.1S8] → [sms-sprint-2.1S8][p=1]
Comment on attachment 8518079 [details] [review] Link to github Hi Oleg, you are right about the unnecessary draft store, and in this patch I only cleanFields when leaving the activity, since it got stronger relationship than draft saving.
Attachment #8518079 - Flags: review?(azasypkin)
Comment on attachment 8518079 [details] [review] Link to github Looks fine! Just left two questions at GitHub
Attachment #8518079 - Flags: review?(azasypkin) → review+
Hi Oleg, I apply the cleanFileds for message send, but it would require some changes in mock recipient as well. Do you think the changes fro mock is fine?
(In reply to Steve Chung [:steveck] from comment #33) > Hi Oleg, I apply the cleanFileds for message send, but it would require some > changes in mock recipient as well. Do you think the changes fro mock is fine? Sorry for the late reply, yeah it looks good to me!
Thanks! in master 1e835d372c7f2e8cb8f29c21895802f449a97634 And create a new bug 1100182 for cleanfields refactoring.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
This issue is verified fixed on the Flame 2.2 Shallow Flash (319mb) I was sure to follow the steps mentrioned from Comment 0 as well as the additional steps as seen from Comment 16. Result: Message draft no longer duplicates. Flame 2.2 Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash) Build ID: 20141121040204 Gaia: 25388c6bce932657ebf93adedf31881bfaf88c15 Gecko: 3366c0fcf9c2 Version: 36.0a1 (2.2) Firmware Version: v188-1 User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
You need to log in before you can comment on or make changes to this bug.