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

RESOLVED FIXED in Firefox OS v1.3

Status

Firefox OS
Gaia::System
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: amitav.anand, Assigned: julienw)

Tracking

({regression})

unspecified
1.3 C2/1.4 S2(17jan)
ARM
Linux
regression
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(2 attachments, 1 obsolete attachment)

3.30 MB, audio/mpeg
Details
46 bytes, text/x-github-pull-request
etienne
: review+
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
Created attachment 8355498 [details]
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.
(Reporter)

Comment 1

4 years ago
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
Last Resolved: 4 years ago
Component: Gaia::Bluetooth File Transfer → Gaia::Music
Resolution: --- → DUPLICATE
Duplicate of bug: 838927

Comment 3

4 years ago
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 → ---
(Reporter)

Comment 4

4 years ago
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)

Comment 5

4 years ago
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
(Reporter)

Comment 6

4 years ago
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.
(Reporter)

Comment 7

4 years ago
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

4 years ago
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
(Reporter)

Comment 9

4 years ago
Created attachment 8357640 [details] [review]
Pull Request for bug 956276

second dispatch of the event has been removed. kindly review.
Attachment #8357640 - Flags: review?(dkuo)

Comment 10

4 years ago
(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 11

4 years ago
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.
Attachment #8357640 - Flags: review?(dkuo) → review?(felash)
(Assignee)

Comment 12

4 years ago
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 ;)
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.
Duplicate of this bug: 950678
Probably 1.3+ since a 1.3+ bug has just been marked as duplicate.
(Assignee)

Comment 16

4 years ago
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
(Assignee)

Comment 17

4 years ago
Francisco might provide some insight as well.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 949257
(Assignee)

Comment 19

4 years ago
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.
(Assignee)

Comment 24

4 years ago
Francisco, you surely meant to add "from 1.3", right ?
Oh! Julien, you are totally right, we removed it 'from 1.3' branch :)

Updated

4 years ago
See Also: → bug 959283
(Assignee)

Comment 26

4 years ago
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
(Assignee)

Comment 27

4 years ago
stealing to write unit tests
Assignee: amitav.anand → felash
(Assignee)

Comment 28

4 years ago
Created attachment 8361727 [details] [review]
pull request with unit tests

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] [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.
Attachment #8361727 - Flags: review?(etienne) → review+
(Assignee)

Comment 30

4 years ago
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.
(Assignee)

Comment 31

4 years ago
(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)
(Assignee)

Updated

4 years ago
Flags: needinfo?(amitav.anand)
(Assignee)

Comment 32

4 years ago
master: 73898bf32b3da33a0de96438302feb21635285aa
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
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] [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]:
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+
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1029188
You need to log in before you can comment on or make changes to this bug.