Bug 85444 – Crash in webkitAddKey() when key parameter is null

• Component: Media
• Status: RESOLVED FIXED

[David Dorwin]

The following results in a crash
  video.webkitAddKey("foo", null);

The IDL ensures that there is a second parameter but not that it isn't null. We need to check this in the code and return an appropriate error.

The algorithm in the proposal also needs to be updated to to handle this.

[David Dorwin]

Created attachment 152791 [details]
Patch

[Kentaro Hara]

Comment on attachment 152791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152791&action=review

> Source/WebCore/ChangeLog:7
> +

Would you add the link to the spec that supports your change? The change looks good but I am not sure if SYNTAX_ERR is the expected error.

[David Dorwin]

(In reply to comment #2)
> (From update of attachment 152791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152791&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> Would you add the link to the spec that supports your change? The change looks good but I am not sure if SYNTAX_ERR is the expected error.

http://dvcs.w3.org/hg/html-media/raw-file/a70cac1c6d77/encrypted-media/encrypted-media.html#dom-addkey step 1. I had to update it to handle the second argument, but the first argument has always been a SYNTAX_ERR. If there is a better or more appropriate way of handling these cases, I can update this patch and the spec.

[Kentaro Hara]

Comment on attachment 152791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152791&action=review

>>> Source/WebCore/ChangeLog:7
>>> +
>> 
>> Would you add the link to the spec that supports your change? The change looks good but I am not sure if SYNTAX_ERR is the expected error.
> 
> http://dvcs.w3.org/hg/html-media/raw-file/a70cac1c6d77/encrypted-media/encrypted-media.html#dom-addkey step 1. I had to update it to handle the second argument, but the first argument has always been a SYNTAX_ERR. If there is a better or more appropriate way of handling these cases, I can update this patch and the spec.

Thanks. Looks OK. Please describe the link and the explanation you described in bugzilla to this ChangeLog.

[Kentaro Hara]

(In reply to comment #3)
> I had to update it to handle the second argument

OK, let's fix it in a separate patch.

[David Dorwin]

Created attachment 153893 [details]
Patch

[David Dorwin]

Added (newer) link to the ChangeLogs. The code is the same. CQ? Thanks.

[WebKit Review Bot]

Comment on attachment 153893 [details]
Patch

Clearing flags on attachment: 153893

Committed r123409: <http://trac.webkit.org/changeset/123409>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.