Closed Bug 1212366 Opened 9 years ago Closed 9 years ago

[Aries] Phone crashes and reboots after tap-holding apps on Homescreen while using screen reader

Categories

(Core :: Audio/Video: MediaStreamGraph, defect)

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Project Flags:
blocking-b2g 2.5+
Tracking Flags:
Tracking Status
firefox44 --- fixed
b2g-master --- affected

People

(Reporter: yzen, Assigned: m_kato)

References

Details

(4 keywords)

Attachments

(2 files, 1 obsolete file)

Part 2. Don't release MediaStream until Destroy is called
3.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 1. Don't call SetAudioOutputVolume if stream is destroyed
791 bytes, patch
roc
: review+
Details | Diff | Splinter Review 
This is likely an issue lower than Gaia, e.g. gecko.
When using a screen reader and exploring the screen with lots of controls (that try to get announced) the phone restarts. This is pretty frequent and happens at least once every 3-5 minutes.

Here's a log https://public.etherpad-mozilla.org/p/eM1puCAGAB
Note: log from somewhere around the middle is an after restart one.
Could you at least provide a crash signature or crash report link? Screen reader hasn't been the most stable; we had it crashing all the time and then we were told not to test it anymore several months ago.
Flags: needinfo?(yzenevich)
(In reply to Pi Wei Cheng [:piwei] from comment #1)
> Could you at least provide a crash signature or crash report link? Screen
> reader hasn't been the most stable; we had it crashing all the time and then
> we were told not to test it anymore several months ago.

Thanks Pi, I will first thing tomorrow. Would you be able to point me to the person that made the call about not testing the screen reader? Thanks again
Flags: needinfo?(pcheng)
It was the QA manager at that time; she's no longer at Mozilla. It's not an official announcement - We encountered crashes like bug 1142747 and then we were told not to exploratory test on screen reader anymore. It's not anything official.
Flags: needinfo?(pcheng)
(In reply to Pi Wei Cheng [:piwei] from comment #3)
> It was the QA manager at that time; she's no longer at Mozilla. It's not an
> official announcement - We encountered crashes like bug 1142747 and then we
> were told not to exploratory test on screen reader anymore. It's not
> anything official.

Sounds good, I'll try to revive the issue you mentioned once I get the crash info and a better STR.
Flags: needinfo?(yzenevich)
Flags: needinfo?(pcheng)
All the crash reports have missing symbols; they're incomplete.

I feel this could be a dupe or related to bug 1142747. I tried on Aries sending an SMS and my phone just crashed and rebooted when I hit the send button. Bug 1142747 has something to do with keyboard as well. Could you confirm that you were always doing something with keyboard when this happens?
Flags: needinfo?(pcheng) → needinfo?(yzenevich)
(In reply to Pi Wei Cheng [:piwei] from comment #6)
> All the crash reports have missing symbols; they're incomplete.
> 
> I feel this could be a dupe or related to bug 1142747. I tried on Aries
> sending an SMS and my phone just crashed and rebooted when I hit the send
> button. Bug 1142747 has something to do with keyboard as well. Could you
> confirm that you were always doing something with keyboard when this happens?

Keyboard is not necessary, it happens pretty reliably on homescreen too
Flags: needinfo?(yzenevich)
I'm getting the crash below (still with missing symbols though) pretty consistently if I tap and hold my finger over homescreen and just slide my finger around homescreen making it try to announce a whole bunch of things at a time and the phone will reboot:

https://crash-stats.mozilla.com/report/index/8566d89b-d62d-4130-abb8-833d42151008

Crashed the phone 3 times and they all came up with the above crash signature with missing symbols.

Could you narrow down your crash to one or two STRs and then I can try and see if I can reproduce, and if I can I'll go ahead and branch check. Right now we're kind of going back and forth without confirming each other's findings. Thanks.
Flags: needinfo?(yzenevich)
So consistent steps to reproduce:

* Enable screen reader
* Stay on home screen
* Tap and hold the screen and start exploring the homescreen (without interrupting the touch)
* The phone crashes within the first 10 seconds after starting to quickly exploring different things on the home screen

Rate of success: 3/3
Flags: needinfo?(yzenevich)
Comment 9 issue is reproducible on latest Aries. Phone reboots when following STR.

Device: Aries 2.5
BuildID: 20151012110146
Gaia: 87f5c9d55ab6a77dcfa48a3f3a8b4f5016f3c657
Gecko: 0b69d304f861d0038fb78f1d52b0f5d13ef7c6fe
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 44.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Unable to reproduce comment 9 issue on Flame 2.5. It seems that Flame is too slow to handle this action and it would delay all the outputs (announcements) after I release my finger at step 3.

Device: Flame 2.5 (319/512MB)
BuildID: 20151012030617
Gaia: 87f5c9d55ab6a77dcfa48a3f3a8b4f5016f3c657
Gecko: 0b69d304f861d0038fb78f1d52b0f5d13ef7c6fe
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 44.0a1 (2.5) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-master: --- → affected
Flags: needinfo?(jmercado)
Keywords: qawanted
Summary: Phone often crashes when using the screen reader. → [Aries] Phone crashes and reboots after tap-holding apps on Homescreen while using screen reader
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
Would it be possible to get a regression window on aries for this one.
Keywords: regressionwindow-wanted
Let's establish on whether this is a regression on Aries at all. QAwanted to test on an older Aries build.
Keywords: regressionwindow-wanted → qawanted
This issue does NOT occur on the following Aries build.

Device: Aries 2.5
BuildID: 20150516110322
Gaia: 4c0f36e9dfe017bf2a698d416a57c8156b43383d
Gecko: 0273e9c967ec
Gonk: 3af1ede0d0956cfbf9c549df7cd9a6807a9efdf2
Version: 41.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Working on getting the window.
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
Keywords: qawanted → regression, regressionwindow-wanted
QA Contact: pcheng
mozilla-inbound regression window:

Last Working
Device: Aries 2.5
BuildID: 20150901083809
Gaia: c80e8ff25425b007181fd6e3de0500a0358fab37
Gecko: dffea8ce8b6073c522d7ea128ad0aee2efdfe66d
Version: 43.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

First Broken
Device: Aries 2.5
BuildID: 20150901095213
Gaia: c80e8ff25425b007181fd6e3de0500a0358fab37
Gecko: fdd0c566464b141f905876e97874e952981798e1
Version: 43.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dffea8ce8b6073c522d7ea128ad0aee2efdfe66d&tochange=fdd0c566464b141f905876e97874e952981798e1

This issue is likely caused by changes made in bug 1191667. Also changing component to Core/DOM.
Blocks: 1191667
Component: Gaia → DOM
Keywords: regressionwindow-wanted
Product: Firefox OS → Core
Version: unspecified → Trunk
QA Whiteboard: [QAnalyst-Triage?]
Keywords: crash, reproducible
Makoto this issue seems to have been caused by the changes for bug 1191667.  Can you please take a look?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado) → needinfo?(m_kato)
(In reply to Jayme Mercado [:JMercado] from comment #15)
> Makoto this issue seems to have been caused by the changes for bug 1191667. 
> Can you please take a look?

OK.

This bug depends on bug 1191814 instead of bug 1191667.  Because bug 1191814 isn't valid for audio indicator support.  After bug 1191667 is fixed, audio indicator will work correctly even if direct audio speech.

I think that this root cause of this crash is that nsSpeechTask is released before SetAudioOutputVolumeImpl is called.  So mStream is already freed, so this crash will occur.
Assignee: nobody → m_kato
Flags: needinfo?(m_kato)
Humm, a lot of MediaSteam's method is async medthod without holding own's refcount, so when calling real method (*Impl) on another thread, it doesn't know whether mStream is still alive.
Attachment #8675685 - Attachment description: Part 1. Check whether the stream is destroyed on SetAudioOutputVolume. → Part 2. Don't release MediaStream until Destroy is called
Comment on attachment 8675685 [details] [diff] [review]
Part 2. Don't release MediaStream until Destroy is called

Yura, could you test this since I have no test environment.  If we reproduce this on Flame, I can test it.
Attachment #8675685 - Flags: feedback?(yzenevich)
Comment on attachment 8675686 [details] [diff] [review]
Part 1. Check whether the stream is destroyed on SetAudioOutputVolume.

Add check whether stream is destroyed.  Also this is required too.
Attachment #8675686 - Flags: feedback?(yzenevich)
(In reply to Makoto Kato [:m_kato] from comment #20)
> Comment on attachment 8675685 [details] [diff] [review]
> Part 2. Don't release MediaStream until Destroy is called
> 
> Yura, could you test this since I have no test environment.  If we reproduce
> this on Flame, I can test it.

Hi Makoto, yes it's present on flame as well.
(In reply to Yura Zenevich [:yzen] from comment #22)
> (In reply to Makoto Kato [:m_kato] from comment #20)
> > Comment on attachment 8675685 [details] [diff] [review]
> > Part 2. Don't release MediaStream until Destroy is called
> > 
> > Yura, could you test this since I have no test environment.  If we reproduce
> > this on Flame, I can test it.
> 
> Hi Makoto, yes it's present on flame as well.

Hi Makoto, can't build it on mac for aries, but as mentioned above should work for flame. I will try building for flame in the meanwhile as well.
According to my testing at comment 10 this crash is NOT reproducible on Flame.
Note I reproduce bug 1214149 on Flame, so if this is a dupe the issue happens everywhere.
Component: DOM → Audio/Video: MSG/cubeb/GMP
Please have an MSG person look at this when it's ready for review.
OK tested on flame. Current master vs Current master + the patches.

I could crash the phone twice with the current master though it was not as easy as on Aries. 

I could not crash the phone with patches applied and tried quick navigation for about 3-5 minutes without stopping.

It looks promising!
Flags: needinfo?(m_kato)
Comment on attachment 8675685 [details] [diff] [review]
Part 2. Don't release MediaStream until Destroy is called

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

I'm not the right person to review this, but I could not crash a flame device with these patches applied
Attachment #8675685 - Flags: feedback?(yzenevich)
Comment on attachment 8675686 [details] [diff] [review]
Part 1. Check whether the stream is destroyed on SetAudioOutputVolume.

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

I'm not the right person to review this, but I could not crash a flame device with these patches applied
Attachment #8675686 - Flags: feedback?(yzenevich)
Andre, can you review these patches?
blocking-b2g: --- → 2.5+
Flags: needinfo?(anatal)
Or maybe Kelly?
Flags: needinfo?(kdavis)
I can take a look, but I think it is related to the speech synth code which is more Eitan's baby.

However, Eitan is on vacation until the 28th.
Flags: needinfo?(kdavis)
(In reply to kdavis from comment #34)
> I can take a look, but I think it is related to the speech synth code which
> is more Eitan's baby.

If possible, that'd be great because AIUI 2.5 is supposed to release on Nov 2 which is pretty tight given Eitan's PTO.
Flags: needinfo?(m_kato)
Comment on attachment 8675686 [details] [diff] [review]
Part 1. Check whether the stream is destroyed on SetAudioOutputVolume.

When stream is destroyed, it is unnecessary to set volume.
Attachment #8675686 - Flags: review?(cpearce)
Comment on attachment 8675686 [details] [diff] [review]
Part 1. Check whether the stream is destroyed on SetAudioOutputVolume.

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

roc should review MSG patches.
Attachment #8675686 - Flags: review?(cpearce) → review?(roc)
Comment on attachment 8675685 [details] [diff] [review]
Part 2. Don't release MediaStream until Destroy is called

Roc, Eitan is PTO, could you review this?

When calling Destroyed(), it append MSG to queue.  So we should wait releasing stream until destroy MSG calls Run().
Attachment #8675685 - Flags: review?(roc)
Comment on attachment 8675686 [details] [diff] [review]
Part 1. Check whether the stream is destroyed on SetAudioOutputVolume.

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

::: dom/media/MediaStreamGraph.cpp
@@ +1803,5 @@
>    };
> +  // If the stream is destroyed, it will not apply volume.
> +  if (!IsDestroyed()) {
> +    GraphImpl()->AppendMessage(new Message(this, aKey, aVolume));
> +  }

I would prefer to assert that the stream is not destroyed, and move this check to the caller in nsSpeechTask. Stream users should not be calling SetAudioOutputVolume (or most other methods) on destroyed streams.
Attachment #8675686 - Flags: review?(roc)
Comment on attachment 8675685 [details] [diff] [review]
Part 2. Don't release MediaStream until Destroy is called

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

::: dom/media/webspeech/synth/nsSpeechTask.cpp
@@ +136,5 @@
>    if (mStream) {
>      if (!mStream->IsDestroyed()) {
>        mStream->Destroy();
>      }
> +    // This will finally destoryed by SynthStreamListener becasue

destroyed
Attachment #8675685 - Flags: review?(roc) → review+
Nice! I go to sleep (CEST), wake up, and there is a patch! I wish it was always that easy.

Thanks Makoto and Robert!
Comment on attachment 8677883 [details] [diff] [review]
Part 1. Don't call SetAudioOutputVolume if stream is destroyed

Before calling SetAudioOutputVolume(), we check whether stream is destroyed on SpeechTask
Attachment #8677883 - Flags: review?(roc)
Attachment #8675686 - Attachment is obsolete: true
Thank you Makoto for tackling that.
Flags: needinfo?(anatal)
https://hg.mozilla.org/mozilla-central/rev/87b6b058c8ca
https://hg.mozilla.org/mozilla-central/rev/3d02436df842
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: