Fix potential null pointer access in HTMLMediaElement::seek

third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1671 matching lines @@
     WTF_LOG(Media, "HTMLMediaElement::setIgnorePreloadNone(%p)", this);
     m_ignorePreloadNone = true;
     setPlayerPreload();
 }
 
 void HTMLMediaElement::seek(double time)
 {
     WTF_LOG(Media, "HTMLMediaElement::seek(%p, %f)", this, time);
 
     // 2 - If the media element's readyState is HAVE_NOTHING, abort these steps.
     if (m_readyState == HAVE_NOTHING)

[liberato (no reviews please), 2016/04/06 14:58:05]

would it be better to early out here on !webMediaPlayer()?  i'm not sure if the
side-effects of setIgnorePreloadNone are intended in this case anyway.

i'll defer to kbr and philipj.

[philipj_slow, 2016/04/06 15:08:01]

I think so, yes. Also duplicate the FIXME from HTMLMediaElement::duration if you
take this route.

         return;
 
     // Ignore preload none and start load if necessary.
     setIgnorePreloadNone();
 
     // Get the current time before setting m_seeking, m_lastSeekTime is returned once it is set.
     refreshCachedTime();
     // This is needed to avoid getting default playback start position from currentTime().
     double now = m_cachedTime;
 
@@ skipping 11 matching lines @@
     time = std::min(time, duration());
 
     // 7 - If the new playback position is less than the earliest possible position, let it be that position instead.
     time = std::max(time, 0.0);
 
     // Ask the media engine for the time value in the movie's time scale before comparing with current time. This
     // is necessary because if the seek time is not equal to currentTime but the delta is less than the movie's
     // time scale, we will ask the media engine to "seek" to the current movie time, which may be a noop and
     // not generate a timechanged callback. This means m_seeking will never be cleared and we will never
     // fire a 'seeked' event.
-    double mediaTime = webMediaPlayer()->mediaTimeForTimeValue(time);
+    double mediaTime = webMediaPlayer() ? webMediaPlayer()->mediaTimeForTimeValue(time) : 0.0;
     if (time != mediaTime) {
         WTF_LOG(Media, "HTMLMediaElement::seek(%p, %f) - media timeline equivalent is %f", this, time, mediaTime);
         time = mediaTime;
     }
 
     // 8 - If the (possibly now changed) new playback position is not in one of the ranges given in the
     // seekable attribute, then let it be the position in one of the ranges given in the seekable attribute
     // that is the nearest to the new playback position. ... If there are no ranges given in the seekable
     // attribute then set the seeking IDL attribute to false and abort these steps.
     TimeRanges* seekableRanges = seekable();
 
-    if (!seekableRanges->length()) {
+    if (!webMediaPlayer() || !seekableRanges->length()) {
         m_seeking = false;
         return;
     }
     time = seekableRanges->nearest(time, now);
 
     if (m_playing && m_lastSeekTime < now)
         addPlayedRange(m_lastSeekTime, now);
 
     m_lastSeekTime = time;
     m_sentEndEvent = false;
@@ skipping 383 matching lines @@
         scheduleEvent(EventTypeNames::pause);
         scheduleRejectPlayPromises(AbortError);
     }
 
     updatePlayState();
 }
 
 void HTMLMediaElement::requestRemotePlayback()
 {
     ASSERT(m_remoteRoutesAvailable);
-    webMediaPlayer()->requestRemotePlayback();
+    if (webMediaPlayer())

[philipj_slow, 2016/04/06 15:08:01]

Have you seen a crash here? I'd like to avoid spreading the hack any further
than necessary, so the same question applies below too.

[pavor, 2016/05/12 12:03:36]

I have only seen a crash inside seek. Because I haven't figured out the reason,
and there is a lot of discrepancy in the rest of the file whether to check it or
not, and even how to access m_webMediaPlayer pointer (directly as a field or
with webMediaPlayer()), I decided to leave without checks only
WebMediaPlayerClient overrides, which appear to be called from webMediaPlayer
itself.

[liberato (no reviews please), 2016/05/18 17:27:54]

do you see these crashes only on android?  can you tell if the player is src= or
mse?

some non-trivial work happens in WebMediaPlayerAndroid's (WMPA) destructor.  it
notifies |media_source_delegate_| to stop, which in turn does things like talks
to the MSE demuxer.  If some callback into WMPA happens during that process,
such as UpdateReadyState, then i could see WMPA calling back into
HTMLMediaElement and resetting the ready state.

however, that's all conjecture on my part.  i have no idea if those callbacks
happen, or if the ready state is reset in HTMLMediaElement before or after the
media player is cleared.

destroying the weak refs explicitly in WMPA::~WMPA would prevent it, if that's
true.

+        webMediaPlayer()->requestRemotePlayback();
     Platform::current()->recordAction(UserMetricsAction("Media_RequestRemotePlayback"));
 }
 
 void HTMLMediaElement::requestRemotePlaybackControl()
 {
     ASSERT(m_remoteRoutesAvailable);
-    webMediaPlayer()->requestRemotePlaybackControl();
+    if (webMediaPlayer())
+        webMediaPlayer()->requestRemotePlaybackControl();
     Platform::current()->recordAction(UserMetricsAction("Media_RequestRemotePlayback_Control"));
 }
 
 void HTMLMediaElement::closeMediaSource()
 {
     if (!m_mediaSource)
         return;
 
     m_mediaSource->close();
     m_mediaSource = nullptr;
@@ skipping 188 matching lines @@
 
 void HTMLMediaElement::audioTracksTimerFired(Timer<HTMLMediaElement>*)
 {
     Vector<WebMediaPlayer::TrackId> enabledTrackIds;
     for (unsigned i = 0; i < audioTracks().length(); ++i) {
         AudioTrack* track = audioTracks().anonymousIndexedGetter(i);
         if (track->enabled())
             enabledTrackIds.append(track->trackId());
     }
 
-    webMediaPlayer()->enabledAudioTracksChanged(enabledTrackIds);
+    if (webMediaPlayer())
+        webMediaPlayer()->enabledAudioTracksChanged(enabledTrackIds);
 }
 
 WebMediaPlayer::TrackId HTMLMediaElement::addAudioTrack(const WebString& id, WebMediaPlayerClient::AudioTrackKind kind, const WebString& label, const WebString& language, bool enabled)
 {
     AtomicString kindString = AudioKindToString(kind);
     WTF_LOG(Media, "HTMLMediaElement::addAudioTrack(%p, '%s', '%s', '%s', '%s', %d)",
         this, id.utf8().data(), kindString.ascii().data(), label.utf8().data(), language.utf8().data(), enabled);
 
     if (!RuntimeEnabledFeatures::audioVideoTracksEnabled())
         return 0;
@@ skipping 23 matching lines @@
 void HTMLMediaElement::selectedVideoTrackChanged(WebMediaPlayer::TrackId* selectedTrackId)
 {
     WTF_LOG(Media, "HTMLMediaElement::selectedVideoTrackChanged(%p)", this);
     ASSERT(RuntimeEnabledFeatures::audioVideoTracksEnabled());
 
     if (selectedTrackId)
         videoTracks().trackSelected(*selectedTrackId);
 
     // FIXME: Add call on m_mediaSource to notify it of track changes once the SourceBuffer.videoTracks attribute is added.
 
-    webMediaPlayer()->selectedVideoTrackChanged(selectedTrackId);
+    if (webMediaPlayer())
+        webMediaPlayer()->selectedVideoTrackChanged(selectedTrackId);
 }
 
 WebMediaPlayer::TrackId HTMLMediaElement::addVideoTrack(const WebString& id, WebMediaPlayerClient::VideoTrackKind kind, const WebString& label, const WebString& language, bool selected)
 {
     AtomicString kindString = VideoKindToString(kind);
     WTF_LOG(Media, "HTMLMediaElement::addVideoTrack(%p, '%s', '%s', '%s', '%s', %d)",
         this, id.utf8().data(), kindString.ascii().data(), label.utf8().data(), language.utf8().data(), selected);
 
     if (!RuntimeEnabledFeatures::audioVideoTracksEnabled())
         return 0;
@@ skipping 1471 matching lines @@
 }
 
 #if !ENABLE(OILPAN)
 WeakPtr<HTMLMediaElement> HTMLMediaElement::createWeakPtr()
 {
     return m_weakPtrFactory.createWeakPtr();
 }
 #endif
 
 } // namespace blink