956276 - [ZTE]-Two or more instances of open activity will be launched after received files via bluetooth

Status: RESOLVED FIXED

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

Description

[amitav.anand]

Attachment: Test File

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131206152142

Steps to reproduce:

Test Environment:
Device: ZTE
Hardware revision : roamer2
OS version : 1.4.0.0-prerelease
BuildID: 20131227113200
Git Commit Info:2013-12-18 21:39:18 8bbb5170

Steps to reproduce:
1.Pair any phone with the DUT (ZTE).
2.Transfer an MP3 file via blue-tooth.
3.After the file transfer is complete, tap on the notification.
4.The Music app launches and plays the music file just received.





Actual results:

Actual Results:
Carefully listen to the song. More than 1 instance of the mp3 files can be heard overlapped with one another.Pause the song from UI .After Pause the playback of the music file can still be heard.


Expected results:

Music app should play only 1 instance of the file received via blue-tooth.

Comment 1

[amitav.anand]

Seems like this issue is similar to Bug 838927.

Comment 2

[[e-mail], [e-mail]]

According to the symptom, this is a duplicated issue with 838927. That's track it via bug 838927.

Comment 3

[Dominic Kuo [:dkuo]]

[e-mail], does the reproduce steps launched two instances of music app at the same time? or one is already launched in the background and another is launched by the notification? and I think this is not the same as bug 838927 because the steps are different so I will first reopen it, thanks.

Comment 4

[amitav.anand]

Yes,It launches 2 instance at the same time. On pausing the song from UI the song still plays and on resuming the song both the instance plays together.
Before trying this test I had made sure that the music player is not running.

Comment 5

[Dominic Kuo [:dkuo]]

Thanks [e-mail], I followed the reproduce steps and also tested on transferring images, then found the notification launches more than one instance of the gallery open activity as well(I believe it's the same for videos), so this should be a general issue of launching apps from the notification, I am changing the component back to Gaia::Bluetooth File Transfer and modifying the title to fit the description.

Ian, as I described above, I found the open activity will be triggered more than once(my test result is 4 times) when users tap on the complete notification from bluetooth, please re-visit this issue and investigate it, thanks.

Comment 6

[amitav.anand]

I had traced out the same.
Till onTransferComplete: function bt_onTransferComplete in bluetooth_transfer.js the control flow seems to be correct, as it is called only once. On successfully receiving the file below notification is called:-

NotificationHelper.send(_('transferFinished-receivedSuccessful-title'),fileName,icon,
this.openReceivedFile.bind(this, transferInfo));

This calls gaia/shared/js/notification_helper.js, here notification.onclick = (function() is getting called twice.

Comment 7

[amitav.anand]

in gaia/apps/system/js/notification.js the "tap" method has window.dispatchEvent(event) has been called twice even though there is no change in event paramenter, so this caused music app to play the song twice.

event.initCustomEvent('mozContentEvent', true, true, {
      type: 'desktop-notification-click',
      id: notificationId
    });
    window.dispatchEvent(event);

    window.dispatchEvent(new CustomEvent('notification-clicked', {
      detail: {
        id: notificationId
      }
    }));
    //window.dispatchEvent(event);
Commenting out the window.dispatchEvent(event) solves this issue.
kindly comment.

Comment 8

[Dominic Kuo [:dkuo]]

Thanks [e-mail], that explained why and make sense to cause this issue, since this is a regression so I am nominating this 1.3? and finding which bug has caused this issue.

Comment 9

[amitav.anand]

Attachment: Pull Request for bug 956276

second dispatch of the event has been removed. kindly review.

Comment 10

[Dominic Kuo [:dkuo]]

(In reply to Dominic Kuo [:dkuo] from comment #8)
> Thanks [e-mail], that explained why and make sense to
> cause this issue, since this is a regression so I am nominating this 1.3?
> and finding which bug has caused this issue.

This bug is caused by the patch of bug 935094, setting dependency and changing the component to Gaia::System.

Comment 11

[Dominic Kuo [:dkuo]]

Comment on attachment 8357640 [details] [review]
Pull Request for bug 956276

amitav-anand, thanks for contributing this patch, although it's an one line patch but since I am not the peer of system app, I am redirecting the review to Julien.

Julien, would you please review the patch, thanks.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8357640 [details] [review]
Pull Request for bug 956276

I'm not a peer either but I can review this :)

r=me, but I'd like that you add a simple unit test to system/test/unit/notifications_test.js before merging. Something like:

* add a suite
* in setup:
  + call addNotification, save the returned node in a var
  + add event listeners on window for mozContentEvent and notification-clicked, use "sinon.stub()" functions to do this, that you save in variables.
* in test:
  + dispatch a "tap" event to the notification node
  + test that the stubs were called only once (sinon.assert.calledOnce(stub)).
* in teardown:
  + remove the event listeners

You can ask another feedback from me when you do the test, if you don't feel confident enough ;)

Comment 13

[Etienne Segonzac (:etienne)]

FWIW if you guys want to land the patch as is you have my blessing :)

It's fixing what was probably a copy/paste mistake in bug 935094, not adding/modifying logic.

I'm all for a follow up bug to cover notification event dispatch with unit tests, but we should fix the regression from bug 935094 ASAP.

Comment 15

[Etienne Segonzac (:etienne)]

Probably 1.3+ since a 1.3+ bug has just been marked as duplicate.

Comment 16

[Julien Wajsberg [:julienw]]

mmm AFAIK Download Manager was backed out from 1.3 so I doubt this regression is on 1.3.

QAWanted to find out.

Comment 17

[Julien Wajsberg [:julienw]]

Francisco might provide some insight as well.

Comment 19

[Julien Wajsberg [:julienw]]

Re: comment 16: maybe this part was not backed out. Etienne is having a look :)

Comment 20

[Etienne Segonzac (:etienne)]

Looks like it was properly backed out.
So hopefully the qawanted will yieled good news :)
(that the bug only happen on master)

Comment 21

[Peter Bylenga [:PBylenga]]

I was unable to reproduce this issue using the current 1.3 build.

Environmental Variables
Device: Buri v1.3 Mozilla RIL
Build ID: 20140109004002
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/2c8f8683bd0d
Gaia: 22bc6be5b76cdc6d4e9667ff070979041a20ce2f
Platform Version: 28.0a2 
Firmware Version: 20131115

Comment 22

[Jason Smith [:jsmith]]

Moving to 1.4? per comment 21

Comment 23

[Francisco Jordano [:arcturus] [:francisco]]

Hi folks, we did rollback of all the commits for DM. We didn't experience this bug during our development.

Cheers,
F.

Comment 24

[Julien Wajsberg [:julienw]]

Francisco, you surely meant to add "from 1.3", right ?

Comment 25

[Francisco Jordano [:arcturus] [:francisco]]

Oh! Julien, you are totally right, we removed it 'from 1.3' branch :)

Comment 26

[Julien Wajsberg [:julienw]]

Amitav, do you think you can do the unit tests for your patch? See comment 12 for more information.

Comment 27

[Julien Wajsberg [:julienw]]

stealing to write unit tests

Comment 28

[Julien Wajsberg [:julienw]]

Attachment: pull request with unit tests

I also took the opportunity to fix jshint warnings.

Comment 29

[Etienne Segonzac (:etienne)]

Comment on attachment 8361727 [details] [review]
pull request with unit tests

r=me

With all the boilerplate work that you did I think it's worth it to add 2 tests checking the content of the events (that the right notificationId is passed). But it's up to you, we can land without it.

Comment 30

[Julien Wajsberg [:julienw]]

Good idea, I added it, and I changed also some other tests that were using an integer id for a notification whereas in the real life it's strings.

Comment 31

[Julien Wajsberg [:julienw]]

(this is important because the notification id is used in a dataset and as a result is converted to a string, so if it was previously an integer, a comparison would fail)

Comment 32

[Julien Wajsberg [:julienw]]

master: 73898bf32b3da33a0de96438302feb21635285aa

Comment 33

[Michael Henretty [:mikehenrty][:mhenretty]]

This needs to be uplifted to 1.3, since we are in the process of uplifting bug 890440 to 1.3, which will cause this same problem there.

Comment 34

[Gregor Wagner [:gwagner]]

Comment on attachment 8361727 [details] [review]
pull request with unit tests

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

We have to uplift the bug that caused this regression.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):890440
[User impact] if declined: Notifications show up twice.
[Testing completed]: 
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 35

[Gregor Wagner [:gwagner]]

uplifted to 1.3: https://github.com/mozilla-b2g/gaia/commit/932789749406a79c5630c27c724f1856bd3c3f10