Last Comment Bug 705112 - crash JNI_CreateJavaVM | sa_stream_destroy
: crash JNI_CreateJavaVM | sa_stream_destroy
: crash, regression, reproducible, topcrash
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: 10 Branch
: Other Android
: -- critical (vote)
: mozilla11
Assigned To: Matthew Gregan [:kinetik]
: Maire Reavy [:mreavy]
Depends on:
  Show dependency treegraph
Reported: 2011-11-24 05:16 PST by Bill Gianopoulos [:WG9s]
Modified: 2012-04-11 20:34 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v0 (3.54 KB, patch)
2011-11-24 11:40 PST, Matthew Gregan [:kinetik]
doug.turner: review+
Details | Diff | Splinter Review
esr patch v0 (3.19 KB, patch)
2012-04-06 17:03 PDT, Matthew Gregan [:kinetik]
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Bill Gianopoulos [:WG9s] 2011-11-24 05:16:10 PST
This bug was filed from the Socorro interface and is 
report bp-23e33a0e-f29f-418b-a33f-fb7962111124 .
Comment 1 Bill Gianopoulos [:WG9s] 2011-11-24 05:19:51 PST
Trying to play the OGG file in this attachment from bug 704450 crashes on a Samsung Galaxy Tab 8.9.
Comment 2 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-11-24 06:28:29 PST
Frame 	Module 	Signature [Expand] 	Source
0 	JNI_CreateJavaVM 	
1 	sa_stream_destroy 	media/libsydneyaudio/src/sydney_audio_android.c:280
2 	nsNativeAudioStream::Init 	content/media/nsAudioStream.cpp:424
3 	mozilla::dom::AudioParent::AudioParent 	dom/ipc/AudioParent.cpp:303
4 	mozilla::dom::ContentParent::AllocPAudio 	dom/ipc/ContentParent.cpp:876
5 	mozilla::dom::PContentParent::OnMessageReceived 	obj-firefox/ipc/ipdl/PContentParent.cpp:851
6 	mozilla::ipc::AsyncChannel::OnDispatchMessage 	ipc/glue/AsyncChannel.cpp:294
7 	mozilla::ipc::RPCChannel::OnMaybeDequeueOne 	ipc/glue/RPCChannel.cpp:433
8 	RunnableMethod<mozilla::ipc::RPCChannel, bool , Tuple0>::Run 	ipc/chromium/src/base/tuple.h:383
9 	mozilla::ipc::RPCChannel::DequeueTask::Run 	RPCChannel.h:464
10 	MessageLoop::RunTask 	ipc/chromium/src/base/
11 	MessageLoop::DeferOrRunPendingTask 	ipc/chromium/src/base/
12 	MessageLoop::DoWork 	ipc/chromium/src/base/
13 	mozilla::ipc::DoWorkRunnable::Run 	ipc/glue/MessagePump.cpp:70
14 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
15 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:245
16 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
17 	MessageLoop::RunInternal 	ipc/chromium/src/base/
18 	MessageLoop::Run 	ipc/chromium/src/base/
19 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
20 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:228
21 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3551
22 	Java_org_mozilla_gecko_GeckoAppShell_nativeRun 	toolkit/xre/nsAndroidStartup.cpp:132
23 	Java_org_mozilla_gecko_GeckoAppShell_nativeRun 	other-licenses/android/APKOpen.cpp:232
24 	dvmPlatformInvoke 	
25 	dvmCallJNIMethod_general 	
26 	dvmResolveNativeMethod 	
27 	dvmAsmSisterStart 	
28 	dvmMterpStd 	
29 	dvmInterpret 	
30 	dvmCallMethodV 	
31 	dvmCallMethod 	
32 	dvmAttachCurrentThread 	
33 	__thread_entry 	
34 	pthread_create 	
35 		@0x5e8301ae
Comment 3 Bill Gianopoulos [:WG9s] 2011-11-24 07:12:13 PST
That audio file has a non-standard sample-rate.

Using this file newly attached results in a file that plays without crashing:

Perhaps for Android we should have a list of valid sample rates and not try to play things not in the list?
Comment 4 Bill Gianopoulos [:WG9s] 2011-11-24 07:41:39 PST
Adding regression Keyword and requesting tracking Firefox 10 because this does not crash in the current Firefox version 9 beta.
Comment 5 Matthew Gregan [:kinetik] 2011-11-24 11:40:02 PST
Created attachment 576810 [details] [diff] [review]
Comment 6 Doug Turner (:dougt) 2011-11-25 00:28:17 PST
Comment on attachment 576810 [details] [diff] [review]

Review of attachment 576810 [details] [diff] [review]:

::: media/libsydneyaudio/src/sydney_audio_android.c
@@ +119,4 @@
>  static jclass
>  init_jni_bindings(JNIEnv *jenv) {
> +  jclass class = (*jenv)->FindClass(jenv, "android/media/AudioTrack");

I am kinda of surprised that this returns null.
Comment 7 Doug Turner (:dougt) 2011-11-25 00:28:47 PST
land on m-c, we can merge back to birch whenever.
Comment 8 Bill Gianopoulos [:WG9s] 2011-11-29 09:08:31 PST
Using my own build including this patch, I am unable to reproduce the crash.
Comment 9 Matthew Gregan [:kinetik] 2011-11-29 20:51:21 PST
Comment 10 Marco Bonardo [::mak] 2011-12-01 03:56:09 PST
Comment 11 Alex Keybl [:akeybl] 2012-01-09 11:23:25 PST
Setting tracking flag for 10 to - since crash-stats does not suggest that this is occurring in FF10beta2.
Comment 12 Scoobidiver (away) 2012-03-28 04:47:04 PDT
It's #9 top browser crasher in Fennec 10.0.3esr.
Comment 13 Alex Keybl [:akeybl] 2012-03-29 17:33:15 PDT
Matthew - how do you feel about the risk to the ESR10 branch (desktop and mobile) if we try to fix on that branch? Thanks in advance.
Comment 14 Matthew Gregan [:kinetik] 2012-03-29 17:59:18 PDT
The code is only used on Android, so the risk is limited to that platform.  It's a straightforward fix, so I'd rate the risk on Android as very low.
Comment 15 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-02 16:44:43 PDT
[Triage Comment]
Please go ahead and prepare/nominate a patch for ESR branch, see
Comment 16 Matthew Gregan [:kinetik] 2012-04-06 17:03:01 PDT
Created attachment 613028 [details] [diff] [review]
esr patch v0

Slightly rebased due to missing patches on ESR.
Comment 17 Matthew Gregan [:kinetik] 2012-04-06 17:04:39 PDT
ESR try push:
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-09 14:47:14 PDT
(In reply to Matthew Gregan [:kinetik] from comment #17)
> ESR try push:

So that push is entirely red - have you another? :)
Comment 19 Matthew Gregan [:kinetik] 2012-04-09 16:08:04 PDT
Building Android ESR on try seems to require additional magic.  Let's see if this one works:
Comment 20 Matthew Gregan [:kinetik] 2012-04-09 19:50:02 PDT
(In reply to Matthew Gregan [:kinetik] from comment #19)

Builds are green.  I think that's good enough--the only difference in this patch compared to the original patch is removal of a few lines freeing resources that aren't allocated in the earlier versions of the code--the changes have effectively been tested by the original patch landing.

I don't know what's up with the tests in this try push, they're all failing to even start due to a config issue/difference on try vs the ESR build config.
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-11 16:16:09 PDT
Comment on attachment 613028 [details] [diff] [review]
esr patch v0

Thanks for preparing this.
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-11 16:17:42 PDT
(In reply to Matthew Gregan [:kinetik] from comment #20)

> I don't know what's up with the tests in this try push, they're all failing
> to even start due to a config issue/difference on try vs the ESR build
> config.

When you land on ESR can you please confirm back in there that the test runs on ESR branch don't mimic what we're seeing on the try push?
Comment 23 Matthew Gregan [:kinetik] 2012-04-11 18:46:55 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #22)
> When you land on ESR can you please confirm back in there that the test runs
> on ESR branch don't mimic what we're seeing on the try push?

Will do.
Comment 24 Matthew Gregan [:kinetik] 2012-04-11 20:34:29 PDT
(In reply to Matthew Gregan [:kinetik] from comment #23)

Android tests ran fine (other than a few existing intermittent failures), so this looks good.

Note You need to log in before you can comment on or make changes to this bug.