Setting undefined to video.playbackRate causes Safari to crash

• Component: Media
• Status: RESOLVED FIXED

[Takeshi Sone]

Created attachment 449030 [details]
Crash Report

1. Open this on iPhone or iPad:
https://jsitor.com/NPeIszqH_
2. Tap play button.
3. Safari crashes

[Takeshi Sone]

Created attachment 449031 [details]
Patch

[Alexey Proskuryakov]

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

[Takeshi Sone]

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?

[Takeshi Sone]

Created attachment 449301 [details]
Chrome and Firefox snapshot

[Radar WebKit Bug Importer]

<rdar://problem/87815745>

[Takeshi Sone]

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.

[Eric Carlson]

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?

[Chris Dumez]

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.

[Darin Adler]

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.

[Takeshi Sone]

Created attachment 449621 [details]
Patch take 3

[Eric Carlson]

Comment on attachment 449621 [details]
Patch take 3

Thank you for the fix!

[Darin Adler]

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.

[Jon Lee]

any update on this?

[Eric Carlson]

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.

[EWS]

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