1042357 - Code to removeEventListener for radiostatechange in radio.js not working

Status: RESOLVED FIXED

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

Description

[Anshul]

[Blocking Requested - why for this release]:

The code http://lxr.mozilla.org/gaia/source/apps/system/js/radio.js#123 has a bug due to which the radio.js keeps on calling the setRadioEnabled API everytime it received a radiostatechagned event.

This bug is causing a lot of our test scripts to fail on 2.0.

Comment 1

[Anshul]

The issue is that both the variable name as well as the function name at http://lxr.mozilla.org/gaia/source/apps/system/js/radio.js#123 is same causing  conn.removeEventListener to try to remove the incorrect handler.

The fix is pretty simple and I will upload a patch shortly to address the issue.

Comment 2

[Anshul]

Attachment: Github PR

Comment 3

[Anshul]

Attachment: Github PR

Comment 4

[Gregor Wagner [:gwagner]]

ej, do we have tests for this scenario? If no, can we add one?

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Gregor,

currently we have no related test for this case and yes it's possible to add one more test for this bug.

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8460988 [details]
Github PR

Thanks for catching this, Anshul! The patch looks good to me, could you also help add a test for this case?

Comment 7

[Anshul]

Arthur, I am not familiar with the gaia test framework. Wondering if it is okay to file a new bug to add a test case so at least this fix can be landed?

If it is, then please provide a r+ to the patch so this can be landed.

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Anshul from comment #7)
> Arthur, I am not familiar with the gaia test framework. Wondering if it is
> okay to file a new bug to add a test case so at least this fix can be landed?
> 
> If it is, then please provide a r+ to the patch so this can be landed.

Yes lets fix this blocker and file a followup for the test. Arthur can you take care of it?

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8460988 [details]
Github PR

r=me, thanks!

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

master: 8dd303f770121ad2eef084335b580a273a18ecde

The follow up bug for adding unit tests is bug 1044727.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/6d97765a49b510aa7c9dd31e8c7541eea6af925d

Comment 12

[Brogan Zumwalt [Inactive]]

No STR is present to create test case to address bug.

Comment 13

[Anshul]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> v2.0:
> https://github.com/mozilla-b2g/gaia/commit/
> 6d97765a49b510aa7c9dd31e8c7541eea6af925d
Ryan, seems like this change is not present in 2.1 and moz central even though this change was uplifted on 2.0 before 2.1 was even branched. Could you please uplift this change?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

If the change is already present on v2.1 given when it landed, what are you expecting me to uplift? Sounds to me like a new bug needs to be filed for whatever issue you're seeing.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

And maybe that "add test coverage" bug mentioned in comment 10 should actually be assigned to someone so this doesn't apparently regress again.

Comment 16

[Anshul]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> If the change is already present on v2.1 given when it landed, what are you
> expecting me to uplift? Sounds to me like a new bug needs to be filed for
> whatever issue you're seeing.
Ryan, sorry I thought the change is not present in 2.1 as it doesn't show in the git history but the fix is present. Please ignore.

Comment 17

[Anshul]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> If the change is already present on v2.1 given when it landed, what are you
> expecting me to uplift? Sounds to me like a new bug needs to be filed for
> whatever issue you're seeing.
Ryan, I think I know what's going on here. This bug was actually not uplifted to 2.1. However as part of bug 1038496 the author of the bug 1038496 squashed the fix for this bug as part of his bug and uplifted. That is why it seemed that the fix for this bug is present in 2.1. Now that the bug 1038496 has been permanently backed out from 2.1, the fix for this bug is also removed. 

Please help in uplifting this bug to 2.1.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

As has been the case for 6+ weeks now, *all* uplifts need approval. I can't help you with that.

Comment 19

[Anshul]

[Blocking Requested - why for this release]: Due to this bug radio.js keeps on calling the setRadioEnabled API everytime it received a radiostatechagned event.

This bug is causing most of our unit test scripts to fail on 2.1.

Comment 20

[Gregor Wagner [:gwagner]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> As has been the case for 6+ weeks now, *all* uplifts need approval. I can't
> help you with that.

Comment 21

[Andrew Overholt [:overholt]]

Blocking since it seems we just didn't get this onto master/2.1.

Comment 22

[Anshul]

Please uplift this bug to 2.1 now that is it approved to be uplifted. I am correcting the incorrect 2.1 status of fixed.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/857ac4fde427704cff87e174cab2020cba98d5ff

Comment 24

[Elie]

Please provide the reproduce step for tester to verify this bug.Thanks!