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.

Sign up for GitHub

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

Jump to bottom

Application using the library will crash if 1000 DownloadHelper instances are created with a non-null RenderersFactory #1224

Closed
1 task
retiredreplicant opened this issue Mar 26, 2024 · 11 comments
Closed
1 task

Application using the library will crash if 1000 DownloadHelper instances are created with a non-null RenderersFactory #1224

retiredreplicant opened this issue Mar 26, 2024 · 11 comments
Assignees
@icbaker
Labels
bug needs triage

Comments

@retiredreplicant
Copy link

retiredreplicant commented Mar 26, 2024 •
edited
Loading

Version

Media3 1.3.0

More version details

d13a0f4

Devices that reproduce the issue

All devices

Devices that do not reproduce the issue

No devices excluded

Reproducible in the demo app?

No

Reproduction steps

  1. Create a player application that can create and release 1000 DownloadHelper instances with a non-null RenderersFactory (or modify the demo app to do so.)
  2. Create and release 1000 DownloadHelper instances with a non-null RenderersFactory

Expected result

An application using the player should be able to create an unlimited number of DownloadHelper instances with a non-null RenderersFactory.

Actual result

The application will crash with a "Too many receivers, total of 1000, registered for pid" exception.

Source of Issue
The source of the issue is that the DownloadHelper uses the passed RenderersFactory to create renderers, and to get capabilities for those renderers, but never releases the renderers that are created. One of the renderers created is a MediaCodecAudioRenderer. That renderer is created with a DefaultAudioSink. When the DownloadHelper processes a DOWNLOAD_HELPER_CALLBACK_MESSAGE_PREPARED message, the DownloadHelper calls runTrackSelection and this results in the DefaultAudioSink creating an instance of AudioCapabilitiesReceiver and registering that receiver. The release() method for the DefaultAudioSink unregisters that listener, but since the DownloadHelper does not call release() on the renderers it creates, release is never called on the DefaultAudioSink and the unregister never occurs. If 1000 DownloadHelpers are created with a non-null RenderersFactory, the application will crash with a "Too many receivers, total of 1000, registered for pid" exception.

Reproduction with Demo App
Not reproducible with the demo app only because there are not 1000 items to download. However, the registration of a AudioCapabilitiesReceiver for each download without a corresponding unregister can be seen with the demo app. Set breakpoints at lines 1520 audioCapabilitiesReceived.unregister(); and 1749 audioCapabilities = audioCapabilitiesReceiver.register(); in DefaultAudioSink and initiated a download with the demo app. You will hit the breakpoint at 1749 (register) for each download, but the breakpoint at 1520 (unregister) is never hit. This demonstrates receivers being registered but never unregistered. Note that the player does properly release the renderers that it creates. It is only the DownloadHelper that does not.

Recommend Change
The DownloadHelper needs to keep a handle to the renderers it creates much in the same way it keeps a handle on the RendererCapabilities that it obtains. When DownloadHelper.release() is called, the class needs to iterate through the renderers that it has created and call the release method for each. This will result the MediaCodecAudioRenderer being released, which will in turn result in the the DefaultAudioSink being released and the AudioCapabilitiesReceiver being unregistered.

Media

Not applicable

Bug Report

  • You will email the zip file produced by adb bugreport to [email protected] after filing this issue.
@icbaker icbaker self-assigned this Mar 27, 2024
@icbaker
Copy link
Collaborator

icbaker commented Mar 27, 2024

2. Create and release 1000 DownloadHelper instances with a non-null RenderersFactory

I tried to repro this by adding a for (int i = 0; i < 1001; i++) loop around this code:

media/demos/main/src/main/java/androidx/media3/demo/main/DownloadTracker.java

Lines 120 to 128 in 66547cc

if (startDownloadDialogHelper != null) {
startDownloadDialogHelper.release();
}
startDownloadDialogHelper =
new StartDownloadDialogHelper(
fragmentManager,
DownloadHelper.forMediaItem(context, mediaItem, renderersFactory, dataSourceFactory),
mediaItem);
}

This should result in creating, preparing, then releasing 1001 DownloadHelper instances.

I then launched the application and downloaded the Misc > Dizzy (MP4) asset.

The application didn't crash. I also added checkNotNull to ensure that renderersFactory is non-null.

I then tried waiting for DownloadHelper.Callback.onPrepared before releasing and re-creating, by adding the following code here:

media/demos/main/src/main/java/androidx/media3/demo/main/DownloadTracker.java

Line 196 in 66547cc

public void onPrepared(DownloadHelper helper) { 

if (prepareCount.getAndIncrement() < 2001) {
  downloadHelper.release();
  downloadHelper =
      DownloadHelper.forMediaItem(context, mediaItem, renderersFactory, dataSourceFactory);
  downloadHelper.prepare(this);
}

The application still didn't crash.

Please can you provide some more precise repro steps, ideally as a fork of the demo app that we can clone.

@retiredreplicant
Copy link
Author

I will work on modifying the demo app to reproduce. In attempting to reproduce the issue with my application, sometimes it is the player that hits the 1000 receiver limit and the application does not crash in that case. There is just a player error.

Since both the the player and the DownloadHelper register an AudioCapabilitiesReceiver, it is a 50/50 chance which will run into the 1000 receiver limit first. But, it is the DownloadHelper that is registering receivers and not releasing them. The player always unregisters the listeners that in registers. The dumpstate is too big to attach here or email. I have attached the stack trace for the case where the application crashes.

stack_trace.txt

@retiredreplicant
Copy link
Author

For you reproduction attempts, you definitely need to prepare the media. Also, the media selected has to be something that results in DownloadHelper.onMediaPrepared() calling runTrackSelection. That is when the AudioCapabilitiesReceiver is registered.
Screenshot (81)

@icbaker
Copy link
Collaborator

icbaker commented Mar 27, 2024

Also, the media selected has to be something that results in DownloadHelper.onMediaPrepared() calling runTrackSelection.

Thanks, this was the clue I was missing. Switching to using the Clear DASH -> HD (MP4, H264) sample, and then fixing a bug in my second repro case above (adding missing return at the bottom of the if) I'm able to reproduce.

@sahilgarg90 sahilgarg90 mentioned this issue Mar 28, 2024
Closed
@sahilgarg90
Copy link

@retiredreplicant @icbaker
I was facing the same issue and the following pull request changes worked for me.

#1230

copybara-service bot pushed a commit that referenced this issue Mar 28, 2024
@icbaker
Ensure DownloadHelper doesn't leak unreleased Renderer instances
c5e894e 
Issue: #1224
PiperOrigin-RevId: 619935786
@icbaker
Copy link
Collaborator

icbaker commented Mar 28, 2024

This is fixed by the commit referenced above.

@icbaker icbaker closed this as completed Mar 28, 2024
copybara-service bot pushed a commit to google/ExoPlayer that referenced this issue Mar 28, 2024
@icbaker
Ensure DownloadHelper doesn't leak unreleased Renderer instances
0a20d92 
Issue: androidx/media#1224
PiperOrigin-RevId: 619935786
@sahilpocketfm
Copy link

When can we expect this to be included in final release?

@icbaker
Copy link
Collaborator

icbaker commented Apr 2, 2024

The fix will be included in the 1.4.0 release.

@icbaker
Copy link
Collaborator

icbaker commented Apr 8, 2024

Unfortunately c5e894e was rolled back in c2356e3 due to causing crashes when integrated with a google app which calls DownloadHelper.forMediaItem from a non-looper thread. Re-opening this issue while we look into resolving this and rolling the fix back forward.

@icbaker icbaker reopened this Apr 8, 2024
@sahilpocketfm
Copy link

@icbaker Since above mentioned commit is rolled back, can we consider this PR now? This worked in our case

#1230

@icbaker
Copy link
Collaborator

icbaker commented Apr 8, 2024

The fix has been resubmitted with a fix & related tests: fad3257

@icbaker icbaker closed this as completed Apr 8, 2024
@androidx androidx locked and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@icbaker icbaker

Labels
bug needs triage
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@icbaker @sahilgarg90 @retiredreplicant @google-oss-bot @sahilpocketfm