Bug 169731 – Add support for ImplementedAs, Clamp, EnforceRange, TreatNullAs for dictionary members

• Component: Bindings
• Status: RESOLVED FIXED

[Jon Lee]

Add support for ImplementedAs, Clamp, EnforceRange, TreatNullAs for dictionary members

[Jon Lee]

Created attachment 304600 [details]
Patch

[Build Bot]

Comment on attachment 304600 [details]
Patch

Attachment 304600 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3335541

New failing tests:
crypto/subtle/aes-gcm-encrypt-malformed-parameters.html
crypto/subtle/hmac-generate-key-malformed-parameters.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html
crypto/subtle/aes-generate-key-malformed-parameters.html

[Build Bot]

Created attachment 304604 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 304600 [details]
Patch

Attachment 304600 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3335546

New failing tests:
crypto/subtle/aes-gcm-encrypt-malformed-parameters.html
crypto/subtle/hmac-generate-key-malformed-parameters.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html
crypto/subtle/aes-generate-key-malformed-parameters.html

[Build Bot]

Created attachment 304605 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 304600 [details]
Patch

Attachment 304600 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3335552

New failing tests:
crypto/subtle/aes-gcm-encrypt-malformed-parameters.html
crypto/subtle/hmac-generate-key-malformed-parameters.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html
crypto/subtle/aes-generate-key-malformed-parameters.html

[Build Bot]

Created attachment 304606 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 304600 [details]
Patch

Attachment 304600 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3335572

New failing tests:
crypto/subtle/aes-gcm-encrypt-malformed-parameters.html
crypto/subtle/hmac-generate-key-malformed-parameters.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html
crypto/subtle/aes-generate-key-malformed-parameters.html

[Build Bot]

Created attachment 304610 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Jon Lee]

Jiewen and Brent, I think this patch is exposing bugs in subtle crypto. [EnforceRange] in AesGcmParams and AesKeyParams now throw TypeErrors.

Can you help me determine whether the tests need to be rewritten, or rebaselined?

[Sam Weinig]

Comment on attachment 304600 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1570
> +            # 5.1.1. Set a different key in case it is aliased with ImplementedAs.
> +            my $resultKey = $key;
> +            if ($member->extendedAttributes->{"ImplementedAs"}) {
> +                $resultKey = $member->extendedAttributes->{"ImplementedAs"};
> +            }

The 5.1 business is spec step language.  Since this implementation specific, it should not have a number.  Probably just a comment explaining why it's needed here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1586
> -            $result .= "        result.$key = convert<${IDLType}>(state, ${key}Value);\n";
> +
> +            my @conversionArguments = ();
> +            push(@conversionArguments, "state");
> +            push(@conversionArguments, "${key}Value");
> +            push(@conversionArguments, GetIntegerConversionConfiguration($member)) if $codeGenerator->IsIntegerType($type);
> +            push(@conversionArguments, GetStringConversionConfiguration($member)) if $codeGenerator->IsStringType($type);
> +            $result .= "        result.$resultKey = convert<${IDLType}>(" . join(", ", @conversionArguments) . ");\n";
>              $result .= "        RETURN_IF_EXCEPTION(throwScope, { });\n";

Ideally we would be able to use JSValueToNative, rather than repeating things here. Probably need to update the list of contexts to include IDLDictionaryMember.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1632
> +                # 1.1. Set a different key in case it is aliased with ImplementedAs.
> +                my $resultKey = $key;
> +                if ($member->extendedAttributes->{"ImplementedAs"}) {
> +                    $resultKey = $member->extendedAttributes->{"ImplementedAs"};
> +                }

Again, spec language.

[Jiewen Tan]

*** Bug 169333 has been marked as a duplicate of this bug. ***

[Jiewen Tan]

(In reply to comment #10)
> Jiewen and Brent, I think this patch is exposing bugs in subtle crypto.
> [EnforceRange] in AesGcmParams and AesKeyParams now throw TypeErrors.
> 
> Can you help me determine whether the tests need to be rewritten, or
> rebaselined?

Ha, I didn't realize that we didn't support EnforceRange before. For tests cases under crypto/, rebaseline should be fine. However, I need to investigate why the w3c test cases fail.

[Jiewen Tan]

(In reply to comment #13)
> (In reply to comment #10)
> > Jiewen and Brent, I think this patch is exposing bugs in subtle crypto.
> > [EnforceRange] in AesGcmParams and AesKeyParams now throw TypeErrors.
> > 
> > Can you help me determine whether the tests need to be rewritten, or
> > rebaselined?
> 
> Ha, I didn't realize that we didn't support EnforceRange before. For tests
> cases under crypto/, rebaseline should be fine. However, I need to
> investigate why the w3c test cases fail.

We can rebaseline the w3c tests as well. In my opinion, the expected results are wrong. We are doing the right thing. I will file bug reports against the w3c tests later on.

[Jon Lee]

Sam, I took a stab at switching to JSValueToNative, and the way enumerations were written out changed.

Is this a better way to convert enumerations, or is it wrong? If it's the latter, then I will not attempt to switch, and will add a FIXME and file a new bug to do the conversion.

Here's an example of the change:

--- WebCore/bindings/scripts/test/JS/JSTestObj.cpp	2017-03-16 10:43:26.000000000 -0700
+++ /var/folders/sf/90xm7_w50ld3z7dmg4lm01ph0000gn/T/tmppGuUgJ/JSTestObj.cpp	2017-03-16 12:43:53.000000000 -0700
@@ -517,19 +517,19 @@
     }
     JSValue enumerationValueWithDefaultValue = isNullOrUndefined ? jsUndefined() : object->get(&state, Identifier::fromString(&state, "enumerationValueWithDefault"));
     if (!enumerationValueWithDefaultValue.isUndefined()) {
-        result.enumerationValueWithDefault = convert<IDLEnumeration<TestObj::EnumType>>(state, enumerationValueWithDefaultValue);
+        result.enumerationValueWithDefault = parseEnumeration<TestObj::EnumType>(state, enumerationValueWithDefaultValue);
         RETURN_IF_EXCEPTION(throwScope, { });
     } else
         result.enumerationValueWithDefault = TestObj::EnumType::EnumValue1;
     JSValue enumerationValueWithEmptyStringDefaultValue = isNullOrUndefined ? jsUndefined() : object->get(&state, Identifier::fromString(&state, "enumerationValueWithEmptyStringDefa

[Jon Lee]

Created attachment 304765 [details]
Patch

[Chris Dumez]

Comment on attachment 304765 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        (JSValueToNative): Bypass parseEnumeration serialization for enums if the context is an

This does not explain why we should do this. It is not obvious to me at least.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1566
> +            my $implementedAsKey = $key;

nit: Could be on one line:
my $implementedAsKey = $member->extendedAttributes->{"ImplementedAs"} ? $member->extendedAttributes->{"ImplementedAs"} : $key;

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1621
> +                my $implementedAsKey = $key;

nit: Could be on one line:
my $implementedAsKey = $member->extendedAttributes->{"ImplementedAs"} ? $member->extendedAttributes->{"ImplementedAs"} : $key;

[Alex Christensen]

Comment on attachment 304765 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5550
> +    if ($codeGenerator->IsEnumType($type) && ref($context) ne "IDLDictionaryMember") {

Yep, explain this.  We might be able to do it a better way in the future.

[Jon Lee]

Comment on attachment 304765 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        (JSValueToNative): Bypass parseEnumeration serialization for enums if the context is an
> 
> This does not explain why we should do this. It is not obvious to me at least.

I'll add an explanation here and a brief one in the code below. parseEnumeration returns std::optional, where we need it to throw a TypeError instead. To do this, we pass through so that it write out convert<IDLEnumeration<T>>.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1566
>> +            my $implementedAsKey = $key;
> 
> nit: Could be on one line:
> my $implementedAsKey = $member->extendedAttributes->{"ImplementedAs"} ? $member->extendedAttributes->{"ImplementedAs"} : $key;

Done.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1621
>> +                my $implementedAsKey = $key;
> 
> nit: Could be on one line:
> my $implementedAsKey = $member->extendedAttributes->{"ImplementedAs"} ? $member->extendedAttributes->{"ImplementedAs"} : $key;

Done.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5550
>> +    if ($codeGenerator->IsEnumType($type) && ref($context) ne "IDLDictionaryMember") {
> 
> Yep, explain this.  We might be able to do it a better way in the future.

Done.

[Jon Lee]

Created attachment 304846 [details]
Patch for landing

[Build Bot]

Comment on attachment 304846 [details]
Patch for landing

Attachment 304846 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3353570

New failing tests:
crypto/subtle/aes-gcm-encrypt-malformed-parameters.html
crypto/subtle/hmac-generate-key-malformed-parameters.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html
crypto/subtle/aes-generate-key-malformed-parameters.html

[Build Bot]

Created attachment 304854 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jon Lee]

Created attachment 304855 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 304855 [details]
Patch for landing

Clearing flags on attachment: 304855

Committed r214139: <http://trac.webkit.org/changeset/214139>

[Alexey Proskuryakov]

Comment on attachment 304765 [details]
Patch

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

> LayoutTests/imported/w3c/ChangeLog:10
> +        Rebaseline tests. The results seem to show a problem in the original tests.
> +        * web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker-expected.txt:
> +        * web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm-expected.txt:

Do these failures occur in other browsers?