1060873 - Object.isExtensible() should return false when given primitive values as input

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Comment 1

[ziyunfei]

js> Object.isExtensible("foo")
TypeError: "foo" is not an object
// should return false in ES6

Comment 2

[ziyunfei]

(In reply to ziyunfei from comment #0)
> 

I pressed the enter key by mistake.

Comment 3

[ziyunfei]

Attachment: bug1060873.patch

Comment 4

[ziyunfei]

Added a note in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/isExtensible#Examples

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8481879 [details] [diff] [review]
bug1060873.patch

Review of attachment 8481879 [details] [diff] [review]:
-----------------------------------------------------------------

Great, the change looks good. It requires a few more test changes, though, as there are existing tests that now fail.

Going forward, it'd be great if you could run the jstest and jit-test suites before asking for a review. See [1] for more info on that.

thanks again, these changes are much appreciated!

[1]: https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey#Testing_your_changes

Comment 6

[ziyunfei]

(In reply to Till Schneidereit [:till] from comment #5)
> Great, the change looks good. It requires a few more test changes, though,
> as there are existing tests that now fail.

Hi till, could you tell me which test(s) failed for you? Cause I did run the test suites and only saw:

REGRESSIONS
    js1_5/extensions/toLocaleFormat-01.js
    js1_5/extensions/toLocaleFormat-02.js
FAIL

Comment 7

[Till Schneidereit [:till]]

(In reply to ziyunfei from comment #6)
> Hi till, could you tell me which test(s) failed for you? Cause I did run the
> test suites and only saw:
> 
> REGRESSIONS
>     js1_5/extensions/toLocaleFormat-01.js
>     js1_5/extensions/toLocaleFormat-02.js
> FAIL

Interestingly, one of the test failures I saw is entirely unrelated to your patch, but happens on my machine only. I'm investigating this.

The other was in fact your newly-added test. It uses a function `assertFalse`, which doesn't seem to exist. Can you change the tests to use `assertEq(Object.isExtensible(), false)`?

Also, symbols are disabled for Firefox 33 and might not make Firefox 34, either. Can you put the Symbol-related test into an if block along the lines of `if (typeof Symbol !== "undefined") { [test here] }`?

Comment 8

[ziyunfei]

Attachment: bug1060873 v2.patch

Comment 9

[ziyunfei]

Attachment: bug1060873 v3.patch

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8482665 [details] [diff] [review]
bug1060873 v3.patch

Review of attachment 8482665 [details] [diff] [review]:
-----------------------------------------------------------------

Now the test isn't running properly at all anymore, because you introduced various syntax errors. Please make sure the final version you attach successfully runs.

Comment 11

[ziyunfei]

Attachment: bug1060873 v4.patch

Comment 12

[ziyunfei]

(In reply to Till Schneidereit [:till] from comment #10)
> Comment on attachment 8482665 [details] [diff] [review]
> bug1060873 v3.patch
> 
> Review of attachment 8482665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Now the test isn't running properly at all anymore, because you introduced
> various syntax errors. Please make sure the final version you attach
> successfully runs.

Sorry, it's my bad. I promise I will be more careful next time. 
Thanks for your patience!

Comment 13

[Till Schneidereit [:till]]

Comment on attachment 8482713 [details] [diff] [review]
bug1060873 v4.patch

Review of attachment 8482713 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks for the patch and bearing with me on the change requests.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e75a5c333c8

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e75a5c333c8