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.
Created attachment 140810 [details] Patch
eric.carlson: mind taking a look?
Comment on attachment 140810 [details] 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?
(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?
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.
(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).
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. :)
Comment on attachment 140810 [details] 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?
Created attachment 140956 [details] Patch
(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.
Comment on attachment 140956 [details] Patch Clearing flags on attachment: 140956 Committed r116539: <http://trac.webkit.org/changeset/116539>
All reviewed patches have been landed. Closing bug.