[dolphin] If the current activeApp is null when receiving STK message, the STK will no longer work forever.

RESOLVED FIXED in Firefox OS v1.4

Status

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

People

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

Tracking

unspecified
2.1 S3 (29aug)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

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

Details

(Whiteboard: [sprd344854][sprd344856])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Reproduce steps:
---------------------------------------------
1) Restart phone
2) Open settings app, and go into STK menu.

Expected result:
---------------------------------------------
There should be normal response when click the label in STK menu.

Actual result:
---------------------------------------------
There is no response.

Probability:
---------------------------------------------
Always Recurrence using Airtel or Robi sim card.
(Assignee)

Comment 1

4 years ago
I think the rootcause is the current activeApp is null when receiving STK message.

https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/system/js/icc_worker.js#L227-231

After turning on ffos, if receiving STK message before the homescreen is started, the activeApp will be null, then [JavaScript Error] will occur and STK will no longer work forever.
status-b2g-v1.4: --- → affected
(Assignee)

Comment 2

4 years ago
Created attachment 8478057 [details] [diff] [review]
Avoid activeApp is null.patch

Dear Tim 

Could you help to review this issue?
Thanks a great.
Attachment #8478057 - Flags: review?(timdream)
(Assignee)

Comment 3

4 years ago
[Blocking Requested - why for this release]:
blocking-b2g: --- → 1.4?
Attachment #8478057 - Flags: review?(timdream) → review+

Comment 4

4 years ago
Is this 1.4 only or does master need it ,too?
(Assignee)

Updated

4 years ago
Whiteboard: [sprd344854][sprd344856]
(Assignee)

Comment 5

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

Dear Tim

Thanks for your review.
This is a PR for it.
I am sorry to trouble you again for reviw.
Thanks so much.
Attachment #8478066 - Flags: review?(timdream)
Comment on attachment 8478066 [details] [review]
[v1.4] pull request

Actually I should only review PR not the patch after it had shown green on our CI, but since I did you do not need to ask for review again. Please check the CI and ensure you are not triggering any errors when merging.

You should also ask for approval‑gaia‑v1.4 before landing.
Attachment #8478066 - Flags: review?(timdream)
(Assignee)

Comment 7

4 years ago
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #6)
> Comment on attachment 8478066 [details] [review]
> [v1.4] pull request
> 
> Actually I should only review PR not the patch after it had shown green on
> our CI, but since I did you do not need to ask for review again. Please
> check the CI and ensure you are not triggering any errors when merging.
> 
> You should also ask for approval‑gaia‑v1.4 before landing.

Thanks for your advice.
I will wait for the result of CI test.
(Assignee)

Comment 8

4 years ago
CI test has been passed.
(Assignee)

Comment 9

4 years ago
Hi Wayne

Could you help to add a flag block 1.4+
Thanks.
Flags: needinfo?(wchang)
From the bug description, it sounds like Master should also have this problem.

Wei,
Can you confirm?

p.s. landing on 1.4 requires landing on master first.
Flags: needinfo?(wchang)
(Assignee)

Comment 11

4 years ago
(In reply to Wayne Chang [:wchang] from comment #10)
> From the bug description, it sounds like Master should also have this
> problem.
> 
> Wei,
> Can you confirm?
> 
> p.s. landing on 1.4 requires landing on master first.

Thanks for your reply. I am sure master also needs this patch, as 1.4 uses the same method with master to get current activeApp which will causes problem if activeApp is null.

I will commit a PR for master now.
Thank you Wei. Please make sure https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_3 is followed.

1.4+ since this corrects a previously undetected always fail case.


Bhavana,

I think this is worth being considered for 2.0 as well, change is small and makes sense.
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(bbajaj)
(Assignee)

Comment 13

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

This is a PR for master.
I am waiting for the result of CI test.

Thanks.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/c474be573919a4f6df7e6d5f65784ca9b60d06f4

If you think this should land on v2.0, please request approval on the patch and answer the questions asked there.
Assignee: nobody → wei.gao
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
(Assignee)

Comment 15

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> Master:
> https://github.com/mozilla-b2g/gaia/commit/
> c474be573919a4f6df7e6d5f65784ca9b60d06f4
> 
> If you think this should land on v2.0, please request approval on the patch
> and answer the questions asked there.

Dear Ryan

How about landing on v1.4?
Block v1.4+ has been added, so can this patch be landed on v1.4 now?
Or is a approval request for 1.4 also needed before landing? 

Thanks so much.
Flags: needinfo?(ryanvm)
It'll be uplifted to v1.4 once it's confirmed to have stuck on master. There's no need to do anything else on your end since it already has 1.4+ status.
Flags: needinfo?(ryanvm)
(Assignee)

Comment 17

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> It'll be uplifted to v1.4 once it's confirmed to have stuck on master.
> There's no need to do anything else on your end since it already has 1.4+
> status.

I am sorry, but what does "stuck on master" mean? How to confirm this?
Thanks.
It means it has to have landed on master and gone through CI so we can verify that it isn't causing new failures before we'll uplift it.

v1.4: https://github.com/mozilla-b2g/gaia/commit/05653cb12d324649687dad3eeb2ea373a2ad84d4
status-b2g-v1.4: affected → fixed
Can you please ask 2.0 approval here after which we can consider landing it on that branch ?
Flags: needinfo?(bbajaj) → needinfo?(wei.gao)
(Assignee)

Comment 20

4 years ago
(In reply to bhavana bajaj [:bajaj] from comment #19)
> Can you please ask 2.0 approval here after which we can consider landing it
> on that branch ?

Of course, I will do it now.
Flags: needinfo?(wei.gao)
(Assignee)

Comment 21

4 years ago
(In reply to bhavana bajaj [:bajaj] from comment #19)
> Can you please ask 2.0 approval here after which we can consider landing it
> on that branch ?

Dear bhavana

I am afraid the PR on master can't be merged into v2.0, as their codes are a little different from each other.
So, do I need to commit a new PR for v2.0 or ask 2.0 approval on PR of master directly?
Could you give me some advice?

Thanks.
Flags: needinfo?(bbajaj)
(Assignee)

Comment 22

4 years ago
Dear Wayne

What should I do about 2.0 approval request in Comment21.
Could you give me a hand?

Thanks.
Flags: needinfo?(wchang)
Hi Wei,

Thanks! You can create a new PR for v2.0 like you did for [v1.4], and set approval‑gaia‑v2.0?=bbajaj@mozilla.com for that PR
Flags: needinfo?(wchang)
(In reply to Wayne Chang [:wchang] from comment #23)
> Hi Wei,
> 
> Thanks! You can create a new PR for v2.0 like you did for [v1.4], and set
> approval‑gaia‑v2.0?=bbajaj@mozilla.com for that PR

Thanks wayne! Wei, hope this is helpful.
Flags: needinfo?(bbajaj)
(Assignee)

Comment 25

4 years ago
Created attachment 8480348 [details] [review]
[v2.0] 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 #):no
[User impact] if declined:If b2g receives STK message before the homescreen is started, the activeApp will be null, then [JavaScript Error] will occur and STK will no longer work forever unless restart b2g.
[Testing completed]:test pass.
[Risk to taking this patch] (and alternatives if risky):I am sure no risk.
[String changes made]:no
Attachment #8480348 - Flags: approval-gaia-v2.0?(bbajaj)
(Assignee)

Comment 26

4 years ago
(In reply to Wayne Chang [:wchang] from comment #23)
> Hi Wei,
> 
> Thanks! You can create a new PR for v2.0 like you did for [v1.4], and set
> approval‑gaia‑v2.0?=bbajaj@mozilla.com for that PR

Thank Wayne and bhavana,
I have made the approval request for v2.0, please check it.
Thanks.

Updated

4 years ago
Attachment #8480348 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Unfortunately the v2.0 PR needs rebasing :(
Flags: needinfo?(wei.gao)
(Assignee)

Comment 28

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> Unfortunately the v2.0 PR needs rebasing :(

Dear Ryan

I am sorry, I can't understand your mean.
So, what should I do for it?
Flags: needinfo?(ryanvm)
Github says that the PR can't be automatically merged, which usually means that your patch needs to be rebased on top of whatever bitrotted it.
Flags: needinfo?(ryanvm)
(Assignee)

Comment 30

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #29)
> Github says that the PR can't be automatically merged, which usually means
> that your patch needs to be rebased on top of whatever bitrotted it.

Oh, I see, I will check it now.
Thanks so much.
Flags: needinfo?(wei.gao)
(Assignee)

Comment 31

4 years ago
Created attachment 8481062 [details] [review]
[v2.0] pull request

Dear Ryan

I have recommit a new PR for v2.0.
Thanks.
Flags: needinfo?(ryanvm)
Attachment #8480348 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
v2.0: https://github.com/mozilla-b2g/gaia/commit/bee44b6fa2f19db606359d6756638f63097cf4dc

Thanks :)
status-b2g-v2.0: affected → fixed
You need to log in before you can comment on or make changes to this bug.