Stack trace: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%2032/builds/7836/steps/webkit_tests/logs/stdio
Created attachment 112946 [details] Patch
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 on attachment 112946 [details] Patch Attachment 112946 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10240214
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.
Created attachment 112962 [details] addressing Darin's comments
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.
Created attachment 112971 [details] Patch for landing
Comment on attachment 112971 [details] Patch for landing Clearing flags on attachment: 112971 Committed r98807: <http://trac.webkit.org/changeset/98807>
All reviewed patches have been landed. Closing bug.
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?
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
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.
Ah, that was my oversight. I'll add the test to those skipped files.