Closed Bug 800263 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

VERIFIED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: johnshih.bugs, Assigned: slee)

References

Details

(Keywords: smoketest, Whiteboard: [caf:blocking])

Attachments

(2 files, 3 obsolete files)

## 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
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.
blocking-basecamp: --- → +
OS: Linux → Gonk
Hardware: x86_64 → ARM
Attached file logcat freeze
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.
I did a couple of runs with gdb but gdb is getting killed also and I could not get a backtrace of anything.
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.
Duplicate of this bug: 800361
(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 ...
(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.
Does the kernel update 10 fix this issue?
John, could you confirm this?
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
assigning to Ray, feel free to reassign if you don't see fit.
Assignee: nobody → pzhang
Here is the commit which causes the regression:
  commit da394578704f4df5b14a6e2ed69e2d204a14b0c5
  Author: Dave Hylands <dhylands@mozilla.com>
  Date:   Mon Oct 8 22:46:19 2012 -0600

      Bug 797239 - Defer scheduling prelaunch app until child goes idle. r=cjones
Dave was the owner of Bug 797239, reassign this bug to Dave.
Assignee: pzhang → 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.
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.
(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?
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.
Priority: -- → P1
(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 :/.
doesn't happen with smoke test today
build info
gaia master: 98498cb5803349b50637dc35640fc2235fee0499
mozilla-central : bbf233397549175223da3687bc5678d91685e54a
Duplicate of this bug: 803365
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
Attached patch patch (obsolete) — Splinter Review
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
Tested with patch, it works fine.

## Environment:
# Otoro phone, build 2012-10-22
Build Info:
* gecko: 2a4606ceba876a3008f490af2b0fe726e538f27c
with patch
Depends on: 804455
Looks like we need a review and then once r+'ed, we should land this asap.  Thanks.
(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.
I can confirm that this patch resolves the issue for me on my unagi.
I met this bug with today build
2012-10-25
gaia master : 4b999a7866c01a61642fa2015df0868d6eb68c88
gecko : 6bb4d66668895b4d24434ce90f483a5246beccd1
Duplicate of this bug: 805706
Assignee: dhylands → slee
Whiteboard: [caf:blocking]
smoke-test blockers have the highest priority.
Severity: normal → critical
Keywords: smoketest
How is this work going, Steven?
Flags: needinfo?(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?
Flags: needinfo?(slee)
Attached patch patch (obsolete) — Splinter Review
Attachment #673147 - Attachment is obsolete: true
Attachment #678216 - Flags: review?(jones.chris.g)
Duplicate of this bug: 804455
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 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.
Attachment #678216 - Flags: review?(jones.chris.g) → review+
Attached patch patch v2 (obsolete) — Splinter Review
address comment #36
Attachment #678216 - Attachment is obsolete: true
Attachment #679008 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch patch v3Splinter Review
Address comment #35, I did not see mwu's comment last time.
Attachment #679008 - Attachment is obsolete: true
Attachment #679014 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d4f04762dc07
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
verified

build info
2012-11-09
gaia master : 18107c6961b8c3f559c82fc7be8ba411c4a67c28
mozilla-aurora(hg) : 6735d6d13751
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.