Fix channel subscribe button causing crash on closing

[Stypox]

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

This was probably caused by the binding being set to null since the activity was closing, but the async channel subscription disposable being run just after that. #5455 (comment)

Due diligence

• I read the contribution guidelines.

[XiangRongLin]

@brnwlshubh Can you confirm that it works now? I couldn't reporoduce that problem myself.

@Stypox Can you add the reasoning to the commit message, so there is a way to find out why that code exists without digging through github

[brnwlshubh]

Will I have to check existing 0.20.9 package or by installing new apk file ?

[XiangRongLin]

Get the apk from here at the bottom under artifacts https://github.com/TeamNewPipe/NewPipe/actions/runs/498326163

[brnwlshubh]

Exception

• User Action: ui error
• Request: App crash, UI failure
• Content Country: IN
• Content Language: en-GB
• App Language: en_GB
• Service: none
• Version: 0.20.8
• OS: Linux samsung/on7xeltedd/on7xelte:8.1.0/M1AJQ/G610FDDU1CSK2:user/release-keys 8.1.0 - 27

Crash log

java.lang.NullPointerException: Attempt to read from field 'androidx.appcompat.widget.AppCompatButton org.schabi.newpipe.databinding.ChannelHeaderBinding.channelSubscribeButton' on a null object reference
	at org.schabi.newpipe.fragments.list.channel.ChannelFragment.updateSubscribeButton(ChannelFragment.java:356)
	at org.schabi.newpipe.fragments.list.channel.ChannelFragment.lambda$monitorSubscription$1$ChannelFragment(ChannelFragment.java:247)
	at org.schabi.newpipe.fragments.list.channel.-$$Lambda$ChannelFragment$00sQ8kobTMBsYnX1bMmu1QeN9lg.accept(Unknown Source:4)
	at io.reactivex.rxjava3.internal.observers.LambdaObserver.onNext(LambdaObserver.java:63)
	at io.reactivex.rxjava3.internal.operators.observable.ObservableObserveOn$ObserveOnObserver.drainNormal(ObservableObserveOn.java:201)
	at io.reactivex.rxjava3.internal.operators.observable.ObservableObserveOn$ObserveOnObserver.run(ObservableObserveOn.java:255)
	at io.reactivex.rxjava3.android.schedulers.HandlerScheduler$ScheduledRunnable.run(HandlerScheduler.java:123)
	at android.os.Handler.handleCallback(Handler.java:790)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:164)
	at android.app.ActivityThread.main(ActivityThread.java:7000)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:441)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1408)

---

[Stypox]

@brnwlshubh could you provide detailed steps to reproduce? For me everything works well (and it did work well even before).

[brnwlshubh]

When we create Youtube subscribe channel group on the front page.
When I am refreshing the channel group then this error is occurring and the app crashes.
I hope I am able to explain the problem caused,If not then mention me I will give the screen recording of the problem.

Thank you @Stypox

[Stypox]

Do you also have a channel as a main page tab? That error lies inside the channel view, not the channel groups view, so I don't understand

[brnwlshubh]

Error occured in refreshing the channel group not in opening channel page.

Channel page is working fine.
Problem is in refreshing the subscribed channel group.

[Stypox]

Problem is in refreshing the subscribed channel group.

I understand, but from a code point of view the problem lies in the channel page. So I'm asking: do you have a channel page as a main page tab?

[XiangRongLin]

@Stypox I think the easiest thing would be if @brnwlshubh can create an export of the settings and the channels of the app with which the crash occurs.

[brnwlshubh]

@Stypox Main page is "Trending" and beside this All Subscribed Channel list page

[Stypox]

@brnwlshubh could you test now? It should be fixed, I tested with the settings you provided. Thank you for them! I edited your comment to hide them, since they could be sensitive information ;-)

[brnwlshubh]

Should I download new App from "checks" section?

[Stypox]

Yes, this is the apk: https://github.com/TeamNewPipe/NewPipe/suites/1873031044/artifacts/36501228

[brnwlshubh]

Now it's working fine 😊
Thank you all.

[XiangRongLin]

@Stypox Do you have any clue on why that fixes the problem.
Because according to the android doc example onDestroyView() is the correct place to set the bindings to null. Only difference to the examples is that super.onDestroyView() is called before setting them to null

https://developer.android.com/topic/libraries/view-binding#fragments

@Isira-Seneviratne Any ideas?

[Stypox]

@XiangRongLin see #4814 (comment) and #5417

[XiangRongLin]

@Stypox #5417 Does not have any explanation either and #4814 (comment) got rebased, so i can't see the version of the code anymore that you commented on. With the way its currently displayed there i would still assume that it being in onDestroyView() is correct https://github.com/TeamNewPipe/NewPipe/pull/4814/files#diff-2b105f5f139031eb8a0a729a1371c6646ae2b525e85757aaa1e5379feee99bedR123

[opusforlife2]

Channel page is working fine.

@Stypox I have this error when opening the channel page when using RC3.

It's very specific:

1. Finish a video (the replay button must show up, it won't happen for unwatched videos or playing/paused ones).
2. Try to open the channel of the uploader.
3. Crash to home screen. Sometimes it opens the error activity, sometimes it doesn't.

Hope that helps with debugging.

[Stypox]

@opusforlife2 maybe it's this same error, maybe it's not, can you reproduce in this PR?

[opusforlife2]

Yup! This PR fixed the bug. 👍

[TobiGr]

I don't understand how your changes fix the bug. @Stypox Can you please explain, why the bindings need to be set to null in onDestroy() and not onDestroyView()? From what I understand, I think the proper solution is to ensure the bindings are inflated in onCreateView(). Otherwise we might risk memory leaks.

[Stypox]

I have no idea about this, and neither has @Isira-Seneviratne afaik 😅

[Isira-Seneviratne]

Sorry for the late reply, I think it's because the RxJava task(s) hold references to the view binding classes.

[XiangRongLin]

I think it's because the RxJava task(s) hold references to the view binding classes.

Then it should be fixed properly by changing this and not by adding a workaround to the symptoms.
The same applies for #5417 and all other places in #4814 where this could occur.

For me it also seems like a bad practice for RxJava (logic) to have a reference to the binding (UI), but i don't have any experience with RxJava. References should only go UI -> logic -> data. If it the reference is needed, then the task should be cancelled, when the UI is destroyed.
Similiar to how kotlin coroutines are always bound to a scope, so that they are automatically cancelled when the scope is getting destroyed.

[Stypox]

@Isira-Seneviratne oh, ok, maybe we know how this can be fixed then.
@XiangRongLin I agree, though this should be merged anyway before 0.20.9

[XiangRongLin]

@Stypox If you can guarantee that it will be worked on in the next release sure. Otherwise no.

[Stypox]

@XiangRongLin I can't guarantee I will have time to dedicate to this after this release, but it is needed anyway to prevent crashes.

[XiangRongLin]

@Stypox The problem i have with that is that it's then just not going to be done and just forgotten.

With the way it is, i would just revert the binding PR because it replaces one problem with another one. The old one im not even sure if it was a problem, since the deprecation is kotlin only.
That old one is replaced with a new problem of having to clean up bindings in onDestroy instead of onDestroyView that normal developers DON'T know about, since the official guidelines have it in onDestroyView

[Stypox]

I think it's too late to revert view binding, there have been many changes since then

[TobiGr]

@TacoTheDank do you have an idea how to solve the problem properly?

[TacoTheDank]

Uff, I don't really know, either. All the ideas I could currently think of were already stated by @XiangRongLin. Would it be possible to revert viewbinding in just the ChannelFragment for now?

[Stypox]

@TacoTheDank this workaround is already being used in many other places so either we revert them all or we revert none

[Isira-Seneviratne]

I added a commit to set the view bindings in the Fragment classes to null after the disposables were cleared/disposed here: #5461

[Stypox]

@Isira-Seneviratne I think the correct thing to do would be instead to dispose disposables in onDestroyView() just before setting the binding to null, since bindings should be set to null in onDestroyView() according to android documentation.

[Isira-Seneviratne]

@Isira-Seneviratne I think the correct thing to do would be instead to dispose disposables in onDestroyView() just before setting the binding to null, since bindings should be set to null in onDestroyView() according to android documentation.

Sure.

[Stypox]

Thank you @Isira-Seneviratne ! Since a proper solution is there that can be reviewed and merged by next release, I'd merge this for now. @TobiGr @XiangRongLin