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 transaction too large in channel tab fragments #10673

Merged
merged 1 commit into from Dec 21, 2023

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Dec 20, 2023

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 PR contains a really hacky workaround, that avoids storing useless data in the ChannelTabFragment state. ChannelTabFragment uses the corresponding tab link handler to be able to fetch data, since unlike other things in the extractor, channel tabs are not supposed to be obtained from a url (e.g. provided by the user), but rather always from a channel page internally in the app.

The problem is, ReadyChannelTabListLinkHandler might contain raw JSON data that uses a lot of memory (e.g. ~800KB for YouTube). While 800KB doesn't seem much, if you combine just a couple of channel tab fragments you easily go over the 1MB save&restore transaction limit, and get TransactionTooLargeExceptions.

What this PR does is to discard the ReadyChannelTabListLinkHandler after the first time the raw data has been parsed (i.e. the fragment's handleResult has been called). A new link handler with the same properties is then created. The good thing is, even if now the raw data is not in the link handler anymore, if getChannelTab is called on the newly created link handler, the app cache will kick in to still prevent network requests. Note that I didn't test this link-handler-recreation with all services, I will let users do this (or @AudricV could provide some feedback).

A proper solution would require rethinking about ReadyChannelTabListLinkHandlers, but that's out of scope for 0.26.0. We might need to rethink how the cache works, and instead of returning link handlers with raw data (which turned out to be a really bad idea) , we should put the raw data directly in an extractor cache.

You can test the changes by putting one channel from all NewPipe services (Youtube, Soundcloud, ...) as a main page tab. Then open settings and the app should not crash. All video/audio tabs from all channels should still allow e.g. playing in the background without crashing.

Fixes the following issue(s)

APK 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

@Stypox Stypox mentioned this pull request Dec 20, 2023
21 tasks
Copy link

sonarcloud bot commented Dec 20, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@AudricV AudricV added bug Issue is related to a bug channel Anything to do with creator channels in the app size/small PRs with less than 50 changed lines labels Dec 21, 2023
@Stypox Stypox merged commit 5b999a8 into TeamNewPipe:release-0.26.0 Dec 21, 2023
7 of 10 checks passed
Comment on lines +131 to +134
tabHandler = result.getService()
.getChannelTabLHFactory()
.fromQuery(tabHandler.getId(), tabHandler.getContentFilters(),
tabHandler.getSortFilter());
Copy link
Member

Choose a reason for hiding this comment

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

This causes a crash. Use media.ccc.de service, open a channel / conference. getChannelTabLHFactory() is null for that service:

https://github.com/TeamNewPipe/NewPipeExtractor/blob/f276caf54a85a271904943e4aa6c57efd4249fec/extractor/src/main/java/org/schabi/newpipe/extractor/services/media_ccc/MediaCCCService.java#L53-L56

Exception

  • User Action: ui error
  • Request: ACRA report
  • Content Country: DE
  • Content Language: de-
  • App Language: en
  • Service: none
  • Version: 0.26.0
  • OS: Linux Android 12 - 32
Crash log

java.lang.NullPointerException: Attempt to invoke virtual method 'org.schabi.newpipe.extractor.linkhandler.ListLinkHandler org.schabi.newpipe.extractor.linkhandler.ListLinkHandlerFactory.fromQuery(java.lang.String, java.util.List, java.lang.String)' on a null object reference
	at org.schabi.newpipe.fragments.list.channel.ChannelTabFragment.handleResult(ChannelTabFragment.java:133)
	at org.schabi.newpipe.fragments.list.channel.ChannelTabFragment.handleResult(ChannelTabFragment.java:37)
	at org.schabi.newpipe.fragments.list.BaseListInfoFragment.lambda$startLoading$0(BaseListInfoFragment.java:150)
	at org.schabi.newpipe.fragments.list.BaseListInfoFragment.$r8$lambda$yX6S2nKGSbR70YowNdSpGemC7q4(BaseListInfoFragment.java:0)
	at org.schabi.newpipe.fragments.list.BaseListInfoFragment$$ExternalSyntheticLambda4.accept(R8$$SyntheticClass:0)
	at io.reactivex.rxjava3.internal.observers.ConsumerSingleObserver.onSuccess(ConsumerSingleObserver.java:62)
	at io.reactivex.rxjava3.internal.operators.single.SingleObserveOn$ObserveOnSingleObserver.run(SingleObserveOn.java:81)
	at io.reactivex.rxjava3.android.schedulers.HandlerScheduler$ScheduledRunnable.run(HandlerScheduler.java:123)
	at android.os.Handler.handleCallback(Handler.java:938)
	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:7870)
	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:1003)


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug channel Anything to do with creator channels in the app size/small PRs with less than 50 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants