When using an Object as a set in Object.getOwnPropertyNames, null out the proto (issue 11364237)

[ad...@chromium.org]

Reviewers: rossberg,

Message:
Let me know if there's another reviewer we should be sending random JS fixes
to...(this one was found by Rafael while we were looking at something else;
I
decided to fix it while I was there).

Description:
When using an Object as a set in Object.getOwnPropertyNames, null out the
proto

BUG=v8:2410


Please review this at https://codereview.chromium.org/11364237/

SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/v8natives.js
A + test/mjsunit/regress/regress-2410.js


Index: src/v8natives.js
diff --git a/src/v8natives.js b/src/v8natives.js
index
20fc74dc440dcf28dd64ece1de30133611aadf0b..cd7ff78f973a2963d63bfee3eb0413a00911669a
100644
--- a/src/v8natives.js
+++ b/src/v8natives.js
@@ -1025,7 +1025,7 @@ function ObjectGetOwnPropertyNames(obj) {
}

// Property names are expected to be unique strings.
- var propertySet = {};
+ var propertySet = { __proto__: null };
var j = 0;
for (var i = 0; i < propertyNames.length; ++i) {
var name = ToString(propertyNames[i]);
Index: test/mjsunit/regress/regress-2410.js
diff --git a/src/platform-posix.h b/test/mjsunit/regress/regress-2410.js
similarity index 81%
copy from src/platform-posix.h
copy to test/mjsunit/regress/regress-2410.js
index
7a982ed2ef3080dad77860d6f46b356a9067bf3b..c16fd14cdc6070ef641962ca2476ae0b6b3e1d82
100644
--- a/src/platform-posix.h
+++ b/test/mjsunit/regress/regress-2410.js
@@ -25,15 +25,12 @@
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

-#ifndef V8_PLATFORM_POSIX_H_
-#define V8_PLATFORM_POSIX_H_
-
-namespace v8 {
-namespace internal {
-
-// Used by platform implementation files during OS::PostSetUp().
-void POSIXPostSetUp();
-
-} } // namespace v8::internal
+// Object.prototype should be ignored in Object.getOwnPropertyNames
+//
+// See http://code.google.com/p/v8/issues/detail?id=2410 for details.

-#endif // V8_PLATFORM_POSIX_H_
+Object.defineProperty(Object.prototype,
+ 'thrower',
+ { get: function() { throw Error('bug') } });
+var obj = { thrower: 'local' };
+assertEquals(['thrower'], Object.getOwnPropertyNames(obj));

[ross...@chromium.org]

Nice catch!


https://codereview.chromium.org/11364237/diff/3001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/11364237/diff/3001/src/v8natives.js#newcode1069
src/v8natives.js:1069: var attributesClone = {};
While you're at it, could you apply the same fix here? :)

https://codereview.chromium.org/11364237/

[ad...@chromium.org]

https://codereview.chromium.org/11364237/diff/3001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/11364237/diff/3001/src/v8natives.js#newcode1069
src/v8natives.js:1069: var attributesClone = {};

On 2012/11/15 14:23:11, rossberg wrote:
> While you're at it, could you apply the same fix here? :)

But then I'd need a test :). Seems like a separate concern to me (and
my Proxy knowledge at this time in the morning is not so great)...

https://codereview.chromium.org/11364237/

[ross...@chromium.org]

I'm fine without a test for that case.

https://codereview.chromium.org/11364237/

[ad...@chromium.org]

Done.

https://codereview.chromium.org/11364237/

[ross...@chromium.org]

LGTM, will land.


https://codereview.chromium.org/11364237/diff/10001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/11364237/diff/10001/src/v8natives.js#newcode973
src/v8natives.js:973: var names = { __proto__: null }; //
TODO(rossberg): use sets once they are ready.
Actually, this case was correct. ;) But cannot harm either.

I'll fix the line length.

https://codereview.chromium.org/11364237/