Bug 71148 - Use after free in TextTrack::~TextTrack
Summary: Use after free in TextTrack::~TextTrack
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anna Cavender
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-28 15:22 PDT by Anna Cavender
Modified: 2011-10-29 14:17 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.32 KB, patch)
2011-10-28 17:22 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
addressing Darin's comments (6.65 KB, patch)
2011-10-28 19:15 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Patch for landing (7.30 KB, patch)
2011-10-29 08:43 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Anna Cavender 2011-10-28 17:22:52 PDT
Created attachment 112946 [details]
Patch
Comment 2 Darin Adler 2011-10-28 17:28:29 PDT
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.
Comment 3 Early Warning System Bot 2011-10-28 17:49:34 PDT
Comment on attachment 112946 [details]
Patch

Attachment 112946 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10240214
Comment 4 Anna Cavender 2011-10-28 19:07:34 PDT
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.
Comment 5 Anna Cavender 2011-10-28 19:15:10 PDT
Created attachment 112962 [details]
addressing Darin's comments
Comment 6 Eric Carlson 2011-10-28 23:37:03 PDT
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.
Comment 7 Anna Cavender 2011-10-29 08:43:42 PDT
Created attachment 112971 [details]
Patch for landing
Comment 8 WebKit Review Bot 2011-10-29 09:52:39 PDT
Comment on attachment 112971 [details]
Patch for landing

Clearing flags on attachment: 112971

Committed r98807: <http://trac.webkit.org/changeset/98807>
Comment 9 WebKit Review Bot 2011-10-29 09:52:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Ryosuke Niwa 2011-10-29 14:05:04 PDT
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?
Comment 11 Anna Cavender 2011-10-29 14:11:33 PDT
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
Comment 12 Eric Carlson 2011-10-29 14:12:20 PDT
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.
Comment 13 Anna Cavender 2011-10-29 14:17:17 PDT
Ah, that was my oversight.  I'll add the test to those skipped files.