117815 2013-06-19 17:21:50 -0700 Fix TextTrackCue::cueIndex() to handle the null case of TextTrack::cues(() properly 2013-07-15 12:42:07 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED BlinkMergeCandidate, InRadar P2 Normal --- 0 ruthiecftg webkit-unassigned commit-queue eric.carlson esprehn+autocc jer.noble jonlee rniwa ruthiecftg thorton vcarbune webkit-bug-importer oldest_to_newest 901962 0 ruthiecftg 2013-06-19 17:21:50 -0700 TextTrackCue::cueIndex() calls TextTrackCueList::getCueIndex on the return value of TextTrack::cues() without checking if that return value is null. 901964 1 webkit-bug-importer 2013-06-19 17:22:22 -0700 <rdar://problem/14211041> 901967 2 ruthiecftg 2013-06-19 17:27:19 -0700 Jer discovered this bug when commit-queue was failing media/track/media-element-enqueue-event-crash.html on a patch for bug 117765. The stack information for the crashed test can be found there. 902389 3 205137 ruthiecftg 2013-06-20 19:16:28 -0700 Created attachment 205137 Patch 902390 4 205137 rniwa 2013-06-20 19:21:57 -0700 Comment on attachment 205137 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205137&action=review > Source/WebCore/ChangeLog:3 > + <rdar://problem/14211041> Fix TextTrackCue::cueIndex() to handle the null case of TextTrack::cues(() properly (117815) Please list the radar URL in a separate line. Also, there is no need to repeat the bug number here. Simply copy & paste the bug title. > Source/WebCore/ChangeLog:8 > + This patch adds assert statements to try to catch when the call s/assert statements/assertions/ It's probably redundant to say "the call" since "track()->cues()" is a function call already. 902399 5 205141 ruthiecftg 2013-06-20 19:35:54 -0700 Created attachment 205141 Patch 902616 6 205141 commit-queue 2013-06-21 11:08:32 -0700 Comment on attachment 205141 Patch Clearing flags on attachment: 205141 Committed r151845: <http://trac.webkit.org/changeset/151845> 902617 7 commit-queue 2013-06-21 11:08:36 -0700 All reviewed patches have been landed. Closing bug. 905199 8 ruthiecftg 2013-07-01 18:34:40 -0700 Consider merging: https://src.chromium.org/viewvc/blink?revision=153206&view=revision 905200 9 ruthiecftg 2013-07-01 18:37:57 -0700 (In reply to comment #8) > Consider merging: https://src.chromium.org/viewvc/blink?revision=153206&view=revision Could there ever be a case in which tracks() is null? 905268 10 jonlee 2013-07-01 23:52:04 -0700 (In reply to comment #9) > (In reply to comment #8) > > Consider merging: https://src.chromium.org/viewvc/blink?revision=153206&view=revision > > Could there ever be a case in which tracks() is null? According to that change list: However, per HTML rules, a track object returns a non-NULL cue list object only if cues haven't been disabled. The problem is that in the case for an inband text track, the cues are disabled by default, and so the track object returns NULL as the value of the cue list pointer. As currently implemented, the text track cue object assumes that the track's cue list is always non-NULL, but this crashes the browser when the pointer value is dereferenced to get the cue index. Need to verify this is the case; if so, we should probably file a new bug. 905481 11 ruthiecftg 2013-07-02 10:16:10 -0700 (In reply to comment #9) > (In reply to comment #8) > > Consider merging: https://src.chromium.org/viewvc/blink?revision=153206&view=revision > > Could there ever be a case in which tracks() is null? track() not tracks() and I think yes. From W3 spec on TextTrackCue: "The track attribute, on getting, must return the TextTrack object of the text track in whose list of cues the text track cue that the TextTrackCue object represents finds itself, if any; or null otherwise." (http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#texttrackcue) Also, by inspection of the implementation of TextTrackCue::track(), the getter returns m_track, which is a pointer to a TextTrack object and initialized to null in the constructor. If so, then track() should first be checked for a null value before track()->cues(). In the case where track() is null, would it also be appropriate to return invalidCueIndex? Or is this a case that really should never be hit? 905483 12 ruthiecftg 2013-07-02 10:32:15 -0700 (In reply to comment #10) > According to that change list: > > However, per HTML rules, a track object returns a non-NULL > cue list object only if cues haven't been disabled. The > problem is that in the case for an inband text track, the > cues are disabled by default, and so the track object > returns NULL as the value of the cue list pointer. As > currently implemented, the text track cue object assumes > that the track's cue list is always non-NULL, but this > crashes the browser when the pointer value is dereferenced > to get the cue index. > > Need to verify this is the case; if so, we should probably file a new bug. From whatWG spec (http://www.whatwg.org/specs/web-apps/current-work/#dom-texttrack-cues): "If the text track mode of the text track that the TextTrack object represents is not the text track disabled mode, then the cues attribute must return a live TextTrackCueList object that represents the subset of the text track list of cues of the text track that the TextTrack object represents whose end times occur at or after the earliest possible position when the script started, in text track cue order. Otherwise, it must return null. When an object is returned, the same object must be returned each time." From w3 spec (http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#text-track-disabled): "Disabled: Indicates that the text track is not active. Other than for the purposes of exposing the track in the DOM, the user agent is ignoring the text track. No cues are active, no events are fired, and the user agent will not attempt to obtain the track's cues." The last part of the w3 definition makes me a bit wary ("the user agent will not attempt to obtain the track's cues"), but the chromium patch makes sense in terms of whatWG's spec. 906840 13 206253 ruthiecftg 2013-07-08 09:41:39 -0700 Created attachment 206253 Patch 906876 14 206253 commit-queue 2013-07-08 11:34:24 -0700 Comment on attachment 206253 Patch Clearing flags on attachment: 206253 Committed r152459: <http://trac.webkit.org/changeset/152459> 906877 15 commit-queue 2013-07-08 11:34:27 -0700 All reviewed patches have been landed. Closing bug. 908127 16 rniwa 2013-07-12 21:16:43 -0700 Why was test case not added? Did you try using test.mp4 instead of test.ogv? 908145 17 ruthiecftg 2013-07-13 00:22:06 -0700 (In reply to comment #16) > Why was test case not added? Did you try using test.mp4 instead of test.ogv? The test hits a case that isn't solved by this patch: https://chromiumcodereview.appspot.com/17002002#msg31 We should consider merging this updated patch: https://src.chromium.org/viewvc/blink?revision=153810&view=revision 908395 18 eric.carlson 2013-07-15 07:43:46 -0700 (In reply to comment #16) > Why was test case not added? Did you try using test.mp4 instead of test.ogv? Because the test case is worthless, see my comment here: https://chromiumcodereview.appspot.com/17002002#msg29 (In reply to comment #17) > (In reply to comment #16) > > Why was test case not added? Did you try using test.mp4 instead of test.ogv? > > The test hits a case that isn't solved by this patch: https://chromiumcodereview.appspot.com/17002002#msg31 > > We should consider merging this updated patch: https://src.chromium.org/viewvc/blink?revision=153810&view=revision Agreed. 908486 19 eric.carlson 2013-07-15 12:42:07 -0700 (In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > Why was test case not added? Did you try using test.mp4 instead of test.ogv? > > > > The test hits a case that isn't solved by this patch: https://chromiumcodereview.appspot.com/17002002#msg31 > > > > We should consider merging this updated patch: https://src.chromium.org/viewvc/blink?revision=153810&view=revision > > Agreed. Filed https://bugs.webkit.org/show_bug.cgi?id=118682 for merging the Blink changes. 205137 2013-06-20 19:16:28 -0700 2013-06-20 19:35:52 -0700 Patch bug-117815-20130620191437.patch text/plain 1376 ruthiecftg SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1MTgxMikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEzLTA2LTIwICBSdXRoIEZv bmcgIDxydXRoX2ZvbmdAYXBwbGUuY29tPgorCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8xNDIx MTA0MT4gRml4IFRleHRUcmFja0N1ZTo6Y3VlSW5kZXgoKSB0byBoYW5kbGUgdGhlIG51bGwgY2Fz ZSBvZiBUZXh0VHJhY2s6OmN1ZXMoKCkgcHJvcGVybHkgKDExNzgxNSkKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExNzgxNQorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoaXMgcGF0Y2ggYWRkcyBhc3NlcnQg c3RhdGVtZW50cyB0byB0cnkgdG8gY2F0Y2ggd2hlbiB0aGUgY2FsbAorICAgICAgICB0cmFjaygp LT5jdWVzKCkgcmV0dXJucyBhIG51bGwgcG9pbnRlci4KKworICAgICAgICAqIGh0bWwvdHJhY2sv VGV4dFRyYWNrQ3VlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlRleHRUcmFja0N1ZTo6Y3VlSW5k ZXgpOgorCiAyMDEzLTA2LTIwICBBbGV4ZXkgUHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgog CiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTY0OTUK SW5kZXg6IFNvdXJjZS9XZWJDb3JlL2h0bWwvdHJhY2svVGV4dFRyYWNrQ3VlLmNwcAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9odG1sL3RyYWNrL1RleHRUcmFja0N1ZS5jcHAJKHJldmlz aW9uIDE1MTgxMikKKysrIFNvdXJjZS9XZWJDb3JlL2h0bWwvdHJhY2svVGV4dFRyYWNrQ3VlLmNw cAkod29ya2luZyBjb3B5KQpAQCAtNDgzLDggKzQ4MywxMSBAQCB2b2lkIFRleHRUcmFja0N1ZTo6 c2V0VGV4dChjb25zdCBTdHJpbmcmCiAKIGludCBUZXh0VHJhY2tDdWU6OmN1ZUluZGV4KCkKIHsK LSAgICBpZiAobV9jdWVJbmRleCA9PSBpbnZhbGlkQ3VlSW5kZXgpCisgICAgaWYgKG1fY3VlSW5k ZXggPT0gaW52YWxpZEN1ZUluZGV4KSB7CisgICAgICAgIEFTU0VSVCh0cmFjaygpKTsKKyAgICAg ICAgQVNTRVJUKHRyYWNrKCktPmN1ZXMoKSk7CiAgICAgICAgIG1fY3VlSW5kZXggPSB0cmFjaygp LT5jdWVzKCktPmdldEN1ZUluZGV4KHRoaXMpOworICAgIH0KIAogICAgIHJldHVybiBtX2N1ZUlu ZGV4OwogfQo= 205141 2013-06-20 19:35:54 -0700 2013-07-08 09:41:36 -0700 Patch bug-117815-20130620193404.patch text/plain 1369 ruthiecftg SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1MTgxMikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDEzLTA2LTIwICBSdXRoIEZv bmcgIDxydXRoX2ZvbmdAYXBwbGUuY29tPgorCisgICAgICAgIEZpeCBUZXh0VHJhY2tDdWU6OmN1 ZUluZGV4KCkgdG8gaGFuZGxlIHRoZSBudWxsIGNhc2Ugb2YgVGV4dFRyYWNrOjpjdWVzKCgpIHBy b3Blcmx5CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MTc4MTUKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzE0MjExMDQxPiAKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIHBhdGNoIGFkZHMgYXNzZXJ0 IHN0YXRlbWVudHMgdG8gdHJ5IHRvIGNhdGNoIHdoZW4gCisgICAgICAgIHRyYWNrKCktPmN1ZXMo KSByZXR1cm5zIGEgbnVsbCBwb2ludGVyLgorCisgICAgICAgICogaHRtbC90cmFjay9UZXh0VHJh Y2tDdWUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6VGV4dFRyYWNrQ3VlOjpjdWVJbmRleCk6CisK IDIwMTMtMDYtMjAgIEFsZXhleSBQcm9za3VyeWFrb3YgIDxhcEBhcHBsZS5jb20+CiAKICAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExNjQ5NQpJbmRleDog U291cmNlL1dlYkNvcmUvaHRtbC90cmFjay9UZXh0VHJhY2tDdWUuY3BwCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9XZWJDb3JlL2h0bWwvdHJhY2svVGV4dFRyYWNrQ3VlLmNwcAkocmV2aXNpb24gMTUx ODEyKQorKysgU291cmNlL1dlYkNvcmUvaHRtbC90cmFjay9UZXh0VHJhY2tDdWUuY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC00ODMsOCArNDgzLDExIEBAIHZvaWQgVGV4dFRyYWNrQ3VlOjpzZXRUZXh0 KGNvbnN0IFN0cmluZyYKIAogaW50IFRleHRUcmFja0N1ZTo6Y3VlSW5kZXgoKQogewotICAgIGlm IChtX2N1ZUluZGV4ID09IGludmFsaWRDdWVJbmRleCkKKyAgICBpZiAobV9jdWVJbmRleCA9PSBp bnZhbGlkQ3VlSW5kZXgpIHsKKyAgICAgICAgQVNTRVJUKHRyYWNrKCkpOworICAgICAgICBBU1NF UlQodHJhY2soKS0+Y3VlcygpKTsKICAgICAgICAgbV9jdWVJbmRleCA9IHRyYWNrKCktPmN1ZXMo KS0+Z2V0Q3VlSW5kZXgodGhpcyk7CisgICAgfQogCiAgICAgcmV0dXJuIG1fY3VlSW5kZXg7CiB9 Cg== 206253 2013-07-08 09:41:39 -0700 2013-07-08 11:34:24 -0700 Patch bug-117815-20130708093935.patch text/plain 1672 ruthiecftg SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1MjQ1NSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDEzLTA3LTA4ICBSdXRoIEZv bmcgIDxydXRoX2ZvbmdAYXBwbGUuY29tPgorCisgICAgICAgIEZpeCBUZXh0VHJhY2tDdWU6OmN1 ZUluZGV4KCkgdG8gaGFuZGxlIHRoZSBudWxsIGNhc2Ugb2YgVGV4dFRyYWNrOjpjdWVzKCgpIHBy b3Blcmx5CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MTc4MTUKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzE0MjExMDQxPgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE1lcmdlIGh0dHBzOi8vc3JjLmNocm9t aXVtLm9yZy92aWV3dmMvYmxpbms/cmV2aXNpb249MTUzMjA2JnZpZXc9cmV2aXNpb24KKyAgICAg ICAgCisgICAgICAgIE5vIG5ldyB0ZXN0cywgb25seSBiZWhhdmlvciBjaGFuZ2UgaXMgcHJldmVu dGluZyBhIGNyYXNoIGluIHRoZSByZWxlYXNlIGJ1aWxkLgorICAgICAgICBjdWVJbmRleCgpIGlz IG9ubHkgdXNlZCBmb3Igc29ydGluZyBjdWVzLCBzbyByZXR1cm5pbmcgYW4gaW52YWxpZEN1ZUlu ZGV4IGluIHRoZSBjYXNlCisgICAgICAgIHdoZXJlIHRyYWNrKCktPmN1ZXMoKSBpcyBudWxsIG1h a2VzIHNlbnNlIGFuZCB3b24ndCBtb2RpZnkgYmVoYXZpb3IuCisKKyAgICAgICAgKiBodG1sL3Ry YWNrL1RleHRUcmFja0N1ZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpUZXh0VHJhY2tDdWU6OmN1 ZUluZGV4KToKKwogMjAxMy0wNy0wOCAgQW5kcmVhcyBLbGluZyAgPGFrbGluZ0BhcHBsZS5jb20+ CiAKICAgICAgICAgUkVHUkVTU0lPTihyMTI1Mjk0KTogQSBzdHlsZSBydWxlIHdpdGggbW9yZSB0 aGFuIDgxOTIgc2VsZWN0b3JzIGNhbiBjYXVzZSBzdHlsZSBjb3JydXB0aW9uLgpJbmRleDogU291 cmNlL1dlYkNvcmUvaHRtbC90cmFjay9UZXh0VHJhY2tDdWUuY3BwCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9XZWJDb3JlL2h0bWwvdHJhY2svVGV4dFRyYWNrQ3VlLmNwcAkocmV2aXNpb24gMTUyMzcy KQorKysgU291cmNlL1dlYkNvcmUvaHRtbC90cmFjay9UZXh0VHJhY2tDdWUuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC00ODYsNyArNDg2LDggQEAgaW50IFRleHRUcmFja0N1ZTo6Y3VlSW5kZXgoKQog ICAgIGlmIChtX2N1ZUluZGV4ID09IGludmFsaWRDdWVJbmRleCkgewogICAgICAgICBBU1NFUlQo dHJhY2soKSk7CiAgICAgICAgIEFTU0VSVCh0cmFjaygpLT5jdWVzKCkpOwotICAgICAgICBtX2N1 ZUluZGV4ID0gdHJhY2soKS0+Y3VlcygpLT5nZXRDdWVJbmRleCh0aGlzKTsKKyAgICAgICAgaWYg KFRleHRUcmFja0N1ZUxpc3QqIGN1ZUxpc3QgPSB0cmFjaygpLT5jdWVzKCkpCisgICAgICAgICAg ICBtX2N1ZUluZGV4ID0gY3VlTGlzdC0+Z2V0Q3VlSW5kZXgodGhpcyk7CiAgICAgfQogCiAgICAg cmV0dXJuIG1fY3VlSW5kZXg7Cg==