Bug 153547 – Getting / Setting property on prototype object must throw TypeError

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

getting property on prototype object must throw TypeError. Currently, WebKit returns undefined and logs a deprecation error.
Chrome and Firefox throw a TypeError.

[Chris Dumez]

Exception is for attributes that have [LenientThis] IDL extended attribute:
http://heycam.github.io/webidl/#LenientThis

[Radar WebKit Bug Importer]

<rdar://problem/24370650>

[Chris Dumez]

Created attachment 270005 [details]
Patch

[WebKit Commit Bot]

Attachment 270005 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2059:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 270008 [details]
Patch

[WebKit Commit Bot]

Attachment 270008 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2059:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 270008 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Gettingi / Setting property on prototype object must throw TypeError as per

Typo: Gettingi.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
> -                    push(@implContent, "    if (UNLIKELY(!castedThis)) {\n");
> -                    push(@implContent, "        if (jsDynamicCast<${className}Prototype*>(slotBase))\n");
> -                    push(@implContent, "            return reportDeprecatedGetterError(*state, \"$interfaceName\", \"$name\");\n");
> -                    push(@implContent, "        return throwGetterTypeError(*state, \"$interfaceName\", \"$name\");\n");
> -                    push(@implContent, "    }\n");
> +                    push(@implContent, "    if (UNLIKELY(!castedThis))\n");
> +                    if ($attribute->signature->extendedAttributes->{"LenientThis"}) {
> +                        push(@implContent, "        return JSValue::encode(jsUndefined());\n");
> +                    } else {
> +                        push(@implContent, "        return throwGetterTypeError(*state, \"$interfaceName\", \"$name\");\n");
> +                    }

I do remember this was needed for some compat reason.
Perhaps Oliver or Geoff would remember?

[Oliver Hunt]

Comment on attachment 270008 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
>> +                    }
> 
> I do remember this was needed for some compat reason.
> Perhaps Oliver or Geoff would remember?

Alas i can't recall -- does blame help at all?

It _may_ have been YUI as I recall it being a source of pain around that time.

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 270008 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270008&action=review
> 
> >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
> >> +                    }
> > 
> > I do remember this was needed for some compat reason.
> > Perhaps Oliver or Geoff would remember?
> 
> Alas i can't recall -- does blame help at all?
> 
> It _may_ have been YUI as I recall it being a source of pain around that
> time.

I have been going through the git log and found:
https://bugs.webkit.org/show_bug.cgi?id=132626
https://bugs.webkit.org/show_bug.cgi?id=128568

I will take a look at those, but these are public sites and other browsers are throwing exceptions in this case already.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 270008 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=270008&action=review
> > 
> > >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
> > >> +                    }
> > > 
> > > I do remember this was needed for some compat reason.
> > > Perhaps Oliver or Geoff would remember?
> > 
> > Alas i can't recall -- does blame help at all?
> > 
> > It _may_ have been YUI as I recall it being a source of pain around that
> > time.
> 
> I have been going through the git log and found:
> https://bugs.webkit.org/show_bug.cgi?id=132626
> https://bugs.webkit.org/show_bug.cgi?id=128568
> 
> I will take a look at those, but these are public sites and other browsers
> are throwing exceptions in this case already.

1. One of these bugs does not mention which sites were broken :(
2. The second bug mentions making a booking on virginamerica.com. I have just tried doing so (up to the payment stage) and did not see any problem
3. The original patch was overly aggressive as we were not supposed to throw for attributes marked as [LenientThis]. This new patch takes this into consideration.
4. Since then, Chrome has also moved attributes to the prototype and is throwing in this case. Therefore, public sites would likely be broken in Chrome already if this was still a problem. Firefox has been throwing for a long time as well.
5. This was 2 years ago

I propose we give this a shot again. If we notice that a particular attribute is problematic later, we can then mark it as [LenientThis] to restore the previous behavior and ask for this change to be made to the specification as well as it would not be web-compatible.

[Chris Dumez]

Comment on attachment 270008 [details]
Patch

Clearing flags on attachment: 270008

Committed r195695: <http://trac.webkit.org/changeset/195695>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.