85926 2012-05-08 15:53:30 -0700 [chromium] Support multiple buffered time ranges 2012-05-09 10:02:13 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 85925 1 fischman fischman eric.carlson eric feature-media-reviews fischman rniwa webkit.review.bot oldest_to_newest 618899 0 fischman 2012-05-08 15:53:30 -0700 RenderMediaControlsChromium.cpp:paintMediaSlider() contains: // FIXME: Draw multiple ranges if there are multiple buffered ranges. (at http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderMediaControlsChromium.cpp&exact_package=chromium&q=file:WebKit%20file:chromium%20file:controls%20file:media%20-file:layouttests&type=cs&l=130) Indeed, fixing chromium's pipeline code to return multiple buffered time ranges (instead of lying and claiming that if the latest buffered byte is at time t, we've buffered all of [0,t) even when a seek might have jumped over the majority of the bytes) results in bogus buffered-area painting, as the first X% of the slider bar is painted when X% of the resource is buffered, without being broken up by range. This is step #1 of bug 85925. 618940 1 140810 fischman 2012-05-08 16:37:08 -0700 Created attachment 140810 Patch 618950 2 fischman 2012-05-08 16:42:13 -0700 eric.carlson: mind taking a look? 618951 3 140810 eric 2012-05-08 16:42:32 -0700 Comment on attachment 140810 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140810&action=review > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:146 > + double fakePercentLoaded = bufferedTimeRanges->end(bufferedTimeRanges->length() - 1, ignoredException) / mediaElement->duration(); So this could cause JavaScript to execute? And thus possibly invalidate pointers? 618960 4 fischman 2012-05-08 16:49:06 -0700 (In reply to comment #3) > (From update of attachment 140810 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140810&action=review > > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:146 > > + double fakePercentLoaded = bufferedTimeRanges->end(bufferedTimeRanges->length() - 1, ignoredException) / mediaElement->duration(); > > So this could cause JavaScript to execute? And thus possibly invalidate pointers? I'm not sure. I was cargo-culting from the definition of percentLoaded() itself (which was being called in the old version). Are you saying this is a problem? 618965 5 eric 2012-05-08 16:52:22 -0700 Any time you have a method which returns an ExceptionCode, you need to be cautious of the fact that that method is a DOM method, and possibly causing JavaScript to execute. In this case, that may not be relevant at all! I don't know what that method looks like. But seeing ExceptionCode does make me wonder. Also, since you're ignoring that exception code, I believe we have a more modern way to do that which will ASSERT in Debug builds if the exception != 0. 618975 6 fischman 2012-05-08 17:01:47 -0700 (In reply to comment #5) > Any time you have a method which returns an ExceptionCode, you need to be cautious of the fact that that method is a DOM method, and possibly causing JavaScript to execute. In this case, that may not be relevant at all! I don't know what that method looks like. But seeing ExceptionCode does make me wonder. > > Also, since you're ignoring that exception code, I believe we have a more modern way to do that which will ASSERT in Debug builds if the exception != 0. I think you mean ASSERT_NO_EXCEPTION. I hope Eric Carlson can speak to whether there is an easy way to see that no exception can happen (and I'll use A_N_E) or whether we just don't care (and that HTMLMediaElement::percentLoaded() is reasonable to ignore exceptions). 618984 7 eric 2012-05-08 17:10:19 -0700 I'm really not trying to spread FUD here. :) Just trying to make sure you're aware of the potential danger. I suspect that this is not infact a method which can execute JavaScript. :) 619145 8 140810 eric.carlson 2012-05-08 21:04:04 -0700 Comment on attachment 140810 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140810&action=review >>> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:146 >>> + double fakePercentLoaded = bufferedTimeRanges->end(bufferedTimeRanges->length() - 1, ignoredException) / mediaElement->duration(); >> >> So this could cause JavaScript to execute? And thus possibly invalidate pointers? > > I'm not sure. I was cargo-culting from the definition of percentLoaded() itself (which was being called in the old version). > Are you saying this is a problem? TimeRanges.end() will only return an exception when the index is out of bounds, which can't happen here. I can not cause script to execute. Is it possible for this be called when duration is 0 or inf? 619552 9 140956 fischman 2012-05-09 09:13:50 -0700 Created attachment 140956 Patch 619553 10 fischman 2012-05-09 09:14:14 -0700 (In reply to comment #8) > Is it possible for this be called when duration is 0 or inf? I'm not sure, but percentLoaded() guards against that, so now I do too. 619602 11 140956 webkit.review.bot 2012-05-09 10:02:07 -0700 Comment on attachment 140956 Patch Clearing flags on attachment: 140956 Committed r116539: <http://trac.webkit.org/changeset/116539> 619604 12 webkit.review.bot 2012-05-09 10:02:13 -0700 All reviewed patches have been landed. Closing bug. 140810 2012-05-08 16:37:08 -0700 2012-05-09 09:13:46 -0700 Patch bug-85926-20120508163707.patch text/plain 2472 fischman U3VidmVyc2lvbiBSZXZpc2lvbjogMTE2Mjk4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDI3MzZkMTEwOWRjNjNh NjlmNzVhYWEwZWVjNjI5YzdkMTFhMDUyMS4uNzU1MTI0ZTM2YjA3NjE5MWQxNDgwNjk5NjljNjU5 NGU1OWZjMjNlNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEyLTA1LTA4ICBBbWkg RmlzY2htYW4gIDxmaXNjaG1hbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW2Nocm9taXVtXSBT dXBwb3J0IG11bHRpcGxlIGJ1ZmZlcmVkIHRpbWUgcmFuZ2VzCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04NTkyNgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFByZXNlcnZlIGV4aXN0aW5nIHJlbmRlcmluZyBv ZiBhIHNpbmdsZSByZWN0IGV2ZW4gaW4gdGhlIHByZXNlbmNlIG9mIG11bHRpcGxlIGJ1ZmZlcmVk IHJlZ2lvbnMuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIGFzIHRoaXMgY2hhbmdlIGhhcyBubyBm dW5jdGlvbmFsIGVmZmVjdHMuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyTWVkaWFDb250 cm9sc0Nocm9taXVtLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OnBhaW50TWVkaWFTbGlkZXIpOgor CiAyMDEyLTA1LTA3ICBDc2FiYSBPc3p0cm9nb27DoWMgIDxvc3N5QHdlYmtpdC5vcmc+CiAKICAg ICAgICAgQnVpbGRmaXggZm9yIGJ1Z2d5IEdDQwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL1JlbmRlck1lZGlhQ29udHJvbHNDaHJvbWl1bS5jcHAgYi9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvUmVuZGVyTWVkaWFDb250cm9sc0Nocm9taXVtLmNwcAppbmRleCA4OTM4M2Mw ZDFhMWEwMzZkZWQzNTE3MjFiMTAwZDBhZjNmMWMyY2MyLi40NjkzZDUyZDY2NjA4Y2VhYjBiMDU0 Yjg3MGRmMzExOWJiMWVjNThlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcv UmVuZGVyTWVkaWFDb250cm9sc0Nocm9taXVtLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5k ZXJpbmcvUmVuZGVyTWVkaWFDb250cm9sc0Nocm9taXVtLmNwcApAQCAtMzMsNiArMzMsNyBAQAog I2luY2x1ZGUgIkhUTUxNZWRpYUVsZW1lbnQuaCIKICNpbmNsdWRlICJIVE1MTmFtZXMuaCIKICNp bmNsdWRlICJQYWludEluZm8uaCIKKyNpbmNsdWRlICJUaW1lUmFuZ2VzLmgiCiAKIG5hbWVzcGFj ZSBXZWJDb3JlIHsKIApAQCAtMTMyLDE0ICsxMzMsMTggQEAgc3RhdGljIGJvb2wgcGFpbnRNZWRp YVNsaWRlcihSZW5kZXJPYmplY3QqIG9iamVjdCwgY29uc3QgUGFpbnRJbmZvJiBwYWludEluZm8s IGMKICAgICBidWZmZXJlZFJlY3QuaW5mbGF0ZSgtc3R5bGUtPmJvcmRlckxlZnRXaWR0aCgpKTsK IAogICAgIGRvdWJsZSBidWZmZXJlZFdpZHRoID0gMC4wOwotICAgIGlmIChtZWRpYUVsZW1lbnQt PnBlcmNlbnRMb2FkZWQoKSA+IDAuMCkgeworICAgIFJlZlB0cjxUaW1lUmFuZ2VzPiBidWZmZXJl ZFRpbWVSYW5nZXMgPSBtZWRpYUVsZW1lbnQtPmJ1ZmZlcmVkKCk7CisgICAgaWYgKGJ1ZmZlcmVk VGltZVJhbmdlcy0+bGVuZ3RoKCkgPiAwKSB7CiAgICAgICAgIC8vIEFjY291bnQgZm9yIHRoZSB3 aWR0aCBvZiB0aGUgc2xpZGVyIHRodW1iLgogICAgICAgICBJbWFnZSogbWVkaWFTbGlkZXJUaHVt YiA9IGdldE1lZGlhU2xpZGVyVGh1bWIoKTsKICAgICAgICAgZG91YmxlIHRodW1iV2lkdGggPSBt ZWRpYVNsaWRlclRodW1iLT53aWR0aCgpIC8gMi4wICsgMS4wOwogICAgICAgICBkb3VibGUgcmVj dFdpZHRoID0gYnVmZmVyZWRSZWN0LndpZHRoKCkgLSB0aHVtYldpZHRoOwogICAgICAgICBpZiAo cmVjdFdpZHRoIDwgMC4wKQogICAgICAgICAgICAgcmVjdFdpZHRoID0gMC4wOwotICAgICAgICBi dWZmZXJlZFdpZHRoID0gcmVjdFdpZHRoICogbWVkaWFFbGVtZW50LT5wZXJjZW50TG9hZGVkKCkg KyB0aHVtYldpZHRoOworICAgICAgICAvLyBQcmVzZXJ2ZSBvbGQgYmVoYXZpb3IgcGVuZGluZyBy ZXNvbHV0aW9uIG9mIFVJIGRlc2lnbiBvZiBtdWx0aXBsZSByYW5nZXMgKHNlZSBGSVhNRSBhYm92 ZSkuCisgICAgICAgIEV4Y2VwdGlvbkNvZGUgaWdub3JlZEV4Y2VwdGlvbjsKKyAgICAgICAgZG91 YmxlIGZha2VQZXJjZW50TG9hZGVkID0gYnVmZmVyZWRUaW1lUmFuZ2VzLT5lbmQoYnVmZmVyZWRU aW1lUmFuZ2VzLT5sZW5ndGgoKSAtIDEsIGlnbm9yZWRFeGNlcHRpb24pIC8gbWVkaWFFbGVtZW50 LT5kdXJhdGlvbigpOworICAgICAgICBidWZmZXJlZFdpZHRoID0gcmVjdFdpZHRoICogZmFrZVBl cmNlbnRMb2FkZWQgKyB0aHVtYldpZHRoOwogICAgIH0KICAgICBidWZmZXJlZFJlY3Quc2V0V2lk dGgoYnVmZmVyZWRXaWR0aCk7CiAK 140956 2012-05-09 09:13:50 -0700 2012-05-09 10:02:07 -0700 Patch bug-85926-20120509091349.patch text/plain 2858 fischman U3VidmVyc2lvbiBSZXZpc2lvbjogMTE2Mjk4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDI3MzZkMTEwOWRjNjNh NjlmNzVhYWEwZWVjNjI5YzdkMTFhMDUyMS4uNzU1MTI0ZTM2YjA3NjE5MWQxNDgwNjk5NjljNjU5 NGU1OWZjMjNlNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEyLTA1LTA4ICBBbWkg RmlzY2htYW4gIDxmaXNjaG1hbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW2Nocm9taXVtXSBT dXBwb3J0IG11bHRpcGxlIGJ1ZmZlcmVkIHRpbWUgcmFuZ2VzCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04NTkyNgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFByZXNlcnZlIGV4aXN0aW5nIHJlbmRlcmluZyBv ZiBhIHNpbmdsZSByZWN0IGV2ZW4gaW4gdGhlIHByZXNlbmNlIG9mIG11bHRpcGxlIGJ1ZmZlcmVk IHJlZ2lvbnMuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIGFzIHRoaXMgY2hhbmdlIGhhcyBubyBm dW5jdGlvbmFsIGVmZmVjdHMuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyTWVkaWFDb250 cm9sc0Nocm9taXVtLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OnBhaW50TWVkaWFTbGlkZXIpOgor CiAyMDEyLTA1LTA3ICBDc2FiYSBPc3p0cm9nb27DoWMgIDxvc3N5QHdlYmtpdC5vcmc+CiAKICAg ICAgICAgQnVpbGRmaXggZm9yIGJ1Z2d5IEdDQwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL1JlbmRlck1lZGlhQ29udHJvbHNDaHJvbWl1bS5jcHAgYi9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvUmVuZGVyTWVkaWFDb250cm9sc0Nocm9taXVtLmNwcAppbmRleCA4OTM4M2Mw ZDFhMWEwMzZkZWQzNTE3MjFiMTAwZDBhZjNmMWMyY2MyLi5jMjgwNDljMmI4Y2JmZDE5NjkyMDBj NmQ1MzdlNjZhYmQ1NjY2NzcyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcv UmVuZGVyTWVkaWFDb250cm9sc0Nocm9taXVtLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5k ZXJpbmcvUmVuZGVyTWVkaWFDb250cm9sc0Nocm9taXVtLmNwcApAQCAtMzMsNiArMzMsNyBAQAog I2luY2x1ZGUgIkhUTUxNZWRpYUVsZW1lbnQuaCIKICNpbmNsdWRlICJIVE1MTmFtZXMuaCIKICNp bmNsdWRlICJQYWludEluZm8uaCIKKyNpbmNsdWRlICJUaW1lUmFuZ2VzLmgiCiAKIG5hbWVzcGFj ZSBXZWJDb3JlIHsKIApAQCAtMTI3LDE5ICsxMjgsMjYgQEAgc3RhdGljIGJvb2wgcGFpbnRNZWRp YVNsaWRlcihSZW5kZXJPYmplY3QqIG9iamVjdCwgY29uc3QgUGFpbnRJbmZvJiBwYWludEluZm8s IGMKICAgICBjb250ZXh0LT5yZXN0b3JlKCk7CiAKICAgICAvLyBEcmF3IHRoZSBidWZmZXJlZCBy YW5nZXMuCi0gICAgLy8gRklYTUU6IERyYXcgbXVsdGlwbGUgcmFuZ2VzIGlmIHRoZXJlIGFyZSBt dWx0aXBsZSBidWZmZXJlZCByYW5nZXMuCisgICAgLy8gRklYTUU6IERyYXcgbXVsdGlwbGUgcmFu Z2VzIGlmIHRoZXJlIGFyZSBtdWx0aXBsZSBidWZmZXJlZCByYW5nZXMuIGh0dHA6Ly93ZWJraXQu b3JnL2IvODU5MjUKICAgICBJbnRSZWN0IGJ1ZmZlcmVkUmVjdCA9IHJlY3Q7CiAgICAgYnVmZmVy ZWRSZWN0LmluZmxhdGUoLXN0eWxlLT5ib3JkZXJMZWZ0V2lkdGgoKSk7CiAKICAgICBkb3VibGUg YnVmZmVyZWRXaWR0aCA9IDAuMDsKLSAgICBpZiAobWVkaWFFbGVtZW50LT5wZXJjZW50TG9hZGVk KCkgPiAwLjApIHsKKyAgICBSZWZQdHI8VGltZVJhbmdlcz4gYnVmZmVyZWRUaW1lUmFuZ2VzID0g bWVkaWFFbGVtZW50LT5idWZmZXJlZCgpOworICAgIGlmIChidWZmZXJlZFRpbWVSYW5nZXMtPmxl bmd0aCgpID4gMCkgewogICAgICAgICAvLyBBY2NvdW50IGZvciB0aGUgd2lkdGggb2YgdGhlIHNs aWRlciB0aHVtYi4KICAgICAgICAgSW1hZ2UqIG1lZGlhU2xpZGVyVGh1bWIgPSBnZXRNZWRpYVNs aWRlclRodW1iKCk7CiAgICAgICAgIGRvdWJsZSB0aHVtYldpZHRoID0gbWVkaWFTbGlkZXJUaHVt Yi0+d2lkdGgoKSAvIDIuMCArIDEuMDsKICAgICAgICAgZG91YmxlIHJlY3RXaWR0aCA9IGJ1ZmZl cmVkUmVjdC53aWR0aCgpIC0gdGh1bWJXaWR0aDsKICAgICAgICAgaWYgKHJlY3RXaWR0aCA8IDAu MCkKICAgICAgICAgICAgIHJlY3RXaWR0aCA9IDAuMDsKLSAgICAgICAgYnVmZmVyZWRXaWR0aCA9 IHJlY3RXaWR0aCAqIG1lZGlhRWxlbWVudC0+cGVyY2VudExvYWRlZCgpICsgdGh1bWJXaWR0aDsK KyAgICAgICAgLy8gUHJlc2VydmUgb2xkIGJlaGF2aW9yIHBlbmRpbmcgcmVzb2x1dGlvbiBvZiBV SSBkZXNpZ24gb2YgbXVsdGlwbGUgcmFuZ2VzIChzZWUgRklYTUUgYWJvdmUpLgorICAgICAgICAv LyBodHRwOi8vd2Via2l0Lm9yZy9iLzg1OTI2CisgICAgICAgIGRvdWJsZSBmYWtlUGVyY2VudExv YWRlZCA9IDA7CisgICAgICAgIGZsb2F0IGR1cmF0aW9uID0gbWVkaWFFbGVtZW50LT5kdXJhdGlv bigpOworICAgICAgICBpZiAoZHVyYXRpb24gJiYgIWlzaW5mKGR1cmF0aW9uKSkKKyAgICAgICAg ICAgIGZha2VQZXJjZW50TG9hZGVkID0gYnVmZmVyZWRUaW1lUmFuZ2VzLT5lbmQoYnVmZmVyZWRU aW1lUmFuZ2VzLT5sZW5ndGgoKSAtIDEsIEFTU0VSVF9OT19FWENFUFRJT04pIC8gZHVyYXRpb247 CisgICAgICAgIGJ1ZmZlcmVkV2lkdGggPSByZWN0V2lkdGggKiBmYWtlUGVyY2VudExvYWRlZCAr IHRodW1iV2lkdGg7CiAgICAgfQogICAgIGJ1ZmZlcmVkUmVjdC5zZXRXaWR0aChidWZmZXJlZFdp ZHRoKTsKIAo=