Closed
Bug 1079824
Opened 11 years ago
Closed 10 years ago
[Messages] Draft saved from activity is duplicated
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified)
Tracking | Status | |
---|---|---|
b2g-v2.1 | --- | unaffected |
b2g-v2.2 | --- | verified |
People
(Reporter: azasypkin, Assigned: steveck)
References
Details
(Keywords: regression, Whiteboard: [sms-sprint-2.1S8][p=1])
Attachments
(2 files)
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.
Reporter | ||
Comment 1•11 years ago
|
||
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 [1], but having more precise regression window would be beneficial.
[1] http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d66436af432&tochange=7c24470b6b3a
Keywords: regressionwindow-wanted
Comment 2•11 years ago
|
||
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.
Keywords: regressionwindow-wanted
Reporter | ||
Comment 3•11 years ago
|
||
Hmm, okay, thanks for clarifications!
Ni? myself to look closer
Flags: needinfo?(azasypkin)
Comment 4•11 years ago
|
||
bug 1075302 seems to be fixed so maybe we should just look at it in next build.
Reporter | ||
Comment 5•11 years ago
|
||
I'm just worried that bug 1075302 description sounds too unrelated. But since nobody complains yet, let's wait :)
Reporter | ||
Comment 6•11 years ago
|
||
Okay, the issue is still reproducible with the latest mozilla-central Gecko, will dig into to find culprit.
Flags: needinfo?(azasypkin)
Reporter | ||
Comment 7•11 years ago
|
||
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.
Blocks: IndexedDB-on-PBackground
Updated•11 years ago
|
blocking-b2g: --- → 2.2?
Can you please test again now that bug 1079546 has landed?
Comment 11•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(felash)
Resolution: --- → DUPLICATE
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?
Flags: needinfo?(dscravaglieri)
Comment 13•11 years ago
|
||
Julien, apparently this is causing issues even if TBPL is green, could you please sync with bent ?
Flags: needinfo?(felash)
Flags: needinfo?(dscravaglieri)
Flags: needinfo?(bent.mozilla)
Reporter | ||
Comment 15•10 years ago
|
||
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.
Flags: needinfo?(azasypkin)
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment 16•10 years ago
|
||
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!
Keywords: regressionwindow-wanted
Updated•10 years ago
|
No longer blocks: IndexedDB-on-PBackground
Updated•10 years ago
|
QA Contact: aalldredge
Reporter | ||
Comment 17•10 years ago
|
||
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.
Flags: needinfo?(azasypkin)
Comment 18•10 years ago
|
||
----------------------------------------------------
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
Comment 19•10 years ago
|
||
Caused by Bug 994190 - Ben - can you take a look
Blocks: IndexedDB-on-PBackground
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
QA Contact: aalldredge
Comment 20•10 years ago
|
||
Moving to IndexedDB component, but Ben, we're here if you need anything.
Component: Gaia::SMS → DOM: IndexedDB
Product: Firefox OS → Core
Comment 21•10 years ago
|
||
I'll try to can write an integration test to help the resolution and prevent that it comes back.
Updated•10 years ago
|
Flags: needinfo?(felash)
Comment 22•10 years ago
|
||
as requested on IRC
Updated•10 years ago
|
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.
Flags: needinfo?(bent.mozilla)
Comment 24•10 years ago
|
||
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
Flags: needinfo?(felash)
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
Comment 26•10 years ago
|
||
No it's ok, thanks Ben, and sorry, I should have looked at it before.
Assignee | ||
Comment 28•10 years ago
|
||
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 29•10 years ago
|
||
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)
Updated•10 years ago
|
No longer blocks: IndexedDB-on-PBackground
Reporter | ||
Comment 30•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [sms-sprint-2.1S8]
Updated•10 years ago
|
Blocks: sms-sprint-2.1S9
Whiteboard: [sms-sprint-2.1S8] → [sms-sprint-2.1S8][p=1]
Updated•10 years ago
|
Target Milestone: --- → 2.1 S9 (21Nov)
Assignee | ||
Comment 31•10 years ago
|
||
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)
Reporter | ||
Comment 32•10 years ago
|
||
Comment on attachment 8518079 [details] [review]
Link to github
Looks fine! Just left two questions at GitHub
Attachment #8518079 -
Flags: review?(azasypkin) → review+
Assignee | ||
Comment 33•10 years ago
|
||
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?
Flags: needinfo?(azasypkin)
Reporter | ||
Comment 34•10 years ago
|
||
(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!
Flags: needinfo?(azasypkin)
Assignee | ||
Comment 35•10 years ago
|
||
Thanks! in master 1e835d372c7f2e8cb8f29c21895802f449a97634
And create a new bug 1100182 for cleanfields refactoring.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Comment 36•10 years ago
|
||
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•