800263 - [FM Radio] Launch the FM Radio with headphone already plugged-in will cause screen freeze

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[John Shih]

## Environment :
Otoro phone, build 2012-10-11
Build info: 
* "gaia" revision= beee2bf29e779b12306b16c2bc2785c1c22ede45
* "gecko" revision= c0ea0728044b2a5f9556ca988635140963cff037

## Repro :
1. Plug in the headphone
2. Launch the FM Radio app
3. Try to press the buttons

## Expected:
* The app works fine

## Actual:
* The screen will freeze and can't do anything (even press the sleep button) but put off the battery

## Note:
* It works fine if launch the app first, then plug in the headphone

Comment 1

[Tony Chung [:tchung]]

This is absolutely blocking.  I can reproduce this on the Unagi phone, 10-11 build, and my phone completely freezes. i have to reboot the phone.

logcat attached, but i dont see anything that logs the freeze.

Comment 2

[Tony Chung [:tchung]]

Attachment: logcat freeze

Comment 3

[[:fabrice] Fabrice Desré]

I reproduced here, but when the freeze happened, the phone rebooted by itself after a couple of minutes.

I guess something was churning in the background that finally crashed us hard.

Comment 4

[[:fabrice] Fabrice Desré]

I did a couple of runs with gdb but gdb is getting killed also and I could not get a backtrace of anything.

Comment 5

[Pin Zhang [:pzhang] (inactive)]

I updated my Otoro device on 10-09 Beijing time, and there is no such problem, so it is caused by some commits of 10-09, 10-10 or 10-11.

Comment 7

[Pin Zhang [:pzhang] (inactive)]

(In reply to Ray Cheung [:pzhang] from comment #5)
> I updated my Otoro device on 10-09 Beijing time, and there is no such
> problem, so it is caused by some commits of 10-09, 10-10 or 10-11.
I tried to flash the same version to another Otoro device, and it could be reproduced, it's weird ...

Comment 8

[Pin Zhang [:pzhang] (inactive)]

(In reply to Ray Cheung [:pzhang] from comment #7)
> (In reply to Ray Cheung [:pzhang] from comment #5)
> > I updated my Otoro device on 10-09 Beijing time, and there is no such
> > problem, so it is caused by some commits of 10-09, 10-10 or 10-11.
> I tried to flash the same version to another Otoro device, and it could be
> reproduced, it's weird ...
I checked the buildId of the two Otoro devices (cat /system/b2g/application.ini), they are different, the good one was built on 20121009, and the bad one was built on 20121010, both are Beijing time, so the date of the commits which caused the problem should be 10-09.

Comment 9

[Chris Lee [:clee]]

Does the kernel update 10 fix this issue?

Comment 10

[James Ho]

John, could you confirm this?

Comment 11

[Al Tsai [:atsai]]

I updated my otoro to kernel update 10. It still crashes with below log.

E/GeckoConsole( 2480): Content JS LOG at app://fm.gaiamobile.org/js/fm.js:142 in updatePowerUI: Power status: on

Comment 12

[James Ho]

assigning to Ray, feel free to reassign if you don't see fit.

Comment 13

[Pin Zhang [:pzhang] (inactive)]

Here is the commit which causes the regression:
  commit da394578704f4df5b14a6e2ed69e2d204a14b0c5
  Author: Dave Hylands <[e-mail]>
  Date:   Mon Oct 8 22:46:19 2012 -0600

      Bug 797239 - Defer scheduling prelaunch app until child goes idle. r=cjones

Comment 14

[Pin Zhang [:pzhang] (inactive)]

Dave was the owner of Bug 797239, reassign this bug to Dave.

Comment 15

[Dave Hylands [:dhylands]]

I'm not sure how bug 797239 can be responsible for this other than to change the timings of things (which would imply that there is a race condition somewhere) and defering the scheduling of the prelaunch app may very well cause the timings to change.

So I don't think defering the prelaunch caused the bug, but rather just exposed the bug.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I agree with that analysis.  Qualcomm can't reproduce this bug on their development devices, so I think we're just exposing a race condition in the older version of this driver that we're using.

Comment 17

[Pin Zhang [:pzhang] (inactive)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> I agree with that analysis.  Qualcomm can't reproduce this bug on their
> development devices, so I think we're just exposing a race condition in the
> older version of this driver that we're using.
Is it possible that the Bug797698 is the same issue?

Comment 18

[Dave Hylands [:dhylands]]

I'm still using one kernel revision behind.

I played with various times of dom.ipc.processPrelaunch.delayMs (5, 10, and 20 sec) and the screen freeze occurs very shortly after the prelaunch. And in every case, the phone reboots very shortly afterwards.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Ray Cheung [:pzhang] from comment #17)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> > I agree with that analysis.  Qualcomm can't reproduce this bug on their
> > development devices, so I think we're just exposing a race condition in the
> > older version of this driver that we're using.
> Is it possible that the Bug797698 is the same issue?

Available evidence suggests a kernel bug, and the bug isn't reproducible in never builds, so we may need an otoro/unagi-specific workaround in gecko :/.

Comment 20

[John Shih]

doesn't happen with smoke test today
build info
gaia master: 98498cb5803349b50637dc35640fc2235fee0499
mozilla-central : bbf233397549175223da3687bc5678d91685e54a

Comment 22

[StevenLee[:slee]]

I can reproduce this problem for today's code.
## Environment :
Otoro phone, build 2012-10-19
Build info: 
* "gecko" revision= cc11217f371bbe3f555d4c4f23ec9c817f3171df

## Repro :
1. Plug in the headphone
2. flash.sh 
3. Launch the FM Radio app
4. Try to press the buttons

Comment 23

[StevenLee[:slee]]

Attachment: patch

I found that this problem is always reproducible on my device after I flash the whole image. In this patch, I modified the initial function of FM radio. Just follow the initialization method, fm_turn_on, in [1]. It uses |property_set| and checks the property to determine whether the initialization is done. Please help me check whether the patch works on your devices. BTW, should we contact the vendor about this issue?

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=749053&attachment=656384

Comment 24

[Al Tsai [:atsai]]

Tested with patch, it works fine.

## Environment:
# Otoro phone, build 2012-10-22
Build Info:
* gecko: 2a4606ceba876a3008f490af2b0fe726e538f27c
with patch

Comment 25

[Chris Lee [:clee]]

Looks like we need a review and then once r+'ed, we should land this asap.  Thanks.

Comment 26

[StevenLee[:slee]]

(In reply to Chris Lee [:clee] from comment #25)
> Looks like we need a review and then once r+'ed, we should land this asap. 
> Thanks.

Hi clee,

This is just a WIP version. I created bug 804455 for discussing if the patch is really solving this problem.

Comment 27

[Dave Hylands [:dhylands]]

I can confirm that this patch resolves the issue for me on my unagi.

Comment 28

[John Shih]

I met this bug with today build
2012-10-25
gaia master : 4b999a7866c01a61642fa2015df0868d6eb68c88
gecko : 6bb4d66668895b4d24434ce90f483a5246beccd1

Comment 30

[Faramarz Rashed]

smoke-test blockers have the highest priority.

Comment 31

[Andrew Overholt [:overholt]]

How is this work going, Steven?

Comment 32

[StevenLee[:slee]]

Hi Andrew, 
I have a work around patch and waiting for mwu's response in Bug 804455. BTW, should I mark this bug dup?

Comment 33

[StevenLee[:slee]]

Attachment: patch

Comment 35

[Michael Wu [:mwu]]

Comment on attachment 678216 [details] [diff] [review]
patch

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

All nits but these add up and make the patch look ugly.

::: hal/gonk/GonkFMRadio.cpp
@@ +75,5 @@
> +  int rc;
> +  snprintf(version, sizeof(version), "%d", sTavaruaVersion);
> +  property_set("hw.fm.version", version);
> +
> +  /*Set the mode for soc downloader*/

Add a space after /* and before */.

@@ +79,5 @@
> +  /*Set the mode for soc downloader*/
> +  property_set("hw.fm.mode", "normal");
> +  /* start fm_dl service */
> +  property_set("ctl.start", "fm_dl");
> +  

Trailing whitespace.

@@ +87,5 @@
> +   * work around is from codeaurora
> +   * (git://codeaurora.org/platform/frameworks/base.git).
> +   */
> +  for (int i = 0; i < 4; ++i)
> +  {

{ on the same line as for.

@@ +91,5 @@
> +  {
> +    sleep(1);
> +    char value[PROPERTY_VALUE_MAX];
> +    property_get("hw.fm.init", value, "0");
> +    if(!strcmp(value, "1")) {

Space after if.

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 678216 [details] [diff] [review]
patch

>diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp

>+  for (int i = 0; i < 4; ++i)
>+  {

Nit: style is "{" on the same line, 

  for (int i = 0; i < 4; ++i) {

Looks good, thanks.  r=me with that change.

Comment 37

[StevenLee[:slee]]

Attachment: patch v2

address comment #36

Comment 38

[StevenLee[:slee]]

Attachment: patch v3

Address comment #35, I did not see mwu's comment last time.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f04762dc07

Comment 40

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d4f04762dc07

Comment 41

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6cfcecb75844

Comment 42

[John Shih]

verified

build info
2012-11-09
gaia master : 18107c6961b8c3f559c82fc7be8ba411c4a67c28
mozilla-aurora(hg) : 6735d6d13751