Code to removeEventListener for radiostatechange in radio.js not working

RESOLVED FIXED in Firefox OS v2.0

Status

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

People

(Reporter: Anshul, Assigned: Anshul)

Tracking

unspecified
2.1 S1 (1aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

197 bytes, text/html
arthurcc
: review+
Details
(Assignee)

Description

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

Comment 1

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

Comment 2

4 years ago
Created attachment 8460982 [details]
Github PR
(Assignee)

Updated

4 years ago
Attachment #8460982 - Attachment description: 22082[1] → Github PR
(Assignee)

Updated

4 years ago
Attachment #8460982 - Flags: review?(arthur.chen)
(Assignee)

Comment 3

4 years ago
Created attachment 8460988 [details]
Github PR
Attachment #8460982 - Attachment is obsolete: true
Attachment #8460982 - Flags: review?(arthur.chen)
Attachment #8460988 - Flags: review?(arthur.chen)
(Assignee)

Updated

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

Comment 7

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

Updated

4 years ago
status-b2g-v2.0: --- → affected
(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)

Updated

4 years ago
Blocks: 1041241
No longer blocks: 1011657
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
Last Resolved: 4 years ago
status-b2g-v2.1: --- → fixed
Flags: needinfo?(arthur.chen)
Resolution: --- → FIXED
v2.0: https://github.com/mozilla-b2g/gaia/commit/6d97765a49b510aa7c9dd31e8c7541eea6af925d
status-b2g-v2.0: affected → fixed
Target Milestone: --- → 2.1 S1 (1aug)
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-
(Assignee)

Comment 13

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

Comment 16

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

Comment 17

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

Comment 19

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

Updated

4 years ago
Depends on: 1025317
(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+
(Assignee)

Comment 22

4 years ago
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.
status-b2g-v2.1: fixed → affected
v2.1: https://github.com/mozilla-b2g/gaia/commit/857ac4fde427704cff87e174cab2020cba98d5ff
status-b2g-v2.0M: --- → fixed
status-b2g-v2.1: affected → fixed
status-b2g-v2.2: --- → fixed

Comment 24

4 years ago
Please provide the reproduce step for tester to verify this bug.Thanks!
Flags: needinfo?(anshulj)
(Assignee)

Updated

4 years ago
Flags: needinfo?(anshulj)
You need to log in before you can comment on or make changes to this bug.