Closed Bug 1042357 Opened 6 years ago Closed 6 years ago

Code to removeEventListener for radiostatechange in radio.js not working

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S1 (1aug)
blocking-b2g 2.1+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: anshulj, Assigned: anshulj)

References

Details

Attachments

(1 file, 1 obsolete file)

197 bytes, text/html
arthurcc
: review+
Details
[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.
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.
Assignee: nobody → anshulj
Attached file Github PR (obsolete) —
Attachment #8460982 - Attachment description: 22082[1] → Github PR
Attachment #8460982 - Flags: review?(arthur.chen)
Attached file Github PR
Attachment #8460982 - Attachment is obsolete: true
Attachment #8460982 - Flags: review?(arthur.chen)
Attachment #8460988 - Flags: review?(arthur.chen)
Attachment #8460988 - Attachment description: pr.html → Github PR
ej, do we have tests for this scenario? If no, can we add one?
Flags: needinfo?(ejchen)
blocking-b2g: 2.0? → 2.0+
Hi Gregor,

currently we have no related test for this case and yes it's possible to add one more test for this bug.
Flags: needinfo?(ejchen)
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?
Attachment #8460988 - Flags: review?(arthur.chen)
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.
(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?
Flags: needinfo?(arthur.chen)
Blocks: CAF-v2.0-CC-metabug
No longer blocks: CAF-v2.0-FC-metabug
Attachment #8460988 - Flags: review?(arthur.chen)
Comment on attachment 8460988 [details]
Github PR

r=me, thanks!
Attachment #8460988 - Flags: review?(arthur.chen) → review+
master: 8dd303f770121ad2eef084335b580a273a18ecde

The follow up bug for adding unit tests is bug 1044727.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(arthur.chen)
Resolution: --- → FIXED
Flags: in-moztrap?(bzumwalt)
No STR is present to create test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(bzumwalt)
Flags: in-moztrap-
(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?
Flags: needinfo?(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.
Flags: needinfo?(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.
(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.
(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.
Flags: needinfo?(ryanvm)
As has been the case for 6+ weeks now, *all* uplifts need approval. I can't help you with that.
[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.
blocking-b2g: 2.0+ → 2.1?
(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.
Flags: needinfo?(fabrice)
Flags: needinfo?(ryanvm)
Attachment #8460988 - Flags: approval-gaia-v2.1+
Flags: needinfo?(fabrice)
Blocking since it seems we just didn't get this onto master/2.1.
blocking-b2g: 2.1? → 2.1+
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.
Please provide the reproduce step for tester to verify this bug.Thanks!
Flags: needinfo?(anshulj)
Flags: needinfo?(anshulj)
You need to log in before you can comment on or make changes to this bug.