[Messages] Draft saved from activity is duplicated

VERIFIED FIXED in 2.1 S9 (21Nov)

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: azasypkin, Assigned: steveck)

Tracking

({regression})

unspecified
2.1 S9 (21Nov)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified)

Details

(Whiteboard: [sms-sprint-2.1S8][p=1])

Attachments

(2 attachments)

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 [1], but having more precise regression window would be beneficial.

[1] 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
Flags: needinfo?(azasypkin)
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.
Flags: needinfo?(azasypkin)
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.
blocking-b2g: --- → 2.2?
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.
Flags: needinfo?(felash)
Seems fixed by bug 1079546.

Filed bug 1085411 for the issue you've seen in comment 7.

Thanks Ben !
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(felash)
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?
Flags: needinfo?(dscravaglieri)
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)
No longer blocks: 1088595
Filed bug 1088595
Flags: needinfo?(felash)
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)
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!
QA Contact: aalldredge
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)
----------------------------------------------------
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?]
Flags: needinfo?(jmitchell)
Caused by Bug 994190 - Ben - can you take a look
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
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.
Flags: needinfo?(felash)
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.
Flags: needinfo?(bent.mozilla)
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
No it's ok, thanks Ben, and sorry, I should have looked at it before.
Steve is working on this
Assignee: nobody → schung
Assignee

Comment 28

5 years ago
Posted file Link to github
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]
Whiteboard: [sms-sprint-2.1S8] → [sms-sprint-2.1S8][p=1]
Target Milestone: --- → 2.1 S9 (21Nov)
Assignee

Comment 31

5 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)
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

5 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)
(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

Updated

5 years ago
Blocks: 1100182
Assignee

Updated

5 years ago
No longer blocks: 1100182
Assignee

Comment 35

5 years ago
Thanks! in master 1e835d372c7f2e8cb8f29c21895802f449a97634

And create a new bug 1100182 for cleanfields refactoring.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
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.