Bug 77951 – Chrome crashes when attempting to add cue to track element

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Sam Dutton]

Chromium bug filed first: http://code.google.com/p/chromium/issues/detail?id=112789

Chrome Version       : 18.0.1025.6 canary
URLs (if applicable) : http://www.samdutton.com/track/bug/addCue.html
OS version               : <from About This Mac>
Behavior in Safari 3.x/4.x (if applicable):
Behavior in Firefox 3.x (if applicable):
Behavior in Chrome for Windows:

What steps will reproduce the problem?
1. Go to http://www.samdutton.com/track/bug/addCue.html

or...

Run the following code from the Dev Tools console after loading http://www.samdutton.com/track/simple.html:

    var video = document.querySelector("video");	
    var newTrack = video.addTrack("subtitles", "French subtitles", "fr");
    newTrack.mode = 2;
    newTrack.addCue(new TextTrackCue("myId", 0.0, 20.0, "Bonjour!"));

What is the expected result?

Track would be added and cue rendered.


What happens instead?

Tab crashes. 


Comment from  [e-mail] below.


I could repro this issue with 18.0.1025.4 canary on MacOSX 10.7.2.

Here is crash trace:

Thread 0 *CRASHED* ( EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE @ 0x00000004 )

0x014a4e48	 [Google Chrome Framework	 - ../../JavaScriptCore/wtf/Vector.h:514]	WebCore::TextTrackCueList::add
0x014a3031	 [Google Chrome Framework	 - TextTrack.cpp:211]	WebCore::TextTrack::addCue
0x01b0ffab	 [Google Chrome Framework	 - V8TextTrack.cpp:142]	WebCore::TextTrackInternal::addCueCallback
0x00ebf8c1	 [Google Chrome Framework	 - builtins.cc:1220]	v8::internal::Builtin_HandleApiCall
0x5d308735			
0x5d3efdaa			
0x5d31f1b8			
0x5d30b0a9			
0x00eded72	 [Google Chrome Framework	 + 0x00deed72]	v8::internal::Invoke(bool, v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, bool*)
0x00ede9b3	 [Google Chrome Framework	 - execution.cc:170]	v8::internal::Execution::Call
0x00e95efe	 [Google Chrome Framework	 - api.cc:1584]	v8::Script::Run
0x0164403e	 [Google Chrome Framework	 - V8InjectedScriptHostCustom.cpp:82]	WebCore::V8InjectedScriptHost::evaluateCallback
0x00ebf8c1	 [Google Chrome Framework	 - builtins.cc:1220]	v8::internal::Builtin_HandleApiCall
0x5d308735			
0x5d3771e4			
0x5d376ec3			
0x5d376964			
0x5d31f1b8			
0x5d30b0a9			
0x00eded72	 [Google Chrome Framework	 + 0x00deed72]	v8::internal::Invoke(bool, v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, bool*)
0x00ede9b3	 [Google Chrome Framework	 - execution.cc:170]	v8::internal::Execution::Call
0x00e9f1f0	 [Google Chrome Framework	 - api.cc:3603]	v8::Function::Call
0x01615e9b	 [Google Chrome Framework	 - ScriptFunctionCall.cpp:133]	WebCore::ScriptFunctionCall::call
0x017bb887	 [Google Chrome Framework	 - InjectedScript.cpp:211]	WebCore::InjectedScript::makeCall
0x017baece	 [Google Chrome Framework	 - InjectedScript.cpp:226]	WebCore::InjectedScript::makeEvalCall
0x017bae00	 [Google Chrome Framework	 - InjectedScript.cpp:66]	WebCore::InjectedScript::evaluate
0x017fa723	 [Google Chrome Framework	 - InspectorRuntimeAgent.cpp:100]	WebCore::InspectorRuntimeAgent::evaluate
0x01bf5aa2	 [Google Chrome Framework	 - InspectorBackendDispatcher.cpp:994]	WebCore::InspectorBackendDispatcherImpl::Runtime_evaluate
0x01c1356a	 [Google Chrome Framework	 - InspectorBackendDispatcher.cpp:4148]	WebCore::InspectorBackendDispatcherImpl::dispatch
0x017caa9e	 [Google Chrome Framework	 - InspectorController.cpp:329]	WebCore::InspectorController::dispatchMessageFromFrontend
...... (12 stack frames dropped.)
0x9bf3784e	 [CoreFoundation	 + 0x0001184e]	__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
0x9bf37205	 [CoreFoundation	 + 0x00011205]	__CFRunLoopDoSources0
0x9bf610d7	 [CoreFoundation	 + 0x0003b0d7]	__CFRunLoopRun
0x9bf608eb	 [CoreFoundation	 + 0x0003a8eb]	CFRunLoopRunSpecific
0x9bf60797	 [CoreFoundation	 + 0x0003a797]	CFRunLoopRunInMode
0x9225aa7e	 [HIToolbox	 + 0x00002a7e]	RunCurrentEventLoopInMode
0x92261d9a	 [HIToolbox	 + 0x00009d9a]	ReceiveNextEventCommon
0x92261c09	 [HIToolbox	 + 0x00009c09]	BlockUntilNextEventMatchingListInMode
0x92c9f03f	 [AppKit	 + 0x0000a03f]	_DPSNextEvent
0x92c9e8aa	 [AppKit	 + 0x000098aa]	-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
0x92c9ac21	 [AppKit	 + 0x00005c21]	-[NSApplication run]
0x008e88ec	 [Google Chrome Framework	 - message_pump_mac.mm:558]	base::MessagePumpNSApplication::DoRun
0x008e83eb	 [Google Chrome Framework	 - message_pump_mac.mm:179]	base::MessagePumpCFRunLoopBase::Run
0x0091217c	 [Google Chrome Framework	 - message_loop.cc:417]	MessageLoop::Run
0x022a4d27	 [Google Chrome Framework	 - renderer_main.cc:241]	RendererMain
0x008c5a8c	 [Google Chrome Framework	 - content_main.cc:264]	content::ContentMain
0x000f22a8	 [Google Chrome Framework	 - chrome_main.cc:32]	ChromeMain
0x000eaf57	 [Google Chrome Helper	 - chrome_exe_main_mac.cc:16]	main
0x000eaf15	 [Google Chrome Helper	 + 0x00000f15]	start
0x00000007

[Arun Patole]

(In reply to comment #0)
>     var newTrack = video.addTrack("subtitles", "French subtitles", "fr");
>     newTrack.mode = 2;
>     newTrack.addCue(new TextTrackCue("myId", 0.0, 20.0, "Bonjour!"));

addTrack() is now addTextTrack():
https://bugs.webkit.org/show_bug.cgi?id=77381

I think issue is that the text track list of cues is not set to an empty list when new text track is created.

[Arun Patole]

Created attachment 126469 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 126469 [details]
proposed patch

Seems reasonable.  I'm not sure how much it matters if we create this object up-front or not.  How commonly are these cue objects used?

[Eric Carlson]

(In reply to comment #1)
> (In reply to comment #0)
> >     var newTrack = video.addTrack("subtitles", "French subtitles", "fr");
> >     newTrack.mode = 2;
> >     newTrack.addCue(new TextTrackCue("myId", 0.0, 20.0, "Bonjour!"));
> 
> addTrack() is now addTextTrack():
> https://bugs.webkit.org/show_bug.cgi?id=77381
> 
> I think issue is that the text track list of cues is not set to an empty list when new text track is created.

No, the issue is that the TextTrackCueList is not allocated before it is used. Allocating it when the track is created is one option, allocating it just before it is used is another.

[Arun Patole]

> No, the issue is that the TextTrackCueList is not allocated before it is used. Allocating it when the track is created is one option, allocating it just before it is used is another.

Spec says:
The addTextTrack(kind, label, language) method of media elements, when invoked, must run the following steps:
.....
.....
4. Create a new TextTrack object.

5. Create a new text track corresponding to the new object,
....and set its text track list of cues to an empty list...

http://www.whatwg.org/specs/web-apps/current-work/#dom-media-addtexttrack

So, IMO it should set to an empty list when text track is created.

[Eric Carlson]

(In reply to comment #5)
> > No, the issue is that the TextTrackCueList is not allocated before it is used. Allocating it when the track is created is one option, allocating it just before it is used is another.
> 
> Spec says:
> The addTextTrack(kind, label, language) method of media elements, when invoked, must run the following steps:
> .....
> .....
> 4. Create a new TextTrack object.
> 
> 5. Create a new text track corresponding to the new object,
> ....and set its text track list of cues to an empty list...
> 
> http://www.whatwg.org/specs/web-apps/current-work/#dom-media-addtexttrack
> 
> So, IMO it should set to an empty list when text track is created.

We are splitting semantic hairs here, but "and set its text track list of cues to an empty list" it seems to me that 0 is a perfectly valid "empty list". The bug is that the current code tries to *use* the nil ("empty") list.

[Eric Carlson]

Comment on attachment 126469 [details]
proposed patch

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

> Source/WebCore/html/TextTrack.cpp:159
> +    ASSERT(m_cues);

This is unnecessary. Because the list is allocated in the constructor, the only way for this to assert is if the allocation fails. The previous check was because the list was not allocated until it was used.

> Source/WebCore/html/TextTrack.cpp:173
> +    ASSERT(m_cues);

Ditto.

> Source/WebCore/html/TextTrack.cpp:188
> +    ASSERT(m_cues);

Ditto.

> Source/WebCore/html/TextTrack.cpp:226
> +    ASSERT(m_cues);

Ditto.

[Arun Patole]

> We are splitting semantic hairs here, but "and set its text track list of cues to an empty list" it seems to me that 0 is a perfectly valid "empty list". The bug is that the current code tries to *use* the nil ("empty") list.

Hi Eric,
Sorry if i have not understood it correctly, but when I locally reproduced this crash, I found that it was crashing because of null pointer m_cues and not because of the pointer to an empty list. Then I referred spec and found that it should be initialized to an empty list when track is created so i initialized it with TextTrackCueList::create().
"if(m_cues)" checks were removed and asserts were added before it use just to make sure that its non-null and it is not accidently made null at any other place. 
If you still think its wrong I will abandon this patch.

[Eric Carlson]

(In reply to comment #8)
> > We are splitting semantic hairs here, but "and set its text track list of cues to an empty list" it seems to me that 0 is a perfectly valid "empty list". The bug is that the current code tries to *use* the nil ("empty") list.
> 
> Sorry if i have not understood it correctly, but when I locally reproduced this crash, I found that it was crashing because of null pointer m_cues and not because of the pointer to an empty list. Then I referred spec and found that it should be initialized to an empty list when track is created so i initialized it with TextTrackCueList::create().
> "if(m_cues)" checks were removed and asserts were added before it use just to make sure that its non-null and it is not accidently made null at any other place. 
> If you still think its wrong I will abandon this patch.

I was only trying to point out that the way we represent what the spec calls "an empty list" is an implementation detail. The current code *tries* to avoid allocating the list until it is needed, so in the current code nil represents what the spec calls "an empty list". Clearly the current code is wrong, because it will crash if either addCue or removeCue is called. 

One way to fix this is to allocate it in the constructor as you have done. Another way would be to allocate the list in addCue and removeCue. The way you have done it is OK, it makes the code slightly simpler at the cost of more memory when we have a track with no cues. A track will probably have cues more times than not, so the memory saved is negligible.

I don't think the asserts are needed because it is unlikely that the list will be accidentally deleted, and the null checks have been removed so we will now crash immediately. I don't feel strongly about this, you can leave them in if you think they are useful.

[Arun Patole]

(In reply to comment #9)
> (In reply to comment #8)
> > > We are splitting semantic hairs here, but "and set its text track list of cues to an empty list" it seems to me that 0 is a perfectly valid "empty list". The bug is that the current code tries to *use* the nil ("empty") list.
> > 
> > Sorry if i have not understood it correctly, but when I locally reproduced this crash, I found that it was crashing because of null pointer m_cues and not because of the pointer to an empty list. Then I referred spec and found that it should be initialized to an empty list when track is created so i initialized it with TextTrackCueList::create().
> > "if(m_cues)" checks were removed and asserts were added before it use just to make sure that its non-null and it is not accidently made null at any other place. 
> > If you still think its wrong I will abandon this patch.
> 
> I was only trying to point out that the way we represent what the spec calls "an empty list" is an implementation detail. The current code *tries* to avoid allocating the list until it is needed, so in the current code nil represents what the spec calls "an empty list". Clearly the current code is wrong, because it will crash if either addCue or removeCue is called. 
> 
> One way to fix this is to allocate it in the constructor as you have done. Another way would be to allocate the list in addCue and removeCue. The way you have done it is OK, it makes the code slightly simpler at the cost of more memory when we have a track with no cues. A track will probably have cues more times than not, so the memory saved is negligible.
> 
> I don't think the asserts are needed because it is unlikely that the list will be accidentally deleted, and the null checks have been removed so we will now crash immediately. I don't feel strongly about this, you can leave them in if you think they are useful.
Ok thanks, I will upload the modified patch.

[Arun Patole]

(In reply to comment #3)
> (From update of attachment 126469 [details])
> Seems reasonable.  I'm not sure how much it matters if we create this object up-front or not.  How commonly are these cue objects used?

Thanks for r+ Eric, almost all the times you will have tracks with cues so I think it shouldn't be an issue. I will upload the modified patch incorporating Eric Carlson's review comments.

[Arun Patole]

Created attachment 126732 [details]
updated patch

Allocating text track's text track list of cues before using it.

[Eric Carlson]

Comment on attachment 126732 [details]
updated patch

Very nice simplification and refactoring, thanks!

[WebKit Review Bot]

Comment on attachment 126732 [details]
updated patch

Clearing flags on attachment: 126732

Committed r107632: <http://trac.webkit.org/changeset/107632>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.