1076588 - Object.freeze() should return its argument with no conversion when the argument is a primitive value

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[ziyunfei]

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

Comment 1

[ziyunfei]

Attachment: bug-1076588.patch

Comment 2

[ziyunfei]

https://tbpl.mozilla.org/?tree=Try&rev=df4d6ed18c9d

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8498597 [details] [diff] [review]
bug-1076588.patch

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

Most excellent!

Comment 4

[ziyunfei]

Attachment: fix a xpcshell test failure

I will do another try run when the try tree reopens.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8499286 [details] [diff] [review]
fix a xpcshell test failure

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

I'm worried about the test change and what it might mean for the behavior that's being tested. Can you explain what's going on here?

::: toolkit/components/crashmonitor/test/unit/test_invalid_file.js
@@ +7,5 @@
>   * Test with sessionCheckpoints.json containing invalid data
>   */
>  add_task(function test_invalid_file() {
>    // Write bogus data to checkpoint file
> +  let data = '"1234\x00"';

Wait, does this mean that the file is considered valid with the Object.freeze change, but without this one? If so, I don't think that's ok, and we should change things inside of OS.File.writeAtomic (I guess) so that its behavior doesn't change.

Comment 6

[ziyunfei]

Attachment: manually throw if the argument passed to Object.freeze here isn't an object

Till is right, the test should not be changed.

Comment 7

[ziyunfei]

https://tbpl.mozilla.org/?tree=Try&rev=05496597b807

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8499303 [details] [diff] [review]
manually throw if the argument passed to Object.freeze here isn't an object

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

Yeah, that's better.

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8499303 [details] [diff] [review]
manually throw if the argument passed to Object.freeze here isn't an object

Oh, but now we actually need a review from a peer of the CrashMonitor. Based on hg blame, Yoric is one.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8499303 [details] [diff] [review]
manually throw if the argument passed to Object.freeze here isn't an object

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

This is indeed an error case. However, I suspect that throwing an error here might prevent Firefox from starting, so I'd rather you called `Cu.reportError` and returned `null`, as when parsing fails.

Comment 11

[ziyunfei]

Attachment: use Cu.reportError

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8499450 [details] [diff] [review]
use Cu.reportError

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

Looks good (I only reviewed CrashMonitor and test).

::: toolkit/components/crashmonitor/test/unit/test_invalid_file.js
@@ +13,5 @@
>                              {tmpPath: sessionCheckpointsPath + ".tmp"});
>  
> +  // An invalid file will cause |init| to return null
> +  let status = yield CrashMonitor.init();
> +  do_check_true(status === null ? true : false);

Ah, good catch.

Comment 13

[ziyunfei]

All green try run: https://tbpl.mozilla.org/?tree=Try&rev=65715eb8aee1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b532d05bccf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b532d05bccf