[WebIDL] Enable strict type checking for nullable attribute setters of wrapper types

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Enable strict type checking for nullable attribute setters of wrapper types:
- http://heycam.github.io/webidl/#es-nullable-type
- http://heycam.github.io/webidl/#es-interface

For such attributes, if the JS tries to assign a value that is not null / undefined and does not have the expected wrapper type, then we now throw a TypeError instead of silently converting the value to null.

This behavior is consistent with Chrome and Firefox. It also helps identify bugs in JavaScript code.

[Chris Dumez]

Created attachment 284944 [details]
Patch

[Darin Adler]

Comment on attachment 284944 [details]
Patch

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

Seems unnecessary to both make this change and remove all the uses of StrictTypeChecking in the same patch. Would have been slightly nicer to keep this one smaller by separating the IDL changes, I think.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-2914
> +                my $putForwards = $attribute->signature->extendedAttributes->{"PutForwards"};
>                  if (!$attribute->isStatic) {
> -                    my $putForwards = $attribute->signature->extendedAttributes->{"PutForwards"};

I don’t understand the reason for this change. Please don’t do it unless there is a reason.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2928
> +                        $type = $attribute->signature->type;

I don’t understand why this change is needed. Is there a test that shows the bug that was caused by the lack of this line of code?

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2937
> +    TestObj* nativeValue = nullptr;
> +    if (!value.isUndefinedOrNull()) {
> +        nativeValue = JSTestObj::toWrapped(value);
> +        if (UNLIKELY(!nativeValue)) {

This ends up checking for the type twice in the normal non-null code path, once inline here, and then a second time in the toWrapped function. If we think that non-null values are more common than null ones we could consider the version I suggested in the patch with DOMWindow where we do the check only after calling toWrapped to avoid throwing the error. It would also eliminate the need for the store of nullptr.

Generally of course, we want the code for bindings to be both small and fast, with good decisions about what’s inlined and what is not. Would be nice if the type checking didn’t have to be before here *and* in the toWrapped function.

[Chris Dumez]

Comment on attachment 284944 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2928
>> +                        $type = $attribute->signature->type;
> 
> I don’t understand why this change is needed. Is there a test that shows the bug that was caused by the lack of this line of code?

Yes, this change is needed. In the case of PutForward, we setter forwards to another attribute. Therefore, we were updating the attribute above. However, we failed to update the $type to reflect the type of the new attribute. This was causing trouble because of the $codeGenerator->IsWrapperType($type) below that we use for strict type checking. The issue shows for the following attribute for example on Document.idl:
[PutForwards=href, Unforgeable] readonly attribute Location? location;

The setter actually sets Location.href, which is a String. However, $codeGenerator->IsWrapperType($type) was returning true below because $type was "Location" instead of "String". Therefore, we were trying to generate strict wrapper type checking code for an String attribute setter.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2937
>> +        if (UNLIKELY(!nativeValue)) {
> 
> This ends up checking for the type twice in the normal non-null code path, once inline here, and then a second time in the toWrapped function. If we think that non-null values are more common than null ones we could consider the version I suggested in the patch with DOMWindow where we do the check only after calling toWrapped to avoid throwing the error. It would also eliminate the need for the store of nullptr.
> 
> Generally of course, we want the code for bindings to be both small and fast, with good decisions about what’s inlined and what is not. Would be nice if the type checking didn’t have to be before here *and* in the toWrapped function.

For nullable parameters / attributes, I don't think we can assume null is more or less likely than a valid wrapper type (I agreed with you that null is less likely in the dictionary member case though). Now I agree this should be cleaned up a bit. I need to think about this more but I think we may want toWrapped() to:
1. Take a "IsNullable" flag parameter.
2. Take care of throwing a TypeError when needed.

I did not want to refactor however until I:
1. Enabled strict type checking everywhere (I am going incrementally)
2. Given more thought about what the best solution would be.

[Chris Dumez]

Comment on attachment 284944 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-2914
>> -                    my $putForwards = $attribute->signature->extendedAttributes->{"PutForwards"};
> 
> I don’t understand the reason for this change. Please don’t do it unless there is a reason.

Oh, this one is not needed. This is a reminiscence of an earlier iteration of my patch. I'll fix.

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 284944 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284944&action=review
> 
> Seems unnecessary to both make this change and remove all the uses of
> StrictTypeChecking in the same patch. Would have been slightly nicer to keep
> this one smaller by separating the IDL changes, I think.

Ok, I'll keep this in mind for my next refactoring. I seemed a little bit odd to keep [StrictTypeChecking] on attributes given that it was a no-op with my patch and that strict type checking is the default behavior.

[Chris Dumez]

Created attachment 284958 [details]
Patch

[Chris Dumez]

Comment on attachment 284958 [details]
Patch

Clearing flags on attachment: 284958

Committed r203949: <http://trac.webkit.org/changeset/203949>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.