Summary: | [MSE] Infinite loop in sample eviction when duration is NaN | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||||
Component: | Media | Assignee: | Philippe Normand <pnormand> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, calvaris, cdumez, cmarcelo, darin, eocanha, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=218381 | ||||||||||||||||
Attachments: |
|
Description
Philippe Normand
2020-10-27 05:20:22 PDT
Created attachment 412413 [details]
Patch
I'm not sure why the new test would trigger a new failure in another test. Ha might be related with the QuotaExceeded support, I'll check it. Created attachment 412436 [details]
Patch
Comment on attachment 412436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412436&action=review > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999 > + if (!rangeEnd.hasDoubleValue()) { I don't think this test is ok. In the case you are dealing with, I guess rangeEnd should be positiveInfinite so you should check for isPositiveInfinite. Bonus points to assert on validity, which I guess should be true. Comment on attachment 412436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412436&action=review >> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999 >> + if (!rangeEnd.hasDoubleValue()) { > > I don't think this test is ok. In the case you are dealing with, I guess rangeEnd should be positiveInfinite so you should check for isPositiveInfinite. Bonus points to assert on validity, which I guess should be true. checking isPositiveInfinite && !rangeEnd.isIndefinite() makes sense. I'm not sure about adding an assert though, by default the duration is invalidTime(), how can we be sure that ever changes in this part of the code? Created attachment 412531 [details]
Patch
Created attachment 412535 [details]
Patch
Comment on attachment 412535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412535&action=review > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999 > + if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) { Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log? Comment on attachment 412535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412535&action=review >> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999 >> + if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) { > > Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log? Does the test cover all three of these, and negative infinity too? Comment on attachment 412535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412535&action=review >>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999 >>> + if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) { >> >> Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log? > > Does the test cover all three of these, and negative infinity too? NaN is what's mentioned in the spec: https://www.w3.org/TR/media-source/#dom-mediasource-duration So perhaps there's no need to check infinite cases here. Created attachment 412646 [details]
Patch
Comment on attachment 412646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412646&action=review > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1000 > + if (rangeEnd.isInvalid() || rangeEnd.isIndefinite()) { > + rangeEnd = buffered.maximumBufferedTime(); I'm not sure this covers all the possible cases; it sounds like MediaTime needs an "isFinite()" that returns false for invalid, indefinite, and infinite values. (The Cocoa ports, for example, treat "Indefinite" differently than "Positive Infinite") (In reply to Philippe Normand from comment #11) > Comment on attachment 412535 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=412535&action=review > > >>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999 > >>> + if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) { > >> > >> Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log? > > > > Does the test cover all three of these, and negative infinity too? > > NaN is what's mentioned in the spec: > https://www.w3.org/TR/media-source/#dom-mediasource-duration > So perhaps there's no need to check infinite cases here. It's initially set to NaN, but it's also a read-write unrestricted double, so the page can set the duration to positive infinite. (In reply to Jer Noble from comment #13) > Comment on attachment 412646 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=412646&action=review > > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1000 > > + if (rangeEnd.isInvalid() || rangeEnd.isIndefinite()) { > > + rangeEnd = buffered.maximumBufferedTime(); > > I'm not sure this covers all the possible cases; it sounds like MediaTime > needs an "isFinite()" that returns false for invalid, indefinite, and > infinite values. (The Cocoa ports, for example, treat "Indefinite" > differently than "Positive Infinite") Sounds good. I guess you meant to r- instead of clearing the r flag :) (In reply to Jer Noble from comment #14) > (In reply to Philippe Normand from comment #11) > > Comment on attachment 412535 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=412535&action=review > > > > >>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999 > > >>> + if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) { > > >> > > >> Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log? > > > > > > Does the test cover all three of these, and negative infinity too? > > > > NaN is what's mentioned in the spec: > > https://www.w3.org/TR/media-source/#dom-mediasource-duration > > So perhaps there's no need to check infinite cases here. > > It's initially set to NaN, but it's also a read-write unrestricted double, > so the page can set the duration to positive infinite. So a test for Infinite time is needed too? (In reply to Philippe Normand from comment #15) > > Sounds good. I guess you meant to r- instead of clearing the r flag :) Whoops! I did. I initially had it set to r+ until I noticed the unrestricted double thing, and mistakenly set it to empty rather than back to r-. Apologies! > So a test for Infinite time is needed too? Yes, I think so. Either explicitly in a big if-or statement, or by adding an "isFinite()" method to MediaTime that just checks all the right flags. Created attachment 412730 [details]
Patch
Committed r270106: <https://trac.webkit.org/changeset/270106> |