Closed Bug 1057905 Opened 10 years ago Closed 10 years ago

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

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::System
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
RESOLVED FIXED
Milestone:
2.1 S3 (29aug)
Project Flags:
blocking-b2g 1.4+
Tracking Flags:
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: wei.gao, Assigned: wei.gao)

Details

(Whiteboard: [sprd344854][sprd344856])

Attachments

(4 files, 1 obsolete file)

Avoid activeApp is null.patch
682 bytes, patch
timdream
: review+
Details | Diff | Splinter Review
[v1.4] pull request
46 bytes, text/x-github-pull-request
Details | Review
[master] pull request
46 bytes, text/x-github-pull-request
Details | Review
[v2.0] pull request
46 bytes, text/x-github-pull-request
Details | Review 
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.
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
Dear Tim 

Could you help to review this issue?
Thanks a great.
Attachment #8478057 - Flags: review?(timdream)
[Blocking Requested - why for this release]:
blocking-b2g: --- → 1.4?
Attachment #8478057 - Flags: review?(timdream) → review+
Is this 1.4 only or does master need it ,too?
Whiteboard: [sprd344854][sprd344856]
Attached file [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)
(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.
CI test has been passed.
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)
(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)
Attached file [master] pull request — 
This is a PR for master.
I am waiting for the result of CI test.

Thanks.
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
Closed: 10 years ago
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
(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)
(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)
(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)
(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)
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)
Attached file [v2.0] pull request (obsolete) — 
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)
(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.
Attachment #8480348 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Unfortunately the v2.0 PR needs rebasing :(
Flags: needinfo?(wei.gao)
(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)
(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)
Attached file [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)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: