[tarako][dolphin] Switch airplane mode over and over again, the airplane mode will be out of commission.

RESOLVED FIXED in Firefox OS v1.4

Status

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

People

(Reporter: Wei Gao (Spreadtrum), Assigned: Wei Gao (Spreadtrum))

Tracking

unspecified
2.0 S6 (18july)
ARM
Gonk (Firefox OS)
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:1.4+, b2g-v1.3T wontfix, b2g-v1.4 fixed, b2g-v2.0 verified, b2g-v2.1 verified)

Details

(URL)

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
OS version
---------------------------------------------
FireFoxOS v1.3t v1.4

Reproduce steps:
---------------------------------------------
Switch airplane mode over and over again.

Expected result:
---------------------------------------------
The airplane mode should be turned on and off all the time.

Actual result:
---------------------------------------------
The airplane mode will not be turned on or off.

Probability:
---------------------------------------------
Occasionally Recurrence
(Assignee)

Comment 1

4 years ago
Created attachment 8450764 [details] [diff] [review]
radio-enabled.patch

As for its cause, I think that's because the modem's slow response. That's to say, when dispatching 'radio-enabled' or 'radio-disabled', the radio has not been turned on or off yet. 
Then user turn airplane mode again, it will make radio and airplane-gaia chaotic.

https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/system/js/radio.js#L139-149
https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/system/js/radio.js#L164-172

I wonder can this patch be granted.
It will make "window.dispatchEvent" wait untill the radio is turned on or off thoroughly.

Hi Evelyn

Can you help to transmit this bug to the right owner?
Thanks a great.
Flags: needinfo?(ehung)
(Assignee)

Updated

4 years ago
Flags: needinfo?(james.zhang)

Updated

4 years ago
Attachment #8450764 - Flags: review?(ejchen)
Flags: needinfo?(ehung)
(Assignee)

Updated

4 years ago
Flags: needinfo?(james.zhang)
Comment on attachment 8450764 [details] [diff] [review]
radio-enabled.patch

Review of attachment 8450764 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Wei.gao, it makes sense to me and because radio.js resides in System app, you may need its owner's approval as well.

::: apps/system/js/radio.js
@@ +143,5 @@
>          self._setRadioAfterReqsCalled(enabled);
>        };
>  
>        req.onerror = function() {
> +        self._setRadioOpCount++;

Can you put this line below to make code more consistent? thanks :]
Attachment #8450764 - Flags: review?(ejchen)
Attachment #8450764 - Flags: review?(alive)
Attachment #8450764 - Flags: review+
By the way, Wei.gao, please make a PR for it and ask for approval to land.

Thanks !
(Assignee)

Comment 4

4 years ago
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #3)
> By the way, Wei.gao, please make a PR for it and ask for approval to land.

Hi EJ
Thanks for your review.
I will make a PR later. I want to know, is this needed for master also or v1.3t and v1.4 only?

Thanks.
Based on your title, this patch is for 1.3t right ? You can ask for approval for 1.3t first to let the team decide whether this should be taken or not. And also, you can create another PR for master (2.1). Thanks.
WeiGao, I think you can try to ask for approval for 1.3t, 1.4, 2.0 because this patch is valuable in some edge cases. Thanks for your help !!
Comment on attachment 8450764 [details] [diff] [review]
radio-enabled.patch

Review of attachment 8450764 [details] [diff] [review]:
-----------------------------------------------------------------

Better having tests.
Best using semaphore. (asyncSemaphore)
Attachment #8450764 - Flags: review?(alive) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 8452142 [details] [review]
[v1.3t] pull request

Hi EJ and Alive

This is a PR for v1.3t.
Please help to reviw it.
Thanks a great.
Attachment #8452142 - Flags: review?(ejchen)
Attachment #8452142 - Flags: review?(alive)
(Assignee)

Comment 9

4 years ago
Created attachment 8452143 [details] [review]
[v1.4] pull request

Hi EJ and Alive

This is a PR for v1.4.
Please help to reviw it.
Thanks a great again.
Attachment #8452143 - Flags: review?(ejchen)
Attachment #8452143 - Flags: review?(alive)
(Assignee)

Comment 10

4 years ago
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #7)
> Better having tests.
> Best using semaphore. (asyncSemaphore)

Hi Alive

Sorry for I can't get your mean?
Is there anything I can perfect it.
Thanks.
(Assignee)

Comment 11

4 years ago
Created attachment 8452216 [details] [review]
[v1.3t] pull request

I am sorry for some mistakes in that PR.
I recommit it again.
Please help to review it.
Attachment #8452142 - Attachment is obsolete: true
Attachment #8452142 - Flags: review?(ejchen)
Attachment #8452142 - Flags: review?(alive)
Attachment #8452216 - Flags: review?(ejchen)
Attachment #8452216 - Flags: review?(alive)
Comment on attachment 8452216 [details] [review]
[v1.3t] pull request

Please check my comment on github.
Attachment #8452216 - Flags: review?(ejchen)
Wei.gao, from AriplaneMode side, I am ok with your patch. But from System aspect, please follow Alive's comments.

If you get all r+ from Alive, I will automatically r+ after that. Thanks :)
(Assignee)

Comment 14

4 years ago
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #12)
> Comment on attachment 8452216 [details] [review]
> [v1.3t] pull request
> 
> Please check my comment on github.

Thank EJ(龙哥) for your suggestion. I have modified it.

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #13)
> Wei.gao, from AriplaneMode side, I am ok with your patch. But from System
> aspect, please follow Alive's comments.
> 
> If you get all r+ from Alive, I will automatically r+ after that. Thanks :)

Oh, I see.
Thank EJ(龙哥) again.
Attachment #8452143 - Flags: review?(alive) → review+
Comment on attachment 8452216 [details] [review]
[v1.3t] pull request

Squash two comments into one please.
Attachment #8452216 - Flags: review?(alive) → review+
(Assignee)

Comment 16

4 years ago
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #15)
> Comment on attachment 8452216 [details] [review]
> [v1.3t] pull request
> 
> Squash two comments into one please.

Thank Alive very much.
Could it be landed on now or a blocking-b2g flag should be added first?
(In reply to Wei Gao (Spreadtrum) from comment #16)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #15)
> > Comment on attachment 8452216 [details] [review]
> > [v1.3t] pull request
> > 
> > Squash two comments into one please.
> 
> Thank Alive very much.
> Could it be landed on now or a blocking-b2g flag should be added first?

I don't know. For v1.3t I guess it's fine, but I don't know the rule of v1.4 is changed or not.
If not, we need approval-gaia-v1.4
Can't find 1.3t?, so let me help you flag 1.4? first and ask later.
blocking-b2g: --- → 1.4?
(Assignee)

Comment 20

4 years ago
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #18)
> Wei.gao please check this :
> https://wiki.mozilla.org/Release_Management/B2G_Landing

Thank EJ(龍哥), that's usefull, I will check it.

Comment 21

4 years ago
Take in v1.4 but ni? Bhavana to see if it needs to be uplifted to 2.0.
blocking-b2g: 1.4? → 1.4+
Assignee: nobody → wei.gao
(Assignee)

Comment 22

4 years ago
Dear EJ

I don't know why there is nobody land PR on v1.3t and v1.4?
Is there anything I forgot to do?
Thanks.
Flags: needinfo?(ejchen)
(In reply to Wei Gao (Spreadtrum) from comment #22)
> Dear EJ
> 
> I don't know why there is nobody land PR on v1.3t and v1.4?
> Is there anything I forgot to do?
> Thanks.

You may missed my comment from comment #5

> Based on your title, this patch is for 1.3t right ? You can ask for approval
> for 1.3t first to let the team decide whether this should be taken or not.
> And also, you can create another PR for master (2.1). Thanks.

you have to click on the details of attachment and write reasons why you need 
approval-gaia-v1.4 and approval-gaia-v2.0.

And by the way, you have to create another PR for master as what I mentioned above.
Flags: needinfo?(ejchen)
And also, I think there may not be possible to land code to 1.3t because I can't find any approval flag for it. 

For more details, please check comemnt #18 and this would definitely help.

> Wei.gao please check this :
> https://wiki.mozilla.org/Release_Management/B2G_Landing
(Assignee)

Comment 25

4 years ago
Created attachment 8456096 [details] [review]
[master] pull request

Hi Alive and EJ 

This is a PR on master.
Please help to review again.
Thanks a great.
Attachment #8456096 - Flags: review?(ejchen)
Attachment #8456096 - Flags: review?(alive)
(Assignee)

Comment 26

4 years ago
Comment on attachment 8452143 [details] [review]
[v1.4] pull request

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):-
[User impact] if declined:Switch airplane mode over and over again, the airplane mode will be out of commission. That's because the modem's slow response, when dispatching 'radio-enabled' or 'radio-disabled', the radio has not been turned on or off yet. Then user turn airplane mode again, it will make radio and airplane-gaia chaotic.
[Testing completed]:pass on tarako and dolphin
[Risk to taking this patch] (and alternatives if risky):I think there is no risk.
[String changes made]:no
Attachment #8452143 - Flags: approval-gaia-v1.4?(lmandel)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #24)
> And also, I think there may not be possible to land code to 1.3t because I
> can't find any approval flag for it. 

Sorry, yeah, our 1.3T uplift situation is a bit of a mess :(. I think the way you want to go is get 1.3T+ blocking status for this bug to take care of that approval. You already have a v1.4 approval request on attachment 8452143 [details] [review], so that'll cover the 1.4 uplift. Not sure if this is something wanted for 2.0 or not, but you'll need to request approval for that as well if wanted.

Note that per our usual tree rules, the master patch still needs to land first before the branch uplifts. AFAICT, that's still waiting on reviews, so I've removed the checkin-needed keyword for now. Sorry again for all the confusion :(
Keywords: checkin-needed
Comment on attachment 8452143 [details] [review]
[v1.4] pull request

Clearing the approval request as this is already marked as 1.4+, which means that you're clear to land when ready.
Attachment #8452143 - Flags: approval-gaia-v1.4?(lmandel)
Attachment #8456096 - Flags: review?(alive) → review+
Comment on attachment 8456096 [details] [review]
[master] pull request

Thanks Wei.gao !
Attachment #8456096 - Flags: review?(ejchen) → review+
(Assignee)

Comment 30

4 years ago
I think it can be added "checkin-needed" now.
Keywords: checkin-needed
Thanks! I'm not sure what the procedure is for getting this on to 1.3T is at this point, but I'll get this on to v1.4 once this is confirmed OK on master.

Master: https://github.com/mozilla-b2g/gaia/commit/36d0be8b165f8b4b3ba73622a2d620a7bf52d5be
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)
(Assignee)

Comment 34

4 years ago
Dear Julien

I am sorry to trouble.
Do you know how to set a flag of 1.3t approval on this bug, as we all can't find this flag.
Thank you very much.
Flags: needinfo?(felash)
You have all necessary information in [1]. For v1.3t, you need to have the v1.3t+ blocker flag, so you need to request it. It looks like I don't find the flag either, NI Bhavana on this.

[1] https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.3T
Flags: needinfo?(felash) → needinfo?(bbajaj)
NI : wayne to set 1.3T+ if this is a blocker as requested in comment #34.
Flags: needinfo?(bbajaj) → needinfo?(wchang)
Not taking this on 1.3t now given it's at final stage. 
Edge case here, a user wouldn't hit airplane mode over and over again.
Flags: needinfo?(wchang)

Updated

4 years ago
status-b2g-v1.3T: affected → wontfix
Found Test Case: https://moztrap.mozilla.org/manage/case/1295/

STR needs to be added to verify this bug in the existing test case.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(dharris)
Steps have been added to cover this issue in this test case:

https://moztrap.mozilla.org/manage/case/1295/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(dharris)
Flags: in-moztrap+

Updated

3 years ago
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified

Comment 40

3 years ago
Created attachment 8533054 [details]
verify_video.MP4

According to steps of comment 38, this issue has been verified successfully on Flame v2.1 & v2.0
See attachment: verify_video.MP4
Reproducing rate: 0/5
Flame 2.1 versions:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141205.035305
FW-Date         Fri Dec  5 03:53:16 EST 2014
Bootloader      L1TC00011880

Flame 2.0 versions:
Gaia-Rev        856863962362030174bae4e03d59c3ebbc182473
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e40fe21e37f1
Build-ID        20141207000206
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141207.034341
FW-Date         Sun Dec  7 03:43:52 EST 2014
Bootloader      L1TC00011880
You need to log in before you can comment on or make changes to this bug.