Closed Bug 956276 Opened 11 years ago Closed 11 years ago

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

Categories

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

ARM
Linux
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: amitav.anand, Assigned: julienw)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

(deleted), audio/mpeg
Details
(deleted), text/x-github-pull-request
etienne
: review+
Details
Attached audio Test File (deleted) —
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.
Seems like this issue is similar to Bug 838927.
OS: All → Linux
Hardware: All → ARM
According to the symptom, this is a duplicated issue with 838927. That's track it via bug 838927.
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Component: Gaia::Bluetooth File Transfer → Gaia::Music
Resolution: --- → DUPLICATE
amitav.anand@accenture.com, 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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(amitav.anand)
Resolution: DUPLICATE → ---
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.
Flags: needinfo?(amitav.anand)
Thanks amitav.anand@accenture.com, 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.
Component: Gaia::Music → Gaia::Bluetooth File Transfer
Flags: needinfo?(iliu)
Summary: [ZTE]-Two or more instance of Music App is running when an MP3 file received via BT → [ZTE]-Two or more instances of open activity will be launched after received files via bluetooth
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.
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.
Thanks amitav.anand@accenture.com, 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.
blocking-b2g: --- → 1.3?
Keywords: regression
Attached file Pull Request for bug 956276 (obsolete) (deleted) —
second dispatch of the event has been removed. kindly review.
Attachment #8357640 - Flags: review?(dkuo)
(In reply to Dominic Kuo [:dkuo] from comment #8) > Thanks amitav.anand@accenture.com, 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.
Component: Gaia::Bluetooth File Transfer → Gaia::System
Depends on: 935094
Flags: needinfo?(iliu)
Comment on attachment 8357640 [details] 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.
Attachment #8357640 - Flags: review?(dkuo) → review?(felash)
Comment on attachment 8357640 [details] 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 ;)
Attachment #8357640 - Flags: review?(felash) → review+
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.
Probably 1.3+ since a 1.3+ bug has just been marked as duplicate.
mmm AFAIK Download Manager was backed out from 1.3 so I doubt this regression is on 1.3. QAWanted to find out.
Keywords: qawanted
Francisco might provide some insight as well.
Re: comment 16: maybe this part was not backed out. Etienne is having a look :)
Looks like it was properly backed out. So hopefully the qawanted will yieled good news :) (that the bug only happen on master)
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
Keywords: qawanted
QA Contact: pbylenga
Moving to 1.4? per comment 21
blocking-b2g: 1.3? → 1.4?
Hi folks, we did rollback of all the commits for DM. We didn't experience this bug during our development. Cheers, F.
Francisco, you surely meant to add "from 1.3", right ?
Oh! Julien, you are totally right, we removed it 'from 1.3' branch :)
Amitav, do you think you can do the unit tests for your patch? See comment 12 for more information.
Assignee: nobody → amitav.anand
Flags: needinfo?(amitav.anand)
Blocks: 938540
stealing to write unit tests
Assignee: amitav.anand → felash
Attached file pull request with unit tests (deleted) —
I also took the opportunity to fix jshint warnings.
Attachment #8357640 - Attachment is obsolete: true
Attachment #8361727 - Flags: review?(etienne)
Comment on attachment 8361727 [details] 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.
Attachment #8361727 - Flags: review?(etienne) → review+
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.
(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)
Flags: needinfo?(amitav.anand)
master: 73898bf32b3da33a0de96438302feb21635285aa
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
blocking-b2g: 1.4? → 1.4+
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.
Blocks: 890440
blocking-b2g: 1.4+ → 1.3?
Comment on attachment 8361727 [details] 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]:
Attachment #8361727 - Flags: approval-gaia-v1.3?(fabrice)
blocking-b2g: 1.3? → 1.3+
Attachment #8361727 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: