Bug 75789 – defineOwnProperty not implemented for Array objects

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

See ES5.1 15.4.5.1

[Gavin Barraclough]

Created attachment 121567 [details]
WIP

[Gavin Barraclough]

Created attachment 121704 [details]
Fix

[Gavin Barraclough]

Comment on attachment 121704 [details]
Fix

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

> Source/JavaScriptCore/runtime/JSArray.cpp:405
> +        // FIXME: this call to attributes will make absent attributes 

Ooops, this comment is incomplete. The code is safe, put a little conservative in the case of reconfiguring existing properties.
I think the call to attributes will pessimistically assume that if attributes are missing then they'll default to false – however if the property currently exists missing attributes will override from their current 'true' state (i.e. defineOwnProperty could be used to set a value without needing to entering 'SparseMode').

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/runtime/JSArray.cpp:219:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/runtime/JSArray.cpp:419:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/runtime/JSArray.cpp:433:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/JSArray.cpp:599:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/ChangeLog:21:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:23:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:27:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:29:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:31:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:33:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:35:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:37:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:41:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:43:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:45:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:47:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:49:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:51:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:53:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:55:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:57:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:59:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:61:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:63:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:65:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:67:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:70:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:72:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:74:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:76:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/runtime/JSArray.h:86:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 32 in 17 files


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

[Sam Weinig]

Comment on attachment 121704 [details]
Fix

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

> Source/JavaScriptCore/JavaScriptCore.exp:5
> +__ZN3JSC7JSArray17defineOwnPropertyEPNS_8JSObjectEPNS_9ExecStateERKNS_10IdentifierERNS_18PropertyDescriptorEb
> +__ZN3JSC7JSArray9setLengthEjb
>  _JSCheckScriptSyntax
>  _JSClassCreate
>  _JSClassRelease

Please sort this.

> Source/JavaScriptCore/runtime/JSArray.cpp:261
> +    JSValue value = WriteBarrier<Unknown>::get();

Can you add a typedef of WriteBarrier<Unknown> to Base to make this cleaner.

> Source/JavaScriptCore/runtime/JSArray.h:81
> +            m_flags = (Flags)(m_flags | LengthIsReadOnly);

This should be a static cast.

> Source/JavaScriptCore/runtime/PropertyDescriptor.h:70
> +        unsigned attributesOverridingCurrent(const PropertyDescriptor& current) const;
>      private:

Please put a newline after this function.

[Geoffrey Garen]

I just threw up a little bit in my mouth.

[WebKit Review Bot]

Comment on attachment 121704 [details]
Fix

Attachment 121704 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11166645

New failing tests:
fast/js/array-defineOwnProperty.html
fast/js/mozilla/strict/15.4.4.6.html

[Gavin Barraclough]

Fixed in r104488

[Ryosuke Niwa]

It seems that this patch broke Windows build:
http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/25604
http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/25605

Regression window: http://trac.webkit.org/log/?rev=104490&stop_rev=104486

[Gavin Barraclough]

(In reply to comment #9)
> It seems that this patch broke Windows build:
> http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/25604
> http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/25605
> 
> Regression window: http://trac.webkit.org/log/?rev=104490&stop_rev=104486

Ooops! - sorry! - build fixed in r104611.