Closed Bug 1384578 Opened 7 years ago Closed 7 years ago

Crash in java.lang.NullPointerException: Null native pointer at org.mozilla.gecko.media.GeckoHLSDemuxerWrapper$Callbacks.onError(Native Method)

Categories

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

56 Branch
Unspecified
Android
defect
Not set
critical

Tracking

(firefox54 unaffected, firefox55 unaffected, firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: calixte, Assigned: JamesCheng)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-18d49337-7eda-47fb-b9b0-720340170726.
=============================================================

There are 2 crashes in nightly 56 with buildid 20170719100212 and 20170724100320.
:JamesCheng, could you investigate please ?
Flags: needinfo?(jacheng)
Thanks for reporting, investigating!
Assignee: nobody → jacheng
Flags: needinfo?(jacheng)
My guess is 

There is a race condition between
[1]
http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/media/hls/HLSDemuxer.cpp#255-263

and 
[2]
http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java#353-357

So [2] might access the callback which has been destroyed by [1].

I need to find a synchronization approach between java and cpp.

Hi Jim,

May I have your opinions that is there any existing code handling the synchronization between java and cpp?

I can only think of exposing some @WrapForJNI APIs from java like 'Lock() / Unlock()' and use it in C++ side...

Thanks.
Flags: needinfo?(nchen)
As the assumption in comment 2, [1] and [2] are racy.

I think the patch is a simple way to make them mutual exclusive.


synchronized void GeckoPlayer.onPlayerError

and 

public synchronized void GeckoPlayer.release()(which is called by mHLSDemuxerWrapper->Destroy())

is mutual exclusive by 'synchronized'


So the only thing we need to adjust is that

making 

HLSDemuxerCallbacksSupport::DisposeNative(mJavaCallbacks) be the last call to avoid this calling sequence

1. HLSDemuxerCallbacksSupport::DisposeNative(mJavaCallbacks)
2. GeckoPlayer.onPlayerError
3. GeckoPlayer.release()

always make it 


1, 2. GeckoPlayer.onPlayerError | GeckoPlayer.release()
3. HLSDemuxerCallbacksSupport::DisposeNative(mJavaCallbacks)



[1]
http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/media/hls/HLSDemuxer.cpp#255-263

and 
[2]
http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java#353-357
Attachment #8891872 - Flags: review?(jolin)
Comment on attachment 8891872 [details]
Bug 1384578 - Adjust the calling sequence to avoid app crash by race condition.

https://reviewboard.mozilla.org/r/162890/#review168210
Attachment #8891872 - Flags: review?(jolin) → review+
Thank you for review!
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/116f553ba4cd
Adjust the calling sequence to avoid app crash by race condition. r=jolin
https://hg.mozilla.org/mozilla-central/rev/116f553ba4cd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You can lock Java objects in C++ through `jni::Ref::Lock`, e.g.,

> {
>     auto lock = mJavaObject.Lock();
>     // foo
> }

is similar to this Java code,

> synchronized (mJavaObject) {
>     // foo
> }

But your solution looks good!
Flags: needinfo?(nchen)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.