Bug 168404 – REGRESSION (r212311): NULL-dereference in HTMLMediaElement::prepareToPlay()

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

REGRESSION (r212311): NULL-dereference in HTMLMediaElement::prepareToPlay()

[Jer Noble]

Created attachment 301691 [details]
Patch

[Jer Noble]

rdar://problem/30547188

[Brian Weinstein]

Comment on attachment 301691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301691&action=review

> Source/WebCore/ChangeLog:10
> +        prepareToPlay().  r212311 began calling prepareToPlay() on a subsequent run-loop iteration

Extra space between sentences.

[Brian Weinstein]

Comment on attachment 301691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301691&action=review

>> Source/WebCore/ChangeLog:10
>> +        prepareToPlay().  r212311 began calling prepareToPlay() on a subsequent run-loop iteration
> 
> Extra space between sentences.

Extra space between sentences.

[Jer Noble]

Committed r212420: <http://trac.webkit.org/changeset/212420>

[Alexey Proskuryakov]

Comment on attachment 301691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301691&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:2562
>      m_havePreparedToPlay = true;
> -    m_player->prepareToPlay();
> +    if (m_player)
> +        m_player->prepareToPlay();

Shouldn't setting m_havePreparedToPlay be inside the "if" too?

This looks like it fixes the crash, but keeps the incorrect behavior.

[Jer Noble]

(In reply to comment #6)
> Comment on attachment 301691 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301691&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:2562
> >      m_havePreparedToPlay = true;
> > -    m_player->prepareToPlay();
> > +    if (m_player)
> > +        m_player->prepareToPlay();
> 
> Shouldn't setting m_havePreparedToPlay be inside the "if" too?
> 
> This looks like it fixes the crash, but keeps the incorrect behavior.

No. The behavior is correct. If m_havePreparedToPlay is set, we will call m_player->prepareToPlay() next time the MediaPlayer is created.

[Jer Noble]

(In reply to comment #7)
> (In reply to comment #6)
> > Comment on attachment 301691 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=301691&action=review
> > 
> > > Source/WebCore/html/HTMLMediaElement.cpp:2562
> > >      m_havePreparedToPlay = true;
> > > -    m_player->prepareToPlay();
> > > +    if (m_player)
> > > +        m_player->prepareToPlay();
> > 
> > Shouldn't setting m_havePreparedToPlay be inside the "if" too?
> > 
> > This looks like it fixes the crash, but keeps the incorrect behavior.
> 
> No. The behavior is correct. If m_havePreparedToPlay is set, we will call
> m_player->prepareToPlay() next time the MediaPlayer is created.

Double-checking; it's actually the opposite. m_havePreparedToPlay is cleared whenever a MediaPlayer is created. Same deal though: the behavior is exactly the same whether setting m_havePreparedToPlay is inside the null check our outside.

[Alexey Proskuryakov]

Hmm.

If the behaviour is correct, then the naming is not. The code says "have prepared to play" without doing the "prepare to play" now. It used to be straightforward before the fix, but now it immediately raises a red flag.

[Simon Fraser (smfr)]

Is this exercised by tests?

[Jer Noble]

(In reply to comment #10)
> Is this exercised by tests?

This code is hit by every autoplay test.