Bug 212458

Summary:

CodeGenerator should use DOMAttributeGetterSetter when it is DOMAttribute

Product:

WebKit

Reporter:

Philip Jägenstedt <philip>

Component:

Animations

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, dino, ews-watchlist, graouts, graouts, mark.lam, mjs, saam, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

Safari 13

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
crash log	none
standalone repro	none
Patch	none
Patch	mark.lam: review+

Description Philip Jägenstedt 2020-05-28 01:44:36 PDT

Created attachment 400435 [details]
crash log

This is a report for the problem discovered in https://github.com/mdittmer/web-apis/issues/94.

Safari version: 13.1.1 (15609.2.9.1.2)

I found and have attached ~/Library/Logs/DiagnosticReports/com.apple.WebKit.WebContent_2020-05-28-104123_foolip-macbookpro.crash.

I will attach the standalone repro case separately.

It doesn't repro in STP and it also doesn't repro when running with Web Inspector open.

Comment 1 Philip Jägenstedt 2020-05-28 01:44:55 PDT

Created attachment 400436 [details]
standalone repro

Comment 2 Radar WebKit Bug Importer 2020-05-29 03:33:53 PDT

<rdar://problem/63754110>

Comment 3 Yusuke Suzuki 2020-06-06 03:39:33 PDT

Reproduced. Looking!

Comment 4 Yusuke Suzuki 2020-06-06 05:48:44 PDT

Created attachment 401252 [details]
Patch

Comment 5 Yusuke Suzuki 2020-06-06 06:03:53 PDT

I need to update bindings

Comment 6 Yusuke Suzuki 2020-06-06 06:05:59 PDT

Created attachment 401253 [details]
Patch

Comment 7 Mark Lam 2020-06-06 07:28:33 PDT

Comment on attachment 401253 [details]
Patch

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

r=me

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4341
> +                    push(@implContent, "        putDirectCustomAccessor(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames()." . $attributeName . "PublicName(), CustomGetterSetter::create(vm, $getter, $setter), attributesForStructure($jscAttributes));\n");

Can you also add a bindings test case for this?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4467
> +        if (IsAcceleratedDOMAttribute($interface, $attribute)) {

Can you add bindings test for these 2 cases as well?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4487
> +        if (IsAcceleratedDOMAttribute($interface, $attribute)) {

Is it possible to add a bindings test for these 2 cases of PrivateName DOMAttributeGetterSetter and CustomGetterSetter?

Comment 8 Yusuke Suzuki 2020-06-06 19:14:21 PDT

Comment on attachment 401253 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4341
>> +                    push(@implContent, "        putDirectCustomAccessor(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames()." . $attributeName . "PublicName(), CustomGetterSetter::create(vm, $getter, $setter), attributesForStructure($jscAttributes));\n");
> 
> Can you also add a bindings test case for this?

Added.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4467
>> +        if (IsAcceleratedDOMAttribute($interface, $attribute)) {
> 
> Can you add bindings test for these 2 cases as well?

It turned out that this never happens. I've added assertion.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4487
>> +        if (IsAcceleratedDOMAttribute($interface, $attribute)) {
> 
> Is it possible to add a bindings test for these 2 cases of PrivateName DOMAttributeGetterSetter and CustomGetterSetter?

It turned out that this never happens. I've added assertion.

Comment 9 Yusuke Suzuki 2020-06-06 19:54:58 PDT

Committed r262693: <https://trac.webkit.org/changeset/262693>