Created attachment 449030 [details] Crash Report 1. Open this on iPhone or iPad: https://jsitor.com/NPeIszqH_ 2. Tap play button. 3. Safari crashes
Created attachment 449031 [details] Patch
Thank you for the patch! Would you be willing to formally submit it for review as described at <https://webkit.org/contributing-code/> (with a ChangeLog, regression test, and marked as r?)?
I tried to write automated test, but the behavior is inconsistent when the playbackRate is NaN, including on macOS, it's hard to clarify my patch fix the crash. So I'm considering to add additional guards in HTMLMediaElement. I'm not sure if it is better to just ignore NaN silently, or throw DOMException like Chrome and Firefox do. What do you think?
Created attachment 449301 [details] Chrome and Firefox snapshot
<rdar://problem/87815745>
Created attachment 449567 [details] Patch Added guards to ignore all non-finite value silently. Added a test to confirm that fails without these patches and passes with patches.
Comment on attachment 449567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449567&action=review > Source/WebCore/ChangeLog:5 > + rdar://problem/87815745 Nit: You have a tab here. > Source/WebCore/html/HTMLMediaElement.cpp:3498 > + if (!isfinite(rate)) > + return; > + The spec defines `defaultPlaybackRate` and `playbackRate` as `double`: attribute double defaultPlaybackRate; attribute double playbackRate; but HTMLMediaElement.idl uses `unrestricted double`: attribute unrestricted double defaultPlaybackRate; attribute unrestricted double playbackRate; I wonder if the changes to setDefaultPlaybackRate and setPlaybackRate would be unnecessary if this was fixed?
Comment on attachment 449567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449567&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:3498 >> + > > The spec defines `defaultPlaybackRate` and `playbackRate` as `double`: > > attribute double defaultPlaybackRate; > attribute double playbackRate; > > but HTMLMediaElement.idl uses `unrestricted double`: > > attribute unrestricted double defaultPlaybackRate; > attribute unrestricted double playbackRate; > > I wonder if the changes to setDefaultPlaybackRate and setPlaybackRate would be unnecessary if this was fixed? Eric is completely right. if our IDL matched the spec, it wouldn't be possible for the implementation to be called with infinite.
Comment on attachment 449567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449567&action=review >>> Source/WebCore/html/HTMLMediaElement.cpp:3498 >>> + >> >> The spec defines `defaultPlaybackRate` and `playbackRate` as `double`: >> >> attribute double defaultPlaybackRate; >> attribute double playbackRate; >> >> but HTMLMediaElement.idl uses `unrestricted double`: >> >> attribute unrestricted double defaultPlaybackRate; >> attribute unrestricted double playbackRate; >> >> I wonder if the changes to setDefaultPlaybackRate and setPlaybackRate would be unnecessary if this was fixed? > > Eric is completely right. if our IDL matched the spec, it wouldn't be possible for the implementation to be called with infinite. Another way to put this is: The correct fix would be to change the .idl, not to change the .cpp file. I suspect we won’t need any changes to WebAVPlayerController.mm either. Please pursue that solution. > Source/WebCore/platform/ios/WebAVPlayerController.mm:201 > + /* NaN == NaN is false. > + * Catch that case to prevent infinite mutual recursion. */ Incorrect coding style. Please follow WebKit coding style, using "//" comments.
Created attachment 449621 [details] Patch take 3
Comment on attachment 449621 [details] Patch take 3 Thank you for the fix!
Comment on attachment 449621 [details] Patch take 3 Surprised there is not an existing Web Platform Test that covers this. Would be nice to follow up and make sure this test case is covered there, not just in our WebKit project tests.
any update on this?
Comment on attachment 449621 [details] Patch take 3 (In reply to Jon Lee from comment #13) > any update on this? Lets land this and follow up about a WPT test.
Committed r288827 (246597@main): <https://commits.webkit.org/246597@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449621 [details].