Repository navigation
Conversation
addInstance() hides binding.loadingProgressBar once the instance request finishes, but the request was only disposed in onDestroy(), while onDestroyView() already sets binding to null. Leaving the screen is fine today, because onDestroy() runs right after onDestroyView(). If the fragment ever stays alive without its view, for example on the back stack, the callback dereferences the null binding. Clear the disposables in onDestroyView() so the request ends together with the view it updates. Fixes TeamNewPipe#13871 Reported-by: BS.ehlas.cz
|
|
@Huge thanks for the PR. Could you agree with the AI policy and also test your change on a real device to verify if your issue is fixed? |
|
Tested on a Samsung Galaxy S24 (Android 16) with debug builds of |
|
Friendly ping: is there anything else you need from me to get this merged? Happy to adjust. |
| @Override | ||
| public void onDestroy() { | ||
| super.onDestroy(); | ||
| if (disposables != null) { |
There was a problem hiding this comment.
Nitpick but this can be probably removed and the check could be added to the onDestoryView call
Thanks, looks good to me other than one minor comment to avoid duplicate calls. |
No NPE on dev neither does this seems to fix log message
|
@Huge some questions.
Does this means that your issue actually was reported without testing/reproduction? If not, how did you document it as NPE with log message? Asking as I am unable to see any changes of this PR. Second question from me is, did you use any AI to report or generate this fix? If so, how much of the involvement was done with AI in both issue reporting and this MR. |
What is it?
Description of the changes in your PR
PeertubeInstanceListFragment.addInstance()hidesbinding.loadingProgressBarwhen the instance request finishes, but the request was only disposed inonDestroy(), whileonDestroyView()already setsbindingto null. The disposables are now cleared inonDestroyView(), so the request ends together with the view it updates.onDestroy()runs right afteronDestroyView(), before the callback can reach the main thread, and the same holds for rotation and for closing the settings. The callback only meets a nullbindingif the fragment stays alive without its view, for example on the back stack, and no navigation from this screen does that today. So this closes a latent crash, not one users can hit right now.Fixes the following issue(s)
Testing/QA
Devices
devand of this branch: added an unreachable instance (10.255.255.1) and pressed Back right away, then waited for the request to time out. Neither build crashed; the request is disposed when the screen is left. Addingvideo.blender.orgworks with this change.Steps for testing
addInstance(), does one of five things and then lets the request finish. Ondev, only "fragment kept on the back stack without its view" ends inNullPointerException: Cannot read field "loadingProgressBar" because "this.binding" is null. Back, rotation, closing the settings and the open screen are fine. With this change none of the five fails. Happy to share the test if useful../gradlew :app:testDebugUnitTestand:app:assembleDebugpass.APK for testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.
Due diligence
Disclaimer: Found the issue by test-driving an LM-powered vulnerability scanner I work on heavily. The fix and this description were drafted with AI help and reviewed by me.