Skip to content

[core] CPlayListPlayer::Reset dereferences the GUI unguarded, while SetRepeat two functions away null-checks it #131

Description

@malard

CPlayListPlayer::Reset() dereferences the GUI without checking it, two functions away from one
that does check. A test cannot call it, so the production route into the defect fixed by
xbmc#28873 cannot be driven from a test.

Verified against origin/master at 858c6ee4b6.

The inconsistency

Unguarded - xbmc/PlayListPlayer.cpp:517-526:

void CPlayListPlayer::Reset()
{
  m_iCurrentSong = -1;
  ...
  CGUIMessage msg(GUI_MSG_PLAYLIST_CHANGED, 0, 0);
  CServiceBroker::GetGUI()->GetWindowManager().SendMessage(msg);   // :525
}

Guarded - SetRepeat, PlayListPlayer.cpp:668:

if (CServiceBroker::GetGUI() != nullptr)
{
  CGUIMessage msg(GUI_MSG_PLAYLIST_CHANGED, 0, 0);
  CServiceBroker::GetGUI()->GetWindowManager().SendMessage(msg);
}

Across the file: 19 unguarded GetGUI()-> dereferences against 2 guarded ones. Whichever way
this should go, it is currently both.

Why it matters beyond tidiness

ClearPlaylist calls Reset(), which is how the current index reaches -1 in the application.
The test added by xbmc#28873 has to set that index directly instead, so it covers
GetNextItemIdx's handling of -1 - the defect - while the link back to ClearPlaylist rests on
reading the code. Guarding Reset() would let a test drive the real path.

Decision needed

Guarding Reset() alone is three lines and unblocks the above. Whether the other 18 sites follow,
or the class instead declares that it requires a GUI, is the larger question and is why this is
filed rather than fixed.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: testTest infrastructure and coveragestatus: needs-decisionAn open question blocks the work - do not implement until it is answeredtype: cleanupReadability, consistency, tech debt

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions