Bug 25031 - Chrome allows elements with names to override collection properties
Summary: Chrome allows elements with names to override collection properties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-03 11:05 PDT by Dave Moore
Modified: 2009-04-06 11:01 PDT (History)
1 user (show)

See Also:


Attachments
Fix (4.82 KB, patch)
2009-04-03 11:09 PDT, Dave Moore
dglazkov: review-
Details | Formatted Diff | Diff
Fix (5.08 KB, patch)
2009-04-03 12:26 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Fix (6.19 KB, patch)
2009-04-03 13:27 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Fix (5.58 KB, patch)
2009-04-03 14:06 PDT, Dave Moore
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Moore 2009-04-03 11:05:28 PDT
If you have an HTMLSelectElement and it has an option that happens to be named 'length' you will get this element when accessing the length property, instead of the actual length. This also causes these accesses to be slower than they need to be.
Comment 1 Dave Moore 2009-04-03 11:09:57 PDT
Created attachment 29233 [details]
Fix

This patch will only effect the chromium build, as it's in the bindings
Comment 2 Dimitri Glazkov (Google) 2009-04-03 11:19:22 PDT
Comment on attachment 29233 [details]
Fix

Don't forget bug URL in ChangeLog description.

> +        (WebCore::collectionNamedPropertyGetter):
> +        (WebCore::nodeCollectionNamedPropertyGetter):

It's a good practice to add brief descriptions to each line on what's being done.

> +        if (!value.IsEmpty())
> +          return value;

4 spaces.

> +
> +        // Search local callback properties next to find IDL defined
> +        // properties.
> +        if (info.Holder()->HasRealNamedCallbackProperty(name))
> +          return v8::Handle<v8::Value>();

4 spaces.

Also, I think this should be return notHandledbyInterceptor().

> +        if (!value.IsEmpty())
> +          return value;

4 spaces.

> +        if (info.Holder()->HasRealNamedCallbackProperty(name))
> +          return v8::Handle<v8::Value>();

4 spaces, notHandledByInterceptor()

> +2009-04-03  Dave Moore  <davemoore@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).

Need a good description of what the tests test.

> +            function test()
> +            {
> +                if (window.layoutTestController)
> +                    layoutTestController.dumpAsText();

Is this where we should put attempting to override length?

> +
> +                var sl = document.getElementById("sl");
> +                if (sl.length == 1)
Comment 3 Dave Moore 2009-04-03 12:26:22 PDT
Created attachment 29236 [details]
Fix
Comment 4 Dave Moore 2009-04-03 13:27:41 PDT
Created attachment 29237 [details]
Fix

Added a new tests to ensure that JS can override the length property on the HTMLSelectElement
Comment 5 Dave Moore 2009-04-03 14:06:09 PDT
Created attachment 29240 [details]
Fix

Remove test for JS setting the length property.
Comment 6 Dimitri Glazkov (Google) 2009-04-03 14:07:43 PDT
Comment on attachment 29240 [details]
Fix

looks good!
Comment 7 Dimitri Glazkov (Google) 2009-04-06 11:01:56 PDT
Landed as http://trac.webkit.org/changeset/42247.