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)
Tracking
()
People
(Reporter: yzen, Assigned: m_kato)
References
Details
(4 keywords)
Attachments
(2 files, 1 obsolete file)
3.03 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
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)
Reporter | ||
Comment 2•9 years ago
|
||
(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)
Comment 3•9 years ago
|
||
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)
Reporter | ||
Comment 4•9 years ago
|
||
(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)
Reporter | ||
Comment 5•9 years ago
|
||
Here are the report url:
https://crash-stats.mozilla.com/report/index/b5e65be6-c3f3-45d1-8be3-edb352151008
https://crash-stats.mozilla.com/report/index/db7e3b18-d183-48f8-8b51-f271c2151008
https://crash-stats.mozilla.com/report/index/cebdf4f2-f4ac-44fa-8407-f907d2151008
Basically it's is reliably dying every time I start exploring by touch on the screen (best shown when doing that over a keyboard/keypad).
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(pcheng)
Comment 6•9 years ago
|
||
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)
Reporter | ||
Comment 7•9 years ago
|
||
(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)
Comment 8•9 years ago
|
||
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)
Reporter | ||
Comment 9•9 years ago
|
||
str |
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 10•9 years ago
|
||
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
Updated•9 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
Reporter | ||
Comment 11•9 years ago
|
||
Would it be possible to get a regression window on aries for this one.
Keywords: regressionwindow-wanted
Comment 12•9 years ago
|
||
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
Comment 13•9 years ago
|
||
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)
Updated•9 years ago
|
QA Contact: pcheng
Comment 14•9 years ago
|
||
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
Updated•9 years ago
|
QA Whiteboard: [QAnalyst-Triage?]
Keywords: crash,
reproducible
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
(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)
Assignee | ||
Comment 17•9 years ago
|
||
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.
Assignee | ||
Comment 18•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8675685 -
Attachment description: Part 1. Check whether the stream is destroyed on SetAudioOutputVolume. → Part 2. Don't release MediaStream until Destroy is called
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Comment 20•9 years ago
|
||
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)
Assignee | ||
Comment 21•9 years ago
|
||
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)
Reporter | ||
Comment 22•9 years ago
|
||
(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.
Reporter | ||
Comment 23•9 years ago
|
||
(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.
Comment hidden (obsolete) |
Comment 25•9 years ago
|
||
According to my testing at comment 10 this crash is NOT reproducible on Flame.
Comment 26•9 years ago
|
||
Note I reproduce bug 1214149 on Flame, so if this is a dupe the issue happens everywhere.
Updated•9 years ago
|
Component: DOM → Audio/Video: MSG/cubeb/GMP
Comment 27•9 years ago
|
||
Please have an MSG person look at this when it's ready for review.
Reporter | ||
Comment 28•9 years ago
|
||
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)
Reporter | ||
Comment 29•9 years ago
|
||
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)
Reporter | ||
Comment 30•9 years ago
|
||
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)
Comment 32•9 years ago
|
||
Andre, can you review these patches?
blocking-b2g: --- → 2.5+
Flags: needinfo?(anatal)
Comment 34•9 years ago
|
||
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)
Comment 35•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(m_kato)
Assignee | ||
Comment 36•9 years ago
|
||
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 37•9 years ago
|
||
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)
Assignee | ||
Comment 38•9 years ago
|
||
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+
Comment 41•9 years ago
|
||
Nice! I go to sleep (CEST), wake up, and there is a patch! I wish it was always that easy.
Thanks Makoto and Robert!
Assignee | ||
Comment 42•9 years ago
|
||
Assignee | ||
Comment 43•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8675686 -
Attachment is obsolete: true
Comment 44•9 years ago
|
||
Thank you Makoto for tackling that.
Updated•9 years ago
|
Flags: needinfo?(anatal)
Attachment #8677883 -
Flags: review?(roc) → review+
Comment 45•9 years ago
|
||
Comment 46•9 years ago
|
||
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.
Description
•