Closed
Bug 1418766
Opened 5 years ago
Closed 5 years ago
Crash in java.lang.OutOfMemoryError: at com.google.android.exoplayer2.upstream.DefaultAllocator.allocate(DefaultAllocator.java)
Categories
(Firefox for Android Graveyard :: Audio/Video, defect, P1)
Tracking
(firefox57 unaffected, firefox58 unaffected, firefox59 fixed)
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox57 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | fixed |
People
(Reporter: jseward, Assigned: JamesCheng)
References
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
This bug was filed from the Socorro interface and is report bp-f291ef59-b9c4-4166-925e-f34070171118. This is #3 in the Android nightly 20171116100050. ============================================================= Top 10 frames of crashing thread: 0 libxul.so NS_ABORT_OOM xpcom/base/nsDebugImpl.cpp:620 1 libxul.so mozilla::jni::ReportException widget/android/jni/Utils.cpp:225 2 libxul.so GeckoAppShellSupport::ReportJavaCrash widget/android/nsAppShell.cpp:272 3 libxul.so mozilla::jni::NativeStub<mozilla::java::GeckoAppShell::ReportJavaCrash_t, GeckoAppShellSupport, mozilla::jni::Args<const mozilla::jni::Ref<mozilla::jni::TypedObject<_jthrowable*>, _jthrowable*>&, const mozilla::jni::StringParam&> >::Wrap<&GeckoAppShellSupport::ReportJavaCrash> widget/android/jni/Natives.h:778 4 libdvm.so libdvm.so@0x1e74e 5 dalvik-heap (deleted) dalvik-heap @0x2e0b6 6 dalvik-heap (deleted) dalvik-heap @0x6cacbe 7 libdvm.so libdvm.so@0x4fa27 8 data@app@org.mozilla.fennec_aurora-2.apk@classes.dex data@app@org.mozilla.fennec_aurora-2.apk@classes.dex@0x5ad202 9 libxul.so mozilla::jni::NativeStub<mozilla::java::GeckoAppShell::OnSensorChanged_t, GeckoAppShellSupport, mozilla::jni::Args<int, float, float, float, float, int, long long> >::Wrap<&GeckoAppShellSupport::OnSensorChanged> widget/android/jni/Natives.h =============================================================
Reporter | ||
Updated•5 years ago
|
Flags: needinfo?(snorp)
-> bwu
Flags: needinfo?(snorp) → needinfo?(bwu)
Comment 2•5 years ago
|
||
James, This is top#3 crash on nightly. Can you have a look?
Component: Widget: Android → Audio/Video
Flags: needinfo?(bwu)
Priority: -- → P1
Product: Core → Firefox for Android
Target Milestone: --- → Firefox 59
Version: unspecified → Firefox 59
Updated•5 years ago
|
Flags: needinfo?(jacheng)
Assignee | ||
Updated•5 years ago
|
Blocks: HLS_on_Fennec
Assignee | ||
Comment 4•5 years ago
|
||
Some site like http://mashable.com/2017/11/15/justice-league-rotten-tomatoes-score-reviews-fresh-number/?utm_cid=mash-com-fb-ent-link#LSAAnxyHYuqz Set HTMLMediaElement with preload="auto" without playing it in the beginning and we didn't handle this case. We made Exoplayer keep downloading chunks until OOM(If the duration of the content is really long, it will cause OOM). I will test it on my test page. [preload="auto" without playing] https://jameswccheng.github.io/hlslocaltest/test.html [autoplay="true"] https://jameswccheng.github.io/hlslocaltest/test_autoplay.html to make sure the patch is not hitting any regression.
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → jacheng
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8930404 -
Flags: review?(kikuo)
Comment 6•5 years ago
|
||
mozreview-review |
Comment on attachment 8930404 [details] Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default. https://reviewboard.mozilla.org/r/201566/#review206804 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:64 (Diff revision 1) > * the same GeckoHlsPlayer. > * mPlayerId is a token used for Gecko media pipeline to obtain corresponding player. > */ > private final int mPlayerId; > private boolean mExoplayerSuspended = false; > - private boolean mMediaElementSuspended = false; > + private boolean mMediaElementSuspended = true; Please elaborate why this value is true by default, is the mediaelement suspended ? If the intention is to make play() work, an enum for that may be better. e.g. enum MediaElementSuspendState{ UNKNOWN, SUSPENDED, UNSUSPENDED };
Attachment #8930404 -
Flags: review?(kikuo) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•5 years ago
|
||
mozreview-review |
Comment on attachment 8930404 [details] Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default. https://reviewboard.mozilla.org/r/201566/#review206816 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:64 (Diff revision 1) > * the same GeckoHlsPlayer. > * mPlayerId is a token used for Gecko media pipeline to obtain corresponding player. > */ > private final int mPlayerId; > private boolean mExoplayerSuspended = false; > - private boolean mMediaElementSuspended = false; > + private boolean mMediaElementSuspended = true; I will still use the boolean since the state would only be two condition and the variable name is self-explanatory. And I added the comment to tell why the default value keeps true. The original logic is wrong, it will make ``onPlayerStateChanged`` calls resumeExoplayer and the exoplayer will always start working.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•5 years ago
|
||
(In reply to Kilik Kuo [:kikuo] from comment #6) > Comment on attachment 8930404 [details] > Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer > pause by default. > > https://reviewboard.mozilla.org/r/201566/#review206804 > > ::: > mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/ > GeckoHlsPlayer.java:64 > (Diff revision 1) > > * the same GeckoHlsPlayer. > > * mPlayerId is a token used for Gecko media pipeline to obtain corresponding player. > > */ > > private final int mPlayerId; > > private boolean mExoplayerSuspended = false; > > - private boolean mMediaElementSuspended = false; > > + private boolean mMediaElementSuspended = true; > > Please elaborate why this value is true by default, > is the mediaelement suspended ? > > If the intention is to make play() work, an enum for that may be better. > > e.g. > enum MediaElementSuspendState{ > UNKNOWN, > SUSPENDED, > UNSUSPENDED > }; Respect. I obey and I've verified the patch with all the sites shown on comment 4. Please review, thanks.
Comment 11•5 years ago
|
||
mozreview-review |
Comment on attachment 8930404 [details] Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default. https://reviewboard.mozilla.org/r/201566/#review207216 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:65 (Diff revision 3) > * mPlayerId is a token used for Gecko media pipeline to obtain corresponding player. > */ > private final int mPlayerId; > private boolean mExoplayerSuspended = false; > - private boolean mMediaElementSuspended = false; > + > + private enum MediaElementSuspendState{ On second thought, I think the role of GeckoHlsPlayer is much like MediaDecoder in Gecko's media stack. I'd prefer to renamed the enum state to MediaDecoderPlayState and not to using *Suspend*-related words. ExoPlayer has states i.e. 'preparing'/'idel'/'ready'/'buffering'/'ended' MediaDecoder has states i.e. [1] Please try combine these states for GeckoHlsPlayer. [1] https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/dom/media/MediaDecoder.h#93-101 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:778 (Diff revision 3) > - if (!mMediaElementSuspended) { > + if (mMediaElementSuspendState == MediaElementSuspendState.UNSUSPENDED) { > return; > } > if (DEBUG) { Log.d(LOGTAG, "mediaElement played."); } > - mMediaElementSuspended = false; > + mMediaElementSuspendState = MediaElementSuspendState.UNSUSPENDED; > resumeExoplayer(); I'd suggest filing a follow-up to refine the logic about pausing/resuming exoplayer by provide 2 functions, e.g. ShouldPauseExoPlayer(), ShouldResumeExoPlayer(). Then assertion could be added in play()/seek()/resume()/pause()/etc... to ensure that we don't want those methods be called in unexpected states. In that case, we don't need this tricky 'returns'
Attachment #8930404 -
Flags: review?(kikuo) → review-
Assignee | ||
Comment 12•5 years ago
|
||
(In reply to Kilik Kuo [:kikuo] from comment #11) > Comment on attachment 8930404 [details] > Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer > pause by default. > > https://reviewboard.mozilla.org/r/201566/#review207216 > > ::: > mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/ > GeckoHlsPlayer.java:65 > (Diff revision 3) > > * mPlayerId is a token used for Gecko media pipeline to obtain corresponding player. > > */ > > private final int mPlayerId; > > private boolean mExoplayerSuspended = false; > > - private boolean mMediaElementSuspended = false; > > + > > + private enum MediaElementSuspendState{ > > On second thought, I think the role of GeckoHlsPlayer is much like > MediaDecoder in Gecko's media stack. I'd prefer to renamed the enum state > to MediaDecoderPlayState and not to using *Suspend*-related words. > > ExoPlayer has states i.e. 'preparing'/'idel'/'ready'/'buffering'/'ended' > MediaDecoder has states i.e. [1] > > Please try combine these states for GeckoHlsPlayer. > > Thanks. > [1] > https://searchfox.org/mozilla-central/rev/ > 797c93d81fe446f78babf20894f0729f15f71ee6/dom/media/MediaDecoder.h#93-101 > > ::: > mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/ > GeckoHlsPlayer.java:778 > (Diff revision 3) > > - if (!mMediaElementSuspended) { > > + if (mMediaElementSuspendState == MediaElementSuspendState.UNSUSPENDED) { > > return; > > } > > if (DEBUG) { Log.d(LOGTAG, "mediaElement played."); } > > - mMediaElementSuspended = false; > > + mMediaElementSuspendState = MediaElementSuspendState.UNSUSPENDED; > > resumeExoplayer(); > > I'd suggest filing a follow-up to refine the logic about pausing/resuming > exoplayer by provide 2 functions, e.g. > ShouldPauseExoPlayer(), ShouldResumeExoPlayer(). > > Then assertion could be added in play()/seek()/resume()/pause()/etc... to > ensure that we don't want those methods be called in unexpected states. In > that case, we don't need this tricky 'returns' Follow-up bug 1419669. Thank you.
Comment hidden (mozreview-request) |
Comment 14•5 years ago
|
||
mozreview-review |
Comment on attachment 8930404 [details] Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default. https://reviewboard.mozilla.org/r/201566/#review207364 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:781 (Diff revision 4) > @Override > public synchronized void play() { > - if (!mMediaElementSuspended) { > + if (mMediaDecoderPlayState == MediaDecoderPlayState.PLAY_STATE_PLAYING) { > return; > } > if (DEBUG) { Log.d(LOGTAG, "mediaElement played."); } s/mediaElement/MediaDecoder/ ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:792 (Diff revision 4) > @Override > public synchronized void pause() { > - if (mMediaElementSuspended) { > + if (mMediaDecoderPlayState != MediaDecoderPlayState.PLAY_STATE_PLAYING) { > return; > } > if (DEBUG) { Log.d(LOGTAG, "mediaElement paused."); } ditto
Attachment #8930404 -
Flags: review?(kikuo) → review+
Comment hidden (mozreview-request) |
Comment 16•5 years ago
|
||
Pushed by jacheng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c90c64a0adaf Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default. r=kikuo
Comment 17•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c90c64a0adaf
Comment 18•5 years ago
|
||
I encountered this crash today, on Nightly 59 (2017-11-27), while a twitch.tv live stream was blocked and kept loading. So the crash seems to not be fixed.
Assignee | ||
Comment 19•5 years ago
|
||
(In reply to Oana Horvath from comment #18) > I encountered this crash today, on Nightly 59 (2017-11-27), while a > twitch.tv live stream was blocked and kept loading. So the crash seems to > not be fixed. Hi Oana, I wanna make sure something, please help me to clarify. 1. Do you use Google Pixel for testing? 2. Do you test HLS functionality? That means you disable MSE by pref off "media.mediasource.enabled"? 3. Could you please elaborate more how you met this crash? Play random twitch.tv without pausing the playback? "while a twitch.tv live stream was blocked and kept loading" and then crash? 4. If it is easy to reproduce, would you please provide the adb logcat for me? Thank you.
Flags: needinfo?(oana.horvath)
Updated•5 years ago
|
status-firefox57:
--- → unaffected
status-firefox58:
--- → unaffected
Assignee | ||
Comment 20•5 years ago
|
||
The patch of this bug did solve a OOM case due to the preload="auto" MediaElement. It can reduce the crash rate immensely. However, by comment 18, it seems the OOM may happen in some situation that we cannot easily reproduce or think of. I will keep tracking this crash signature to see if I can find any clue.
Comment 21•5 years ago
|
||
(In reply to James Cheng[:JamesCheng] from comment #20) > The patch of this bug did solve a OOM case due to the preload="auto" > MediaElement. > > It can reduce the crash rate immensely. > > However, by comment 18, it seems the OOM may happen in some situation that > we cannot easily reproduce or think of. > > I will keep tracking this crash signature to see if I can find any clue. IIRC, Fennec is using MSE on twitch.tv if the pref is not modified.
Assignee | ||
Comment 22•5 years ago
|
||
(In reply to Kilik Kuo [:kikuo] from comment #21) > (In reply to James Cheng[:JamesCheng] from comment #20) > > The patch of this bug did solve a OOM case due to the preload="auto" > > MediaElement. > > > > It can reduce the crash rate immensely. > > > > However, by comment 18, it seems the OOM may happen in some situation that > > we cannot easily reproduce or think of. > > > > I will keep tracking this crash signature to see if I can find any clue. > > IIRC, Fennec is using MSE on twitch.tv if the pref is not modified. Yes, so I ni? Oana in comment 19 for making sure she is testing HLS not MSE.
Assignee | ||
Comment 23•5 years ago
|
||
The crash reports did not grow within this week. Hope patch fixed the issue entirely.
Comment 24•5 years ago
|
||
(In reply to James Cheng[:JamesCheng] from comment #19) > (In reply to Oana Horvath from comment #18) > > I encountered this crash today, on Nightly 59 (2017-11-27), while a > > twitch.tv live stream was blocked and kept loading. So the crash seems to > > not be fixed. > > Hi Oana, > > I wanna make sure something, please help me to clarify. > > 1. Do you use Google Pixel for testing? > > 2. Do you test HLS functionality? That means you disable MSE by pref off > "media.mediasource.enabled"? > > 3. Could you please elaborate more how you met this crash? > Play random twitch.tv without pausing the playback? > "while a twitch.tv live stream was blocked and kept loading" and then > crash? > > 4. If it is easy to reproduce, would you please provide the adb logcat for > me? > > Thank you. Sorry for the long wait, I was OOO until today. The answer is Yes to your questions: Google Pixel with Android 8.0, with MSE pref off. I've tried to get a logcat, but I can't reproduce it now. I will try again and post an update as soon as I can repro (hopefully not so soon).
Flags: needinfo?(oana.horvath)
Assignee | ||
Comment 25•5 years ago
|
||
See the crash reports again by today, After landing the patch, it immensely reduced the crash rate. So as comment 20 said, we would like Oana to provide some reproducible way or logcat that we can look into it easier. Maybe you provide the site you watched first and I can try to reproduce as well. (Maybe the script in that site has some unexpected behavior).
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•