Closed Bug 1418766 Opened 3 years ago Closed 3 years ago

Crash in java.lang.OutOfMemoryError: at com.google.android.exoplayer2.upstream.DefaultAllocator.allocate(DefaultAllocator.java)

Categories

(Firefox for Android :: Audio/Video, defect, P1)

Product:
Firefox for Android
Component:
Audio/Video
Version:
Firefox 59
Platform:
Unspecified
Android
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: jseward, Assigned: JamesCheng)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default.
59 bytes, text/x-review-board-request
kikuo
: review+
Details 
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

=============================================================
Flags: needinfo?(snorp)
-> bwu
Flags: needinfo?(snorp) → needinfo?(bwu)
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
Flags: needinfo?(jacheng)
OK, I will look into it.
Flags: needinfo?(jacheng)
Blocks: HLS_on_Fennec
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: nobody → jacheng
Attachment #8930404 - Flags: review?(kikuo)
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 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.
(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 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-
Blocks: 1419669
(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 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+
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
https://hg.mozilla.org/mozilla-central/rev/c90c64a0adaf
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
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.
(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)
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.
(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.
(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.
The crash reports did not grow within this week. Hope patch fixed the issue entirely.
(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)
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).
Blocks: 1424168
You need to log in before you can comment on or make changes to this bug.