Bug 153547 - Getting / Setting property on prototype object must throw TypeError
Summary: Getting / Setting property on prototype object must throw TypeError
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://heycam.github.io/webidl/#dfn-a...
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2016-01-27 09:21 PST by Chris Dumez
Modified: 2016-01-27 14:41 PST (History)
12 users (show)

See Also:


Attachments
Patch (664.85 KB, patch)
2016-01-27 10:35 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (574.50 KB, patch)
2016-01-27 10:39 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-01-27 09:21:18 PST
getting property on prototype object must throw TypeError. Currently, WebKit returns undefined and logs a deprecation error.
Chrome and Firefox throw a TypeError.
Comment 1 Chris Dumez 2016-01-27 09:45:18 PST
Exception is for attributes that have [LenientThis] IDL extended attribute:
http://heycam.github.io/webidl/#LenientThis
Comment 2 Radar WebKit Bug Importer 2016-01-27 10:13:40 PST
<rdar://problem/24370650>
Comment 3 Chris Dumez 2016-01-27 10:35:47 PST
Created attachment 270005 [details]
Patch
Comment 4 WebKit Commit Bot 2016-01-27 10:38:33 PST
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.
Comment 5 Chris Dumez 2016-01-27 10:39:57 PST
Created attachment 270008 [details]
Patch
Comment 6 WebKit Commit Bot 2016-01-27 10:42:34 PST
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.
Comment 7 Ryosuke Niwa 2016-01-27 11:15:32 PST
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?
Comment 8 Oliver Hunt 2016-01-27 12:07:05 PST
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.
Comment 9 Chris Dumez 2016-01-27 13:32:43 PST
(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.
Comment 10 Chris Dumez 2016-01-27 13:48:20 PST
(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.
Comment 11 Chris Dumez 2016-01-27 14:41:00 PST
Comment on attachment 270008 [details]
Patch

Clearing flags on attachment: 270008

Committed r195695: <http://trac.webkit.org/changeset/195695>
Comment 12 Chris Dumez 2016-01-27 14:41:07 PST
All reviewed patches have been landed.  Closing bug.