Bug 141951 - Our bizarre behavior on Arguments::defineOwnProperty should be deliberate rather than a spaghetti incident
Summary: Our bizarre behavior on Arguments::defineOwnProperty should be deliberate rat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 141952 141174
  Show dependency treegraph
 
Reported: 2015-02-23 21:10 PST by Filip Pizlo
Modified: 2015-02-24 09:14 PST (History)
11 users (show)

See Also:


Attachments
WRONG PATCH (102.66 KB, patch)
2015-02-23 21:19 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (8.23 KB, patch)
2015-02-23 21:19 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (8.41 KB, patch)
2015-02-23 21:27 PST, Filip Pizlo
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-02-23 21:10:54 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2015-02-23 21:19:14 PST
Created attachment 247199 [details]
WRONG PATCH
Comment 2 Filip Pizlo 2015-02-23 21:19:54 PST
Created attachment 247200 [details]
the patch
Comment 3 Filip Pizlo 2015-02-23 21:23:04 PST
Comment on attachment 247200 [details]
the patch

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

> Source/JavaScriptCore/runtime/Arguments.cpp:308
> +                // ignore the request to change enumerability. We appear to have always do so, in

"*We appear to have always done [sic] so"

I changed this locally.
Comment 4 Filip Pizlo 2015-02-23 21:26:52 PST
Comment on attachment 247200 [details]
the patch

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

> Source/JavaScriptCore/tests/stress/arguments-bizarre-behaviour-disable-enumerability.js:25
> +function foo(x) {
> +    Object.defineProperty(arguments, 0, {configurable: true, enumerable: true, writable:true, value:42});
> +    return [x, arguments[0], arguments]
> +}
> +
> +var result = foo(1);
> +
> +if (result[0] !== 42)
> +    throw new Error();
> +
> +if (result[1] !== 42)
> +    throw new Error();
> +
> +if (Array.prototype.join.call(result[2], ",") != "42")
> +    throw new Error();
> +
> +var array = [];
> +for (var s in result[2])
> +    array.push(s);
> +
> +if (array.join(",") != "0")
> +    throw new Error();
> +
> +if (Object.keys(result[2]).join(",") != "0")
> +    throw new Error();

This was meant to be a different test, that sets enumerable to false.  I messed up when copying files around...
Comment 5 Filip Pizlo 2015-02-23 21:27:36 PST
Created attachment 247201 [details]
the patch
Comment 6 Benjamin Poulain 2015-02-23 21:42:11 PST
Comment on attachment 247201 [details]
the patch

ok
Comment 7 Filip Pizlo 2015-02-24 09:14:31 PST
Landed in http://trac.webkit.org/changeset/180564