Bug 71148 – Use after free in TextTrack::~TextTrack

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

Stack trace:
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%2032/builds/7836/steps/webkit_tests/logs/stdio

[Anna Cavender]

Created attachment 112946 [details]
Patch

[Darin Adler]

Comment on attachment 112946 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:855
> +            m_textTracks.append(textTrack);

This should be textTrack.release() to cut down on reference count churn.

> Source/WebCore/html/HTMLMediaElement.cpp:1993
> +    return textTrack;

This should be textTrack.release() to cut down on reference count churn.

> Source/WebCore/html/HTMLMediaElement.h:62
> +#if ENABLE(VIDEO_TRACK)
> +class HTMLTrackElement;
> +#endif

It's overkill to put class forward declarations inside #if statements. We can just forward declare unconditionally.

> Source/WebCore/html/HTMLTrackElement.cpp:140
> +bool HTMLTrackElement::load(ScriptExecutionContext* context, TextTrackClient* trackClient)

I don’t understand why the function needs to return a bool. The client could easily check the result of track() and figure it out that way.

[Early Warning System Bot]

Comment on attachment 112946 [details]
Patch

Attachment 112946 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10240214

[Anna Cavender]

Comment on attachment 112946 [details]
Patch

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

Thanks Darin!

>> Source/WebCore/html/HTMLMediaElement.cpp:855
>> +            m_textTracks.append(textTrack);
> 
> This should be textTrack.release() to cut down on reference count churn.

Done.

>> Source/WebCore/html/HTMLMediaElement.cpp:1993
>> +    return textTrack;
> 
> This should be textTrack.release() to cut down on reference count churn.

Done.

>> Source/WebCore/html/HTMLMediaElement.h:62
>> +#endif
> 
> It's overkill to put class forward declarations inside #if statements. We can just forward declare unconditionally.

Done.

>> Source/WebCore/html/HTMLTrackElement.cpp:140
>> +bool HTMLTrackElement::load(ScriptExecutionContext* context, TextTrackClient* trackClient)
> 
> I don’t understand why the function needs to return a bool. The client could easily check the result of track() and figure it out that way.

You are right, it does not need to return a bool for now.  There may be cases where we want to return early to avoid loading a second time, but we could add that later.

[Anna Cavender]

Created attachment 112962 [details]
addressing Darin's comments

[Eric Carlson]

Comment on attachment 112962 [details]
addressing Darin's comments

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

r+, but please add the test results before committing.

> LayoutTests/ChangeLog:9
> +        * media/track/track-text-track-destructor-crash.html: Added.
> +

Looks like you forgot to add the expected results.

[Anna Cavender]

Created attachment 112971 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 112971 [details]
Patch for landing

Clearing flags on attachment: 112971

Committed r98807: <http://trac.webkit.org/changeset/98807>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

The test added by this patch is failing on SL:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98807%20(34298)/results.html

Maybe Mac needs some new DRT method?

[Anna Cavender]

Could it just be that a clobber is needed?  The addTrack() function that it is complaining about was added last week:
http://trac.webkit.org/changeset/97926

[Eric Carlson]

In reply to comment #11)
> Could it just be that a clobber is needed?  The addTrack() function that it is complaining about was added last week:
> http://trac.webkit.org/changeset/97926

VIDEO_TRACK is not enabled on the Mac or Windows so the new test should have been added to the skipped files.

[Anna Cavender]

Ah, that was my oversight.  I'll add the test to those skipped files.