| Summary: | Chrome crashes when attempting to add cue to track element | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Dutton <dutton> | ||||||
| Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | arun.patole, eric.carlson, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Sam Dutton
2012-02-07 02:04:03 PST
(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. |