1079824 - [Messages] Draft saved from activity is duplicated

Status: VERIFIED FIXED

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

Description

[Oleg Zasypkin [:azasypkin]]

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.

Comment 1

[Oleg Zasypkin [:azasypkin]]

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

Comment 2

[Joshua Mitchell (Inactive)]

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.

Comment 3

[Oleg Zasypkin [:azasypkin]]

Hmm, okay, thanks for clarifications!

Ni? myself to look closer

Comment 4

[Julien Wajsberg [:julienw]]

bug 1075302 seems to be fixed so maybe we should just look at it in next build.

Comment 5

[Oleg Zasypkin [:azasypkin]]

I'm just worried that bug 1075302 description sounds too unrelated. But since nobody complains yet, let's wait :)

Comment 6

[Oleg Zasypkin [:azasypkin]]

Okay, the issue is still reproducible with the latest mozilla-central Gecko, will dig into to find culprit.

Comment 7

[Oleg Zasypkin [: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.

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: identifiable regression

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Can you please test again now that bug 1079546 has landed?

Comment 10

[Julien Wajsberg [:julienw]]

NI myself because Oleg is in PTO.

Comment 11

[Julien Wajsberg [:julienw]]

Seems fixed by bug 1079546.

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

Thanks Ben !

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 13

[David Scravaglieri [:scravag]]

Julien, apparently this is causing issues even if TBPL is green, could you please sync with bent ?

Comment 14

[Julien Wajsberg [:julienw]]

Filed bug 1088595

Comment 15

[Oleg Zasypkin [:azasypkin]]

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.

Comment 16

[Julien Wajsberg [:julienw]]

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!

Comment 17

[Oleg Zasypkin [:azasypkin]]

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.

Comment 18

[Adam Alldredge [:AdamA][Inactive]]

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

[Joshua Mitchell (Inactive)]

Caused by Bug 994190 - Ben - can you take a look

Comment 20

[Julien Wajsberg [:julienw]]

Moving to IndexedDB component, but Ben, we're here if you need anything.

Comment 21

[Julien Wajsberg [:julienw]]

I'll try to can write an integration test to help the resolution and prevent that it comes back.

Comment 22

[Julien Wajsberg [:julienw]]

Attachment: logcat-duplicate-draft-filtered.log

as requested on IRC

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 24

[Julien Wajsberg [:julienw]]

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

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 26

[Julien Wajsberg [:julienw]]

No it's ok, thanks Ben, and sorry, I should have looked at it before.

Comment 27

[Julien Wajsberg [:julienw]]

Steve is working on this

Comment 28

[Steve Chung [:steveck]]

Attachment: 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?

Comment 29

[Julien Wajsberg [:julienw]]

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

Comment 30

[Oleg Zasypkin [: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"

Comment 31

[Steve Chung [:steveck]]

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.

Comment 32

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8518079 [details] [review]
Link to github

Looks fine! Just left two questions at GitHub

Comment 33

[Steve Chung [:steveck]]

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?

Comment 34

[Oleg Zasypkin [: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!

Comment 35

[Steve Chung [:steveck]]

Thanks! in master 1e835d372c7f2e8cb8f29c21895802f449a97634

And create a new bug 1100182 for cleanfields refactoring.

Comment 36

[John Thomas [:Johnt]]

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