Skip to content

Commit

Permalink
Generated attribute setters for nullable types fail when passing null.
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=259947
rdar://113591277

Reviewed by Chris Dumez.

We aren't allowing `null` as an attribute value in the setter for
nullable enum attributes. This change will check if the attribute is
nullable in the code generator and if so generate C++ code which will
call the native implementation of the setter with `std::nullopt` when
passed `null` in the JS code.

Preserves the existing behavior for non-nullable attributes.

* Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:
(GenerateAttributeSetterBodyDefinition):
* Source/WebCore/bindings/scripts/test:
    Added an attribute to TestObj to check the output of the generator for
    nullable enum type attributes.

Canonical link: https://commits.webkit.org/266726@main
  • Loading branch information
@rreno
rreno committed Aug 9, 2023
1 parent b73ca66 commit 23551a0
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 13 deletions.
42 changes: 29 additions & 13 deletions Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5681,10 +5681,35 @@ sub GenerateAttributeSetterBodyDefinition
push(@$outputArray, " return true;\n");
} else {
push(@$outputArray, " auto& impl = thisObject.wrapped();\n") if !$attribute->isStatic;


my $generateFunctionString = sub {
my $nativeValue = shift;
my ($baseFunctionName, @arguments) = $codeGenerator->SetterExpression(\%implIncludes, $interface->type->name, $attribute);
push(@arguments, $nativeValue);

my $functionName = GetFullyQualifiedImplementationCallName($interface, $attribute, $baseFunctionName, "impl", $conditional);
AddAdditionalArgumentsForImplementationCall(\@arguments, $interface, $attribute, "impl", "lexicalGlobalObject", "", "thisObject");

unshift(@arguments, GenerateCallWithUsingReferences($attribute->extendedAttributes->{SetterCallWith}, $outputArray, "false", "thisObject"));
unshift(@arguments, GenerateCallWithUsingReferences($attribute->extendedAttributes->{CallWith}, $outputArray, "false", "thisObject"));

return "${functionName}(" . join(", ", @arguments) . ")";
};

if ($codeGenerator->IsEnumType($attribute->type)) {
# As per section 3.5.6 of https://webidl.spec.whatwg.org/#dfn-attribute-setter, enumerations do not use
# the standard conversion, but rather silently fail on invalid enumeration values.
# As per section 3.7.6 of https://webidl.spec.whatwg.org/#dfn-attribute-setter, enumerations do not use
# the standard conversion, but rather silently fail on invalid enumeration values unless the attribute is nullable.
if ($attribute->type->isNullable) {
my $functionString = $generateFunctionString->("std::nullopt");

push(@$outputArray, " if (value.isUndefinedOrNull()) {\n");
push(@$outputArray, " invokeFunctorPropagatingExceptionIfNecessary(lexicalGlobalObject, throwScope, [&] {\n");
push(@$outputArray, " return $functionString;\n");
push(@$outputArray, " });\n");
push(@$outputArray, " return true;\n");
push(@$outputArray, " }\n\n");
}

push(@$outputArray, " auto optionalNativeValue = parseEnumeration<" . GetEnumerationClassName($attribute->type, $interface) . ">(lexicalGlobalObject, value);\n");
push(@$outputArray, " RETURN_IF_EXCEPTION(throwScope, false);\n");
push(@$outputArray, " if (UNLIKELY(!optionalNativeValue))\n");
Expand All @@ -5699,15 +5724,7 @@ sub GenerateAttributeSetterBodyDefinition
push(@$outputArray, " RETURN_IF_EXCEPTION(throwScope, false);\n");
}

my ($baseFunctionName, @arguments) = $codeGenerator->SetterExpression(\%implIncludes, $interface->type->name, $attribute);

push(@arguments, PassArgumentExpression("nativeValue", $attribute));

my $functionName = GetFullyQualifiedImplementationCallName($interface, $attribute, $baseFunctionName, "impl", $conditional);
AddAdditionalArgumentsForImplementationCall(\@arguments, $interface, $attribute, "impl", "lexicalGlobalObject", "", "thisObject");

unshift(@arguments, GenerateCallWithUsingReferences($attribute->extendedAttributes->{SetterCallWith}, $outputArray, "false", "thisObject"));
unshift(@arguments, GenerateCallWithUsingReferences($attribute->extendedAttributes->{CallWith}, $outputArray, "false", "thisObject"));
my $functionString = $generateFunctionString->(PassArgumentExpression("nativeValue", $attribute));

my $callTracer = $attribute->extendedAttributes->{CallTracer} || $interface->extendedAttributes->{CallTracer};
if ($callTracer) {
Expand All @@ -5716,7 +5733,6 @@ sub GenerateAttributeSetterBodyDefinition
GenerateCallTracer($outputArray, $callTracer, $attribute->name, \@callTracerArguments, $indent);
}

my $functionString = "${functionName}(" . join(", ", @arguments) . ")";
push(@$outputArray, " invokeFunctorPropagatingExceptionIfNecessary(lexicalGlobalObject, throwScope, [&] {\n");
push(@$outputArray, " return $functionString;\n");
push(@$outputArray, " });\n");
Expand Down
45 changes: 45 additions & 0 deletions Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1793,6 +1793,8 @@ static JSC_DECLARE_CUSTOM_GETTER(jsTestObjConstructor_TestSubObj);
static JSC_DECLARE_CUSTOM_GETTER(jsTestObjConstructor_testStaticReadonlyObj);
static JSC_DECLARE_CUSTOM_GETTER(jsTestObj_enumAttr);
static JSC_DECLARE_CUSTOM_SETTER(setJSTestObj_enumAttr);
static JSC_DECLARE_CUSTOM_GETTER(jsTestObj_nullableEnumAttr);
static JSC_DECLARE_CUSTOM_SETTER(setJSTestObj_nullableEnumAttr);
static JSC_DECLARE_CUSTOM_GETTER(jsTestObj_byteAttr);
static JSC_DECLARE_CUSTOM_SETTER(setJSTestObj_byteAttr);
static JSC_DECLARE_CUSTOM_GETTER(jsTestObj_octetAttr);
Expand Down Expand Up @@ -2208,6 +2210,7 @@ static const HashTableValue JSTestObjPrototypeTableValues[] =
{ "readOnlyStringAttr"_s, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::CustomAccessor | JSC::PropertyAttribute::DOMAttribute, NoIntrinsic, { HashTableValue::GetterSetterType, jsTestObj_readOnlyStringAttr, 0 } },
{ "readOnlyTestObjAttr"_s, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::CustomAccessor | JSC::PropertyAttribute::DOMAttribute, NoIntrinsic, { HashTableValue::GetterSetterType, jsTestObj_readOnlyTestObjAttr, 0 } },
{ "enumAttr"_s, JSC::PropertyAttribute::CustomAccessor | JSC::PropertyAttribute::DOMAttribute, NoIntrinsic, { HashTableValue::GetterSetterType, jsTestObj_enumAttr, setJSTestObj_enumAttr } },
{ "nullableEnumAttr"_s, JSC::PropertyAttribute::CustomAccessor | JSC::PropertyAttribute::DOMAttribute, NoIntrinsic, { HashTableValue::GetterSetterType, jsTestObj_nullableEnumAttr, setJSTestObj_nullableEnumAttr } },
{ "byteAttr"_s, JSC::PropertyAttribute::CustomAccessor | JSC::PropertyAttribute::DOMAttribute, NoIntrinsic, { HashTableValue::GetterSetterType, jsTestObj_byteAttr, setJSTestObj_byteAttr } },
{ "octetAttr"_s, JSC::PropertyAttribute::CustomAccessor | JSC::PropertyAttribute::DOMAttribute, NoIntrinsic, { HashTableValue::GetterSetterType, jsTestObj_octetAttr, setJSTestObj_octetAttr } },
{ "shortAttr"_s, JSC::PropertyAttribute::CustomAccessor | JSC::PropertyAttribute::DOMAttribute, NoIntrinsic, { HashTableValue::GetterSetterType, jsTestObj_shortAttr, setJSTestObj_shortAttr } },
Expand Down Expand Up @@ -2979,6 +2982,48 @@ JSC_DEFINE_CUSTOM_SETTER(setJSTestObj_enumAttr, (JSGlobalObject* lexicalGlobalOb
return IDLAttribute<JSTestObj>::set<setJSTestObj_enumAttrSetter>(*lexicalGlobalObject, thisValue, encodedValue, attributeName);
}

static inline JSValue jsTestObj_nullableEnumAttrGetter(JSGlobalObject& lexicalGlobalObject, JSTestObj& thisObject)
{
auto& vm = JSC::getVM(&lexicalGlobalObject);
auto throwScope = DECLARE_THROW_SCOPE(vm);
auto& impl = thisObject.wrapped();
RELEASE_AND_RETURN(throwScope, (toJS<IDLNullable<IDLEnumeration<TestObj::EnumType>>>(lexicalGlobalObject, throwScope, impl.nullableEnumAttr())));
}

JSC_DEFINE_CUSTOM_GETTER(jsTestObj_nullableEnumAttr, (JSGlobalObject* lexicalGlobalObject, EncodedJSValue thisValue, PropertyName attributeName))
{
return IDLAttribute<JSTestObj>::get<jsTestObj_nullableEnumAttrGetter, CastedThisErrorBehavior::Assert>(*lexicalGlobalObject, thisValue, attributeName);
}

static inline bool setJSTestObj_nullableEnumAttrSetter(JSGlobalObject& lexicalGlobalObject, JSTestObj& thisObject, JSValue value)
{
auto& vm = JSC::getVM(&lexicalGlobalObject);
UNUSED_PARAM(vm);
auto throwScope = DECLARE_THROW_SCOPE(vm);
auto& impl = thisObject.wrapped();
if (value.isUndefinedOrNull()) {
invokeFunctorPropagatingExceptionIfNecessary(lexicalGlobalObject, throwScope, [&] {
return impl.setNullableEnumAttr(std::nullopt);
});
return true;
}

auto optionalNativeValue = parseEnumeration<TestObj::EnumType>(lexicalGlobalObject, value);
RETURN_IF_EXCEPTION(throwScope, false);
if (UNLIKELY(!optionalNativeValue))
return false;
auto nativeValue = optionalNativeValue.value();
invokeFunctorPropagatingExceptionIfNecessary(lexicalGlobalObject, throwScope, [&] {
return impl.setNullableEnumAttr(WTFMove(nativeValue));
});
return true;
}

JSC_DEFINE_CUSTOM_SETTER(setJSTestObj_nullableEnumAttr, (JSGlobalObject* lexicalGlobalObject, EncodedJSValue thisValue, EncodedJSValue encodedValue, PropertyName attributeName))
{
return IDLAttribute<JSTestObj>::set<setJSTestObj_nullableEnumAttrSetter>(*lexicalGlobalObject, thisValue, encodedValue, attributeName);
}

static inline JSValue jsTestObj_byteAttrGetter(JSGlobalObject& lexicalGlobalObject, JSTestObj& thisObject)
{
auto& vm = JSC::getVM(&lexicalGlobalObject);
Expand Down
1 change: 1 addition & 0 deletions Source/WebCore/bindings/scripts/test/TestObj.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ enum TestConfidence { "high", "kinda-low" };
static readonly attribute TestSubObjConstructor TestSubObj;
[CallWith=CurrentDocument] static readonly attribute TestObj testStaticReadonlyObj;
attribute TestEnumType enumAttr;
attribute TestEnumType? nullableEnumAttr;
attribute byte byteAttr;
attribute octet octetAttr;
[Unscopable] attribute short shortAttr;
Expand Down

0 comments on commit 23551a0

Please sign in to comment.