Skip to content

Commit cfcca8a

Browse files
toniheimarcbaechinger
authored andcommitted
Fix potential NPE in DefaultPlaybackSessionManager
When handling timeline updates or position discontinuities, we may remove the current session and later attempt to access it again if we haven't explicitly cleared the currentSessionId. This could happen if this currentSessionId has not been marked as isCreated. Part of this change guards against that gap by more clearly clearing the currentSessionId in any case. This whole situation shouldn't happen though because every situation where we set the currentSessionId should also ensure that this session is marked as isCreated. One way to reproduce an issue is to generate data for a not-created placeholder session and then making this session the current one by matching it with an event for a different window index. This would hit one of the early returns in updateSession before the session is marked as isCreated. To prevent this root cause, we can make sure to not match such sessions in belongsToSession. Additionally, the check in updateSessions for the sequence number should only be used if there actually is a sequence number to check (a bug that got accidentally introduced by e0191dd) Issue: #2885 PiperOrigin-RevId: 832263214 (cherry picked from commit 7784e73)
1 parent 06e459e commit cfcca8a

File tree

3 files changed

+52
-10
lines changed

3 files changed

+52
-10
lines changed

RELEASENOTES.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
a non-empty media source.
1212
* Fix bug where seeking into other media items while in scrubbing mode
1313
could cause `IllegalStateException`.
14+
* Fix potential `NullPointerException` in `DefaultPlaybackSessionManager`
15+
([#2885](https://github.com/androidx/media/issues/2885)).
1416
* CompositionPlayer:
1517
* Transformer:
1618
* Track Selection:

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManager.java

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,8 @@ public synchronized void updateSessions(EventTime eventTime) {
113113
return;
114114
}
115115
if (eventTime.mediaPeriodId != null) {
116-
if (eventTime.mediaPeriodId.windowSequenceNumber < getMinWindowSequenceNumber()) {
116+
if (eventTime.mediaPeriodId.windowSequenceNumber != C.INDEX_UNSET
117+
&& eventTime.mediaPeriodId.windowSequenceNumber < getMinWindowSequenceNumber()) {
117118
// Ignore event because it is part of a past window that has already been finished.
118119
return;
119120
}
@@ -184,10 +185,10 @@ public synchronized void updateSessionsWithTimelineChange(EventTime eventTime) {
184185
if (!session.tryResolvingToNewTimeline(previousTimeline, currentTimeline)
185186
|| session.isFinishedAtEventTime(eventTime)) {
186187
iterator.remove();
188+
if (session.sessionId.equals(currentSessionId)) {
189+
clearCurrentSession(session);
190+
}
187191
if (session.isCreated) {
188-
if (session.sessionId.equals(currentSessionId)) {
189-
clearCurrentSession(session);
190-
}
191192
listener.onSessionFinished(
192193
eventTime, session.sessionId, /* automaticTransitionToNextPlayback= */ false);
193194
}
@@ -206,13 +207,13 @@ public synchronized void updateSessionsWithDiscontinuity(
206207
SessionDescriptor session = iterator.next();
207208
if (session.isFinishedAtEventTime(eventTime)) {
208209
iterator.remove();
210+
boolean isRemovingCurrentSession = session.sessionId.equals(currentSessionId);
211+
if (isRemovingCurrentSession) {
212+
clearCurrentSession(session);
213+
}
209214
if (session.isCreated) {
210-
boolean isRemovingCurrentSession = session.sessionId.equals(currentSessionId);
211215
boolean isAutomaticTransition =
212216
hasAutomaticTransition && isRemovingCurrentSession && session.isActive;
213-
if (isRemovingCurrentSession) {
214-
clearCurrentSession(session);
215-
}
216217
listener.onSessionFinished(eventTime, session.sessionId, isAutomaticTransition);
217218
}
218219
}
@@ -278,7 +279,7 @@ private void updateCurrentSession(EventTime eventTime) {
278279
}
279280

280281
private void clearCurrentSession(SessionDescriptor currentSession) {
281-
if (currentSession.windowSequenceNumber != C.INDEX_UNSET) {
282+
if (currentSession.windowSequenceNumber != C.INDEX_UNSET && currentSession.isCreated) {
282283
lastRemovedCurrentWindowSequenceNumber = currentSession.windowSequenceNumber;
283284
}
284285
currentSessionId = null;
@@ -375,7 +376,7 @@ public boolean tryResolvingToNewTimeline(Timeline oldTimeline, Timeline newTimel
375376

376377
public boolean belongsToSession(
377378
int eventWindowIndex, @Nullable MediaPeriodId eventMediaPeriodId) {
378-
if (eventMediaPeriodId == null) {
379+
if (eventMediaPeriodId == null || eventMediaPeriodId.windowSequenceNumber == C.INDEX_UNSET) {
379380
// Events without concrete media period id are for all sessions of the same window.
380381
return eventWindowIndex == windowIndex;
381382
}

libraries/exoplayer/src/test/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManagerTest.java

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -967,6 +967,45 @@ public void timelineUpdate_toNewMediaWithMediaPeriodId_finishesOtherSessions() {
967967
inOrder.verifyNoMoreInteractions();
968968
}
969969

970+
@Test
971+
public void
972+
timelineUpdates_withUncreatedSessionDataForPlaceholderSession_createsRealSessionOnly() {
973+
Timeline timeline = new FakeTimeline(/* windowCount= */ 2);
974+
MediaPeriodId placeholderMediaPeriodIdWindow0 =
975+
new MediaPeriodId(
976+
timeline.getUidOfPeriod(/* periodIndex= */ 0),
977+
/* windowSequenceNumber= */ C.INDEX_UNSET);
978+
MediaPeriodId placeholderMediaPeriodIdWindow1 =
979+
new MediaPeriodId(
980+
timeline.getUidOfPeriod(/* periodIndex= */ 1),
981+
/* windowSequenceNumber= */ C.INDEX_UNSET);
982+
EventTime placeholderEventTimeWindow0 =
983+
createEventTime(timeline, /* windowIndex= */ 0, placeholderMediaPeriodIdWindow0);
984+
EventTime emptyTimelineEventTime =
985+
createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId= */ null);
986+
987+
// Plant data for a placeholder session with unset sequence number using windowIndex = 1
988+
sessionManager.getSessionForMediaPeriodId(timeline, placeholderMediaPeriodIdWindow1);
989+
// Simulate timeline update to another placeholder id using windowIndex = 0
990+
sessionManager.updateSessionsWithTimelineChange(placeholderEventTimeWindow0);
991+
// Clear timeline completely to verify state is cleared up correctly
992+
sessionManager.updateSessionsWithTimelineChange(emptyTimelineEventTime);
993+
994+
InOrder inOrder = inOrder(mockListener);
995+
ArgumentCaptor<String> sessionId = ArgumentCaptor.forClass(String.class);
996+
inOrder
997+
.verify(mockListener)
998+
.onSessionCreated(eq(placeholderEventTimeWindow0), sessionId.capture());
999+
inOrder.verify(mockListener).onSessionActive(placeholderEventTimeWindow0, sessionId.getValue());
1000+
inOrder
1001+
.verify(mockListener)
1002+
.onSessionFinished(
1003+
emptyTimelineEventTime,
1004+
sessionId.getValue(),
1005+
/* automaticTransitionToNextPlayback= */ false);
1006+
inOrder.verifyNoMoreInteractions();
1007+
}
1008+
9701009
@Test
9711010
public void positionDiscontinuity_withinWindow_doesNotFinishSession() {
9721011
Timeline timeline =

0 commit comments

Comments
 (0)