1011654 - [B2G][SMS]Opening the messages app after sending a message from the Call log causes the sent notification sound to occur a second time

Status: RESOLVED FIXED

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

Description

[Andrew Stole [:astole]]

Attachment: logcat

Opening the messages app after sending a message from the call log triggers the sent notification sound again if a message has been previously sent to the same recipient.  This also occurs if the second message is sent from contact details. On some occasions, the sound occured twice immediately after sending the message from the call log.

Repro Steps:
1) Update a Open_C to BuildID: 20140516040203
2) Have a number in the Call log or a Contact
3) Open messages app and send a message to the number from Step 2
4) Press the home button
5) Open Call log or Contacts and send a message to the number from Step 3
6) Press the home button
7) Open the messages app

Actual:
The sent message notification sound occurs when opening the messages app

Expected:
The sound notification does not occur when opening the messages app

2.0 Environmental Variables:
Device: Open_C 2.0 MOZ
BuildID: 20140516040203
Gaia: 7973e06dc278f67b4109ac3c33020ed086f0d042
Gecko: 58c5a3427997
Version: 32.0a1
Firmware Version: P821A10v1.0.0B06_LOG_DL

Repro frequency: 2/2, 100%
See attached: video, logcat

Comment 1

[Andrew Stole [:astole]]

This issue does not occur on the latest 1.4

1.4 Environmental Variables:
Device: Open_C 1.4
BuildID: 20140516000201
Gaia: 32fca83da31b9a0f9a5a88f96c913a25accdc14b
Gecko: a1e455367fa6
Version: 30.0
Firmware Version: P821A10V1.0.0B06_LOG_DL

Adding qawanted to check 2.0 on Buri.

Comment 2

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue does occur on Buri 2.0

2.0 Environmental Variables:
Device: Buri 2.0 MOZ
BuildID: 20140516052831
Gaia: 799b0f8bb71bc1b944f90c117ab5d6be4837ba1f
Gecko: b5b35ec88db8
Version: 32.0a1
Firmware Version: v1.2-device.cfg

An additional notification sound can be heard.

Comment 3

[Julien Wajsberg [:julienw]]

Hey,

just want to make sure you hear the sound notification in step 3 and 5 too.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Hey,
> 
> just want to make sure you hear the sound notification in step 3 and 5 too.

The video does show that the sound is heard.

Comment 5

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

I was able to reproduce this issue on the 1.4 build in comment 1 on an OpenC with the actual results listed in comment 0 where the sound immediately plays twice.  Builds are not available far enough back to get a more precise window.

Mozilla Nightly Central Regression Window

Last Working environment variables
Device: Buri 1.3 MOZ
BuildID: 20131101040203
Gaia: ccdf357ea150fc7d8b8a4b74c7adf31e7a57e465
Gecko: abe6790a5dd8
Version: 28.0a1
Firmware Version: v1.2-device.cfg

First broken environment variables
Device: Buri 1.3 MOZ
BuildID: 20131102040200
Gaia: 50f0c914fa5cb89694c0cdd41b6e10b714787070
Gecko: 396e59370945
Version: 28.0a1
Firmware Version: v1.2-device.cfg


Last working gecko / First broken gaia - occurs - Indicates a Gaia issue
Gaia: 50f0c914fa5cb89694c0cdd41b6e10b714787070
Gecko: abe6790a5dd8


First broken gecko / Last working gaia - does not occur
Gaia: ccdf357ea150fc7d8b8a4b74c7adf31e7a57e465
Gecko: 396e59370945

Gaia Pushlog: https://github.com/mozilla-b2g/gaia/compare/ccdf357ea150fc7d8b8a4b74c7adf31e7a57e465...50f0c914fa5cb89694c0cdd41b6e10b714787070

Comment 6

[Jason Smith [:jsmith]]

There's three SMS bugs in the range - bug 815093, bug 927784, and unknown bug # (https://github.com/mozilla-b2g/gaia/commit/d82ecd50783cc79121b25f24f899b9df2f702c7c).

Julien - Which one looks like the cause of the regression here?

Comment 7

[Julien Wajsberg [:julienw]]

bug 815093 for sure: yet another inline activity change regression.

we need to move the part that plays the sound from ThreadUI.onMessageSent to the onsuccess send/sendMMS callback.

Comment 8

[Julien Wajsberg [:julienw]]

Hey Oleg, any chance you can have a look at this?

Comment 9

[Oleg Zasypkin [:azasypkin]]

Sure, will look into it.

Comment 10

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (v1.4)

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8426456 [details] [review]
GitHub pull request URL (v1.4)

handing over the review to Steve as I'm fairly overloaded.

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8426456 [details] [review]
GitHub pull request URL (v1.4)

Some nits on github but overall is good, so r=me :)
BTW, will you also submit another patch for master later?

Comment 13

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Please review this master patch too as I've rebased it on master and it differs from v1.4 version now.

Comment 14

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #13)
> Created attachment 8429080 [details] [review]
> GitHub pull request URL
> 
> Please review this master patch too as I've rebased it on master and it
> differs from v1.4 version now.

Hey Oleg, since the patch will change the parameter of resendMessage, would you mind adding a test for it? Thanks!

Comment 15

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #14)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #13)
> > Created attachment 8429080 [details] [review]
> > GitHub pull request URL
> > 
> > Please review this master patch too as I've rebased it on master and it
> > differs from v1.4 version now.
> 
> Hey Oleg, since the patch will change the parameter of resendMessage, would
> you mind adding a test for it? Thanks!

Sure! I just wanted to confirm first that you're OK with the method parameter changes, I'll add some tests for resendMessage.

Comment 16

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #14)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #13)
> > Created attachment 8429080 [details] [review]
> > GitHub pull request URL
> > 
> > Please review this master patch too as I've rebased it on master and it
> > differs from v1.4 version now.
> 
> Hey Oleg, since the patch will change the parameter of resendMessage, would
> you mind adding a test for it? Thanks!

Added more tests! Please, take a look :)

Thanks!

Comment 17

[Steve Chung [:steveck]]

Only one comment about using the mock message ;)

Comment 18

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #17)
> Only one comment about using the mock message ;)

Thanks for review! Added mock message as you've suggested and waiting for green Travis.

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8429080 [details] [review]
GitHub pull request URL

Thanks for the fixing and r=me. I trigger the travis again since the test failed in other module. It should be fine to land the patch if the test still failed in unrelated test.

Comment 20

[Steve Chung [:steveck]]

in master: 93402e84de5a9ae3824aead5fb03ce9be984f09e

Comment 21

[Steve Chung [:steveck]]

in v1.4: 7bc1c15c67661a0b8e35f18f15a9d03d1d2cfcd5