opentok / opentok-android-sdk-samples

Sample applications illustrating best practices using OpenTok Android SDK.
https://tokbox.com/developer/sdks/android/
MIT License
212 stars 169 forks source link

java.lang.NullPointerException: Attempt to invoke virtual method 'void android.os.Handler.removeCallbacks(java.lang.Runnable)' on a null object reference #476

Closed derekpitts28 closed 9 months ago

derekpitts28 commented 1 year ago

Describe the bug This is happening on production for us also. Device: Pixel 6 Android 13 Vonage SDK: 2.26

We see this occur when closing a previous session and creating a new close together. There seems to be no 'good' or recommended way to close a session other than nulling its reference and hoping that the garbage collector collects. That being said we are trying to use the life cycle methods to gracefully close the vonage session.

The main issue seems to be that vonage is attempting to remove something that does not exist or is already stopped.

The fact that I have to jump though so many hoops just to dispose and ensure vonage is not controlling the camera anymore is a bit crazy to me.

FATAL EXCEPTION: main
Process: com.transcarent.app, PID: 550
java.lang.NullPointerException: Attempt to invoke virtual method 'void android.os.Handler.removeCallbacks(java.lang.Runnable)' on a null object reference
    at com.opentok.android.Camera2VideoCapturer.stopDisplayOrientationCache(Unknown Source:4)
    at com.opentok.android.Camera2VideoCapturer.destroy(Unknown Source:11)
    at com.opentok.android.PublisherKit.a(Unknown Source:4)
    at com.opentok.android.PublisherKit.z(Unknown Source:0)
    at com.opentok.android.m0.run(Unknown Source:2)
    at android.os.Handler.handleCallback(Handler.java:942)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at android.os.Looper.loopOnce(Looper.java:201)
    at android.os.Looper.loop(Looper.java:288)
    at android.app.ActivityThread.main(ActivityThread.java:7918)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)

It should be noted we have also tried to go directly to the capture of the publisher and stop that however stopping that results in a separate error.

Please give us a guaranteed way to COMPLETELY dispose of a session/publisher this is so frustrating.

I believe this error seems to be due to the SDK expecting a callback but because i removed it it is getting null. Is your SDK nullsafe on the java side when running callbacks that can be nulled?

To Reproduce If you run this code to close the previous sessions references x3-4 times you will run into the error i got.

// Fully connect to a session
// Leave. session

patientPublisher?.setPublisherListener(null)
patientPublisher?.setMuteListener(null)
patientPublisher?.setCameraListener(null)
patientPublisher?.setVideoListener(null)

session?.onPause()
patientPublisher?.onStop()

Expected behavior The session is closed without throwing errors.

Device (please compete the following information): Device: Pixel 6 Android 13 Vonage SDK: 2.26

Additional context Add any other context about the problem here.

goncalocostamendes commented 1 year ago

@derekpitts28

In a soon to be released v2.26.1 version (between this and the following week), we currectly terminate the camera capturer for all scenarios.

When combining the 2.26.1 version together with the correct way to gracefully terminate a session:

mPublisher.onStop();
mSession.unpublish(mPublisher);
mSession.disconnect();

I attached a video recording to the ticket reproducing the scenario with the new fixes: https://github.com/opentok/opentok-android-sdk-samples/assets/106695857/386057f6-36af-4fee-adbd-85b1282d8d78

Also, in regards to the object life-cycle concerns, I do not recommend nullifying anything. Resources will be automatically released by the garbage collector and all our native resources are released explicitly by the SDK (we override the finalize() method in order to do so).

I will close the ticket. If for some reason you find these changes do not solve your problem, please re-open it.

derekpitts28 commented 1 year ago

Thanks so much @goncalocostamendes !! Do you by chance have an ETA for 2.26.1?

goncalocostamendes commented 1 year ago

@v-kpheng can you please give a feedback to @derekpitts28?

v-kpheng commented 1 year ago

@derekpitts28, 2.26.1 should be out by mid-October.

derekpitts28 commented 1 year ago

@goncalocostamendes After we updated to the new 2.26.1 and using the closing code you mentioned above, our testers still found this issue on android Xiaomi API 33. I am wondering if this is a race condition because on average i am not reproducing this on my Pixel 6 but we are seeing it consistently on other devices.

Could we surround these calls with a try catch or add in null safe calls so we don't see stale calls call this? This is completely bricking the app when it happens.

Here is the error again:

digest=============com.transcarent.appjava.lang.NullPointerException: Attempt to invoke virtual method 'void android.os.Handler.removeCallbacks(java.lang.Runnable)' on a null object reference
at com.opentok.android.CameraVideoCapturer.stopDisplayOrientationCache(SourceFile:)
at com.opentok.android.CameraVideoCapturer.destroy(SourceFile:XX)
at com.opentok.android.PublisherKit.a(SourceFile:)
at com.opentok.android.PublisherKit.z(SourceFile:)
at com.opentok.android.q.run(R$$SyntheticClass:)
at android.os.Handler.handleCallback(Handler.java:XX)
at android.os.Handler.dispatchMessage(Handler.java:XX)
at android.os.Looper.loopOnce(Looper.java:XX)
at android.os.Looper.loop(Looper.java:XX)
at android.app.ActivityThread.main(ActivityThread.java:XX)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:XX)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:XX)
derekpitts28 commented 1 year ago

I believe you said i can reopen this however i do not think i have the ability to do so, please let me know how you want me to proceed when you have a chance, thanks!

goncalocostamendes commented 1 year ago

Reopening issue @derekpitts28

derekpitts28 commented 12 months ago

Any movement on this? @goncalocostamendes @v-kpheng

derekpitts28 commented 11 months ago

@goncalocostamendes @v-kpheng Any way I can get this looked at, we have users crashing because of this, thanks!

goncalocostamendes commented 11 months ago

@derekpitts28 I can now have access to a mobile device with the same model. This bug will be worked on next sprint. I will provide you an update once I have one.

Please provide me the entire content of the function you call to end the session.

derekpitts28 commented 11 months ago

Here is the dispose call:

    private fun dispose() {
        try {
            patientPublisher?.onStop()
        } catch (e: Exception) {
            VonagePlugin.logInfo("Dispose session .onStop() error $e")
        }

        try {
            session?.unpublish(patientPublisher)
        } catch (e: Exception) {
            VonagePlugin.logInfo("Dispose session .unpublish() error $e")
        }
        try {
            session?.disconnect()
        } catch (e: Exception) {
            VonagePlugin.logInfo("Dispose session .disconnect() error $e")
        }

        doctorVideoView?.removeVideoFromContainer()
        patientVideoView?.removeVideoFromContainer()

        patientPublisher = null
        doctorSubscriber = null
        session = null
    }
goncalocostamendes commented 11 months ago

@derekpitts28 I see the issue. There might indeed occur a race on the disposal of the session together with the publisher.

Session.disconnect() is configured to disconnect and delete internally any publisher(s) and subscriber(s).

I would recommend you changing the dispose method to:

private fun dispose() {
        try {
            session?.disconnect()
        } catch (e: Exception) {
            VonagePlugin.logInfo("Dispose session .disconnect() error $e")
        }

        doctorVideoView?.removeVideoFromContainer()
        patientVideoView?.removeVideoFromContainer()

    }

As previously mentioned, Session.disconnect() will delete the publisher, which will trigger the callback onStreamDestroyed(). On that callback, the capturer can be safely stopped.

public void onStreamDestroyed(PublisherKit publisher, Stream stream) {
    try {
            patientPublisher?.onStop()
        } catch (e: Exception) {
            VonagePlugin.logInfo("Dispose session .onStop() error $e")
        }
}

Fell free to test it and give me feedback on it.

Furthermore, I am adding a safety check on our SDK which will be released in 2.27.1 (mid January).

derekpitts28 commented 10 months ago

Thanks, let me implement what you mentioned above and see if that fixes this on the devices that were crashing. I will get back to you once I have a result. Thanks again! 🚀

tinder-enginrumelioglu commented 10 months ago

@goncalocostamendes We are running into the same issue on our code base. Following your previous suggestion of not using onStop on the publisher seems to resolve the issue however I have some concerns. We create a publisher before creating a session because we want to give the user a preview of the video before joining the session. This sometimes leads to cases where a session is never created because the user decides to leave the activity without joining the chat, and our code is setup to call publisher.onStop since there is no session to call disconnect on. Do you see any issues with removing this onStop call (my immediate concern would be memory leaks)? Or should we wait for the 2.27.1 release that will address this issue?

goncalocostamendes commented 10 months ago

@tinder-enginrumelioglu publisher.onStop can be triggered multiple times, so you can leave both.

derekpitts28 commented 10 months ago

@goncalocostamendes just an FYI for your suggestion for (2) above I cannot find a way to verify if onStopped was called it does not look like there is a public getter for isStopped as it is package private for the BaseVideoCapturer. If this is to be the recommended way of closing this, it would be great to have access to that state vs managing that ourselves.

goncalocostamendes commented 10 months ago

@tinder-enginrumelioglu @derekpitts28 publisher.onStop can be triggered multiple times. It internally validates if it was already stopped. As such, for the question:

@goncalocostamendes We are running into the same issue on our code base. Following your previous suggestion of not using onStop on the publisher seems to resolve the issue however I have some concerns. We create a publisher before creating a session because we want to give the user a preview of the video before joining the session. This sometimes leads to cases where a session is never created because the user decides to leave the activity without joining the chat, and our code is setup to call publisher.onStop since there is no session to call disconnect on. Do you see any issues with removing this onStop call (my immediate concern would be memory leaks)? Or should we wait for the 2.27.1 release that will address this issue?

You can leave both publisher.onStop calls.

goncalocostamendes commented 10 months ago

I will close the ticket since it seems the issue was resolved. Please open it if that is not the case.

derekpitts28 commented 10 months ago

@goncalocostamendes I don't think this was resolved unless you mean you have a fix for the later release. When trying to call onStop I get all sorts of errors unless you fixed those also. For example the situation where we want to keep the session open but stop the capturer, i call onStop and get a slew of errors:

W/MessageQueue(16897): Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897): java.lang.IllegalStateException: Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897):  at android.os.MessageQueue.enqueueMessage(MessageQueue.java:560)
W/MessageQueue(16897):  at android.os.Handler.enqueueMessage(Handler.java:786)
W/MessageQueue(16897):  at android.os.Handler.sendMessageAtTime(Handler.java:735)
W/MessageQueue(16897):  at android.os.Handler.sendMessageDelayed(Handler.java:705)
W/MessageQueue(16897):  at android.os.Handler.post(Handler.java:435)
W/MessageQueue(16897):  at android.media.ImageReader$HandlerExecutor.execute(ImageReader.java:1155)
W/MessageQueue(16897):  at android.media.ImageReader.postEventFromNative(ImageReader.java:944)
W/MessageQueue(16897): Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897): java.lang.IllegalStateException: Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897):  at android.os.MessageQueue.enqueueMessage(MessageQueue.java:560)
W/MessageQueue(16897):  at android.os.Handler.enqueueMessage(Handler.java:786)
W/MessageQueue(16897):  at android.os.Handler.sendMessageAtTime(Handler.java:735)
W/MessageQueue(16897):  at android.os.Handler.sendMessageDelayed(Handler.java:705)
W/MessageQueue(16897):  at android.os.Handler.post(Handler.java:435)
W/MessageQueue(16897):  at android.media.ImageReader$HandlerExecutor.execute(ImageReader.java:1155)
W/MessageQueue(16897):  at android.media.ImageReader.postEventFromNative(ImageReader.java:944)
W/MessageQueue(16897): Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897): java.lang.IllegalStateException: Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897):  at android.os.MessageQueue.enqueueMessage(MessageQueue.java:560)
W/MessageQueue(16897):  at android.os.Handler.enqueueMessage(Handler.java:786)
W/MessageQueue(16897):  at android.os.Handler.sendMessageAtTime(Handler.java:735)
W/MessageQueue(16897):  at android.os.Handler.sendMessageDelayed(Handler.java:705)
W/MessageQueue(16897):  at android.os.Handler.post(Handler.java:435)
W/MessageQueue(16897):  at android.media.ImageReader$HandlerExecutor.execute(ImageReader.java:1155)
W/MessageQueue(16897):  at android.media.ImageReader.postEventFromNative(ImageReader.java:944)
W/MessageQueue(16897): Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897): java.lang.IllegalStateException: Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897):  at android.os.MessageQueue.enqueueMessage(MessageQueue.java:560)
W/MessageQueue(16897):  at android.os.Handler.enqueueMessage(Handler.java:786)
W/MessageQueue(16897):  at android.os.Handler.sendMessageAtTime(Handler.java:735)
W/MessageQueue(16897):  at android.os.Handler.sendMessageDelayed(Handler.java:705)
W/MessageQueue(16897):  at android.os.Handler.post(Handler.java:435)
W/MessageQueue(16897):  at android.media.ImageReader$HandlerExecutor.execute(ImageReader.java:1155)
W/MessageQueue(16897):  at android.media.ImageReader.postEventFromNative(ImageReader.java:944)
W/MessageQueue(16897): Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897): java.lang.IllegalStateException: Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897):  at android.os.MessageQueue.enqueueMessage(MessageQueue.java:560)
W/MessageQueue(16897):  at android.os.Handler.enqueueMessage(Handler.java:786)
W/MessageQueue(16897):  at android.os.Handler.sendMessageAtTime(Handler.java:735)
W/MessageQueue(16897):  at android.os.Handler.sendMessageDelayed(Handler.java:705)
W/MessageQueue(16897):  at android.os.Handler.post(Handler.java:435)
W/MessageQueue(16897):  at android.media.ImageReader$HandlerExecutor.execute(ImageReader.java:1155)
W/MessageQueue(16897):  at android.media.ImageReader.postEventFromNative(ImageReader.java:944)
W/MessageQueue(16897): Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897): java.lang.IllegalStateException: Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread
W/MessageQueue(16897):  at android.os.MessageQueue.enqueueMessage(MessageQueue.java:560)
W/MessageQueue(16897):  at android.os.Handler.enqueueMessage(Handler.java:786)
W/MessageQueue(16897):  at android.os.Handler.sendMessageAtTime(Handler.java:735)
W/MessageQueue(16897):  at android.os.Handler.sendMessageDelayed(Handler.java:705)
W/MessageQueue(16897):  at android.os.Handler.post(Handler.java:435)
W/MessageQueue(16897):  at android.media.ImageReader$HandlerExecutor.execute(ImageReader.java:1155)
W/MessageQueue(16897):  at android.media.ImageReader.postEventFromNative(ImageReader.java:944)

Can you give a recommendation on how to close the Publisher when it has an active capturer but it is not attached to the session? onStop does not seem to be a stable way to stop the publisher.

UPDATE (OLD): It seems a though so far this is the way i can stop the capturer without getting errors:

// Stop video and audio, mainly video so the renderer can be disposed of.
patientPublisher?.publishVideo = false
patientPublisher?.publishAudio = false
// Pause renderer so nothing new get queued before we want to stop it.
patientPublisher?.renderer?.onPause()
// Attempt to release the surface
(patientPublisher?.view as? SurfaceView)?.holder?.surface?.release()
// Set the view to gone so it is not being rendered to the view hierarchy.
(patientPublisher?.view as? SurfaceView)?.visibility = View.GONE
// Finally call onStop to fully stop the publisher and its resources.
patientPublisher?.onStop()

UPDATE: It looks like eventually this error comes back even with the code above is used

java.lang.NullPointerException: Attempt to invoke virtual method 'void android.os.Handler.removeCallbacks(java.lang.Runnable)' on a null object reference

UPDATE: I found i could reproduce the null object reference by creating a Publisher > displaying it > closing it > calling onStop > repeat previous sequence a 1-3 times > then let the app sit for a while it seams like it is eventually caused by something getting garbage collected see logs:


I/BpBinder( 6585): onLastStrongRef automatically unlinking death recipients: 
I/BpBinder( 6585): onLastStrongRef automatically unlinking death recipients: 
I/BpBinder( 6585): onLastStrongRef automatically unlinking death recipients: 
D/AndroidRuntime( 6585): Shutting down VM
E/AndroidRuntime( 6585): FATAL EXCEPTION: main
E/AndroidRuntime( 6585): Process: com.transcarent.app, PID: 6585
E/AndroidRuntime( 6585): java.lang.NullPointerException: Attempt to invoke virtual method 'void android.os.Handler.removeCallbacks(java.lang.Runnable)' on a null object reference
E/AndroidRuntime( 6585):    at com.opentok.android.Camera2VideoCapturer.stopDisplayOrientationCache(Unknown Source:4)
E/AndroidRuntime( 6585):    at com.opentok.android.Camera2VideoCapturer.destroy(Unknown Source:11)
E/AndroidRuntime( 6585):    at com.opentok.android.PublisherKit.a(Unknown Source:4)
E/AndroidRuntime( 6585):    at com.opentok.android.PublisherKit.$r8$lambda$sUgdI5pEkyMBdAHJN5h9DorvkA0(Unknown Source:0)
E/AndroidRuntime( 6585):    at com.opentok.android.PublisherKit$$ExternalSyntheticLambda5.run(Unknown Source:2)
E/AndroidRuntime( 6585):    at android.os.Handler.handleCallback(Handler.java:958)
E/AndroidRuntime( 6585):    at android.os.Handler.dispatchMessage(Handler.java:99)
E/AndroidRuntime( 6585):    at android.os.Looper.loopOnce(Looper.java:205)
E/AndroidRuntime( 6585):    at android.os.Looper.loop(Looper.java:294)
E/AndroidRuntime( 6585):    at android.app.ActivityThread.main(ActivityThread.java:8194)
E/AndroidRuntime( 6585):    at java.lang.reflect.Method.invoke(Native Method)
E/AndroidRuntime( 6585):    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
E/AndroidRuntime( 6585):    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971)
W/System  ( 6585): A resource failed to call Surface.release. 
W/System  ( 6585): A resource failed to call Surface.release. 
W/System  ( 6585): A resource failed to call Surface.release. 
W/System  ( 6585): A resource failed to call Surface.release. 
W/System  ( 6585): A resource failed to call Surface.release. 
goncalocostamendes commented 10 months ago

@derekpitts28 just an FYI, the first error you mentioned Handler (android.media.ImageReader$ListenerHandler) {982841} sending message to a Handler on a dead thread is can also be caused by some event updating the UI from a background thread. At this moment, from our side, all subscriberListener methods are configured to run on a background thread. We are updating this on 2.27.1

goncalocostamendes commented 10 months ago

@derekpitts28 regarding your scenario keep the session open but stop the capturer, do you mean simply muting the video? Or unpublishing the publisher?

derekpitts28 commented 10 months ago

So in our case we have a session connected but we do not want to publish the users video yet. We only want to show a preview of the users view before they publish their video. If they decided to exit before publishing we need to close the publisher otherwise the capturer stays open and the user video indicator stays on. In almost all usages of onStop the app seems to run into the null object error eventually if not right away. I had my testers go through and use your recommended code for stopping the publisher ie using onStop and in almost all cases it crashes.

goncalocostamendes commented 10 months ago

@derekpitts28 it seems to me you will need to wait for the 2.27.1 release which provides more safety checks when stopping the capturer and associated threads. In the mean time I will try to replicate that scenario and check if any other improvement can be made

tinder-enginrumelioglu commented 10 months ago

I am running into the same issues that Derek is running into. It should be rather easy to duplicate with the following steps;

janoonaj commented 10 months ago

+1 It is happening in production with 2.27.0 We start a call but we see black screen

goncalocostamendes commented 10 months ago

@derekpitts28 @janoonaj @tinder-enginrumelioglu The safest way to stop the camera is, instead of calling directly onStop() method, use PublisherKit.setPublishVideo(boolean publishVideo) which internally will call onStop() method, it will stop publishing video (will send black frames to the subscriber) and will release the capturer. To stop it simply call publisher.setPublishVideo(false)

derekpitts28 commented 10 months ago

@derekpitts28 @janoonaj @tinder-enginrumelioglu The safest way to stop the camera is, instead of calling directly onStop() method, use PublisherKit.setPublishVideo(boolean publishVideo) which internally will call onStop() method, it will stop publishing video (will send black frames to the subscriber) and will release the capturer. To stop it simply call publisher.setPublishVideo(false)

@goncalocostamendes I believe this does not turn on the camera and the system green light stays on because vonage holds onto the camera object. I don't think this truely, stops/releases the camera and could cause users to think we are still using their camera when we are really not.

goncalocostamendes commented 10 months ago

@derekpitts28 @janoonaj @tinder-enginrumelioglu The safest way to stop the camera is, instead of calling directly onStop() method, use PublisherKit.setPublishVideo(boolean publishVideo) which internally will call onStop() method, it will stop publishing video (will send black frames to the subscriber) and will release the capturer. To stop it simply call publisher.setPublishVideo(false)

@goncalocostamendes I believe this does not turn on the camera and the system green light stays on because vonage holds onto the camera object. I don't think this truely, stops/releases the camera and could cause users to think we are still using their camera when we are really not.

This is how we do it on our internal app, and I can validate that the onStop() method is called when publisher.setPublishVideo(false) is triggered. In addition, by calling onStop() directly instead of stop publishing video there may occur some errors, since you are trying to publish video that you are no longer capturing/rendering.

derekpitts28 commented 10 months ago

Can you verify does publisher.setPublishVideo(false) release the camera ie does the camera green light turn off on newer devices?

goncalocostamendes commented 9 months ago

@derekpitts28 yes, it turns off the green LED

bricepowell commented 9 months ago

@derekpitts28 @janoonaj @tinder-enginrumelioglu The safest way to stop the camera is, instead of calling directly onStop() method, use PublisherKit.setPublishVideo(boolean publishVideo) which internally will call onStop() method, it will stop publishing video (will send black frames to the subscriber) and will release the capturer. To stop it simply call publisher.setPublishVideo(false)

@goncalocostamendes I believe this does not turn on the camera and the system green light stays on because vonage holds onto the camera object. I don't think this truely, stops/releases the camera and could cause users to think we are still using their camera when we are really not.

This is how we do it on our internal app, and I can validate that the onStop() method is called when publisher.setPublishVideo(false) is triggered. In addition, by calling onStop() directly instead of stop publishing video there may occur some errors, since you are trying to publish video that you are no longer capturing/rendering.

can confirm this worked for me. Thanks!