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 imasaki@chromium.org 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
(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.
Created attachment 126469 [details] proposed patch
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?
(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.
> 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.
(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.
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.
> 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.
(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.
(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.
(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.
Created attachment 126732 [details] updated patch Allocating text track's text track list of cues before using it.
Comment on attachment 126732 [details] updated patch Very nice simplification and refactoring, thanks!
Comment on attachment 126732 [details] updated patch Clearing flags on attachment: 126732 Committed r107632: <http://trac.webkit.org/changeset/107632>
All reviewed patches have been landed. Closing bug.