Bug 85926 – [chromium] Support multiple buffered time ranges

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ami Fischman]

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.

[Ami Fischman]

Created attachment 140810 [details]
Patch

[Ami Fischman]

eric.carlson: mind taking a look?

[Eric Seidel (no email)]

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?

[Ami Fischman]

(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?

[Eric Seidel (no email)]

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.

[Ami Fischman]

(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).

[Eric Seidel (no email)]

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. :)

[Eric Carlson]

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?

[Ami Fischman]

Created attachment 140956 [details]
Patch

[Ami Fischman]

(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.

[WebKit Review Bot]

Comment on attachment 140956 [details]
Patch

Clearing flags on attachment: 140956

Committed r116539: <http://trac.webkit.org/changeset/116539>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.