Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix null pointer exception in play button method #5301

Merged
merged 1 commit into from
Dec 30, 2020

Conversation

EricLemieux
Copy link
Contributor

First time looking into the source for NewPipe, if anything needs to be updated or done in a different way let me know and I will be happy to update.

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

When the play queue was null, and this method was called a null pointer exception would be thrown. This change adds an additional check to see if the play queue is not null before making additional changes.

Fixes the following issue(s)

I didn't open an issue, because this seemed like a such a small straightforward issue. I did search and didn't see any other related issues.

This is the error I got from the app, where I can see a NPE being thrown in the animatePlayButtons method, when getting the index of the play queue.

{
  "user_action": "ui error",
  "request": "App crash, UI failure",
  "content_language": "en-CA",
  "content_country": "CA",
  "app_language": "en_CA",
  "service": "none",
  "package": "org.schabi.newpipe",
  "version": "0.20.6",
  "os": "Linux samsung/dream2qltevl/dream2qltecan:9/PPR1.180610.011/G955WVLU8CTF3:user/release-keys 9 - 28",
  "time": "2020-12-22 13:28",
  "exceptions": [
              "java.lang.NullPointerException: Attempt to invoke virtual method 'int org.schabi.newpipe.player.playqueue.PlayQueue.getIndex()' on a null object reference\n\tat org.schabi.newpipe.player.VideoPlayerImpl.animatePlayButtons(VideoPlayerImpl.java:1066)\n\tat org.schabi.newpipe.player.VideoPlayerImpl.onPausedSeek(VideoPlayerImpl.java:1146)\n\tat org.schabi.newpipe.player.BasePlayer.changeState(BasePlayer.java:591)\n\tat org.schabi.newpipe.player.VideoPlayerImpl.changeState(VideoPlayerImpl.java:1077)\n\tat org.schabi.newpipe.player.VideoPlayer.onStartTrackingTouch(VideoPlayer.java:826)\n\tat android.widget.SeekBar.onStartTrackingTouch(SeekBar.java:115)\n\tat android.widget.AbsSeekBar.onTouchEvent(AbsSeekBar.java:1160)\n\tat android.view.View.dispatchTouchEvent(View.java:13484)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:3222)\n\tat android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2904)\n\tat com.android.internal.policy.DecorView.superDispatchTouchEvent(DecorView.java:697)\n\tat com.android.internal.policy.PhoneWindow.superDispatchTouchEvent(PhoneWindow.java:1879)\n\tat android.app.Activity.dispatchTouchEvent(Activity.java:3487)\n\tat androidx.appcompat.view.WindowCallbackWrapper.dispatchTouchEvent(WindowCallbackWrapper.java:69)\n\tat androidx.appcompat.view.WindowCallbackWrapper.dispatchTouchEvent(WindowCallbackWrapper.java:69)\n\tat com.android.internal.policy.DecorView.dispatchTouchEvent(DecorView.java:655)\n\tat android.view.View.dispatchPointerEvent(View.java:13732)\n\tat android.view.ViewRootImpl$ViewPostImeInputStage.processPointerEvent(ViewRootImpl.java:6119)\n\tat android.view.ViewRootImpl$ViewPostImeInputStage.onProcess(ViewRootImpl.java:5897)\n\tat android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:5346)\n\tat android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:5399)\n\tat android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:5365)\n\tat android.view.ViewRootImpl$AsyncInputStage.forward(ViewRootImpl.java:5524)\n\tat android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:5373)\n\tat android.view.ViewRootImpl$AsyncInputStage.apply(ViewRootImpl.java:5581)\n\tat android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:5346)\n\tat android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:5399)\n\tat android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:5365)\n\tat android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:5373)\n\tat android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:5346)\n\tat android.view.ViewRootImpl.deliverInputEvent(ViewRootImpl.java:8408)\n\tat android.view.ViewRootImpl.doProcessInputEvents(ViewRootImpl.java:8341)\n\tat android.view.ViewRootImpl.enqueueInputEvent(ViewRootImpl.java:8294)\n\tat android.view.ViewRootImpl$WindowInputEventReceiver.onInputEvent(ViewRootImpl.java:8523)\n\tat android.view.InputEventReceiver.dispatchInputEvent(InputEventReceiver.java:198)\n\tat android.os.MessageQueue.nativePollOnce(Native Method)\n\tat android.os.MessageQueue.next(MessageQueue.java:326)\n\tat android.os.Looper.loop(Looper.java:181)\n\tat android.app.ActivityThread.main(ActivityThread.java:7050)\n\tat java.lang.reflect.Method.invoke(Native Method)\n\tat com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:494)\n\tat com.android.internal.os.ZygoteInit.main(ZygoteInit.java:964)\n"
  ],
  "user_comment": ""
}

APK testing

I don't currently have a apk for testing, if this is required I can look into generating one.

Due diligence

@Redirion
Copy link
Member

Thanks for finding this.

If show is false, the view would be set to GONE which should be preferred instead of doing nothing.

So I would suggest to change playQueue.getIndex() > 0 || !show to !show || playQueue.getIndex() > 0 and remove the final modifier from show and set it to false if playQueue is null.

I think when the queue is not available it doesn't make sense to ever show the playPreviousButton.

@XiangRongLin
Copy link
Collaborator

Is there a reason why the play queue can be null in the first place?

Also how did you produce the crash, what steps where necessary?

When the play queue was null, and this method was called a null pointer
exception would be thrown. This change adds an additional check to see
if the play queue is not null before making additional changes.
@EricLemieux
Copy link
Contributor Author

@Redirion I have updated the logic to match what you were describing. When I removed final from show, checkStyle didn't like that so I created a new variable to be used. CheckStyle is still complaining, but about things that I didn't change.

@XiangRongLin I wasn't able to create a reproducible way of causing this crash, the issue originally happened after switching back to the app after some time, so it could be caused by a cache being cleared while the app is in the background. This doesn't seem to be a wide spread issue as I haven't seen anyone else with the issue and I have only seen it happen once.

@Redirion
Copy link
Member

thank you @EricLemieux

what you could do so that we can maybe someday invest more work to find the root cause is to add a warning log:

if (DEBUG) {
            Log.w(TAG, "animatePlayButtons() playQueue is null");
        }

within your null check

@XiangRongLin I don't think this is a big issue that would require immediate action to find the root cause. So I would prefer to just merge this fix to make the app more resilient. Do you agree?

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @Redirion
Thank you! :-D

@XiangRongLin
Copy link
Collaborator

@Redirion

I don't think this is a big issue that would require immediate action to find the root cause. So I would prefer to just merge this fix to make the app more resilient. Do you agree?

Fine by me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants