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

Status: RESOLVED FIXED

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

Description

[Julian Seward [:jseward]]

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@[e-mail][e-mail] data@[e-mail][e-mail]@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

=============================================================

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

-> bwu

Comment 2

[Blake Wu [:bwu][:blakewu]]

James,
This is top#3 crash on nightly. Can you have a look?

Comment 3

[James Cheng[:JamesCheng]]

OK, I will look into it.

Comment 4

[James Cheng[:JamesCheng]]

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.

Comment 5

[James Cheng[:JamesCheng]]

Attachment: Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default.

Review commit: https://reviewboard.mozilla.org/r/201566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201566/

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

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
};

Comment 7

[James Cheng[:JamesCheng]]

Comment on attachment 8930404 [details]
Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201566/diff/1-2/

Comment 8

[James Cheng[:JamesCheng]]

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 9

[James Cheng[:JamesCheng]]

Comment on attachment 8930404 [details]
Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201566/diff/2-3/

Comment 10

[James Cheng[:JamesCheng]]

(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

[Kilik Kuo [:kikuo] (inactive)]

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'

Comment 12

[James Cheng[:JamesCheng]]

(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 13

[James Cheng[:JamesCheng]]

Comment on attachment 8930404 [details]
Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201566/diff/3-4/

Comment 14

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 15

[James Cheng[:JamesCheng]]

Comment on attachment 8930404 [details]
Bug 1418766 - Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201566/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c90c64a0adaf
Fix Crash in java.lang.OutOfMemoryError by making Exoplayer pause by default. r=kikuo

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c90c64a0adaf

Comment 18

[Oana Horvath]

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.

Comment 19

[James Cheng[:JamesCheng]]

(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.

Comment 20

[James Cheng[:JamesCheng]]

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

[Kilik Kuo [:kikuo] (inactive)]

(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.

Comment 22

[James Cheng[:JamesCheng]]

(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.

Comment 23

[James Cheng[:JamesCheng]]

The crash reports did not grow within this week. Hope patch fixed the issue entirely.

Comment 24

[Oana Horvath]

(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).

Comment 25

[James Cheng[:JamesCheng]]

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).