Closed Bug 1076588 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: 446240525, Assigned: 446240525)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(1 file, 3 obsolete files)

js> Object.freeze("foo")
TypeError: "foo" is not an object      
// should return "foo" in ES6
Attached patch bug-1076588.patch (obsolete) — Splinter Review
Attachment #8498597 - Flags: review?(till)
Comment on attachment 8498597 [details] [diff] [review]
bug-1076588.patch

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

Most excellent!
Attachment #8498597 - Flags: review?(till) → review+
Attached patch fix a xpcshell test failure (obsolete) — Splinter Review
I will do another try run when the try tree reopens.
Attachment #8498597 - Attachment is obsolete: true
Attachment #8499286 - Flags: review+
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.
Attachment #8499286 - Flags: review+
Till is right, the test should not be changed.
Attachment #8499286 - Attachment is obsolete: true
Attachment #8499303 - Flags: review?(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.
Attachment #8499303 - Flags: review?(till) → review+
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.
Attachment #8499303 - Flags: review?(dteller)
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.
Attachment #8499303 - Flags: review?(dteller) → feedback+
Attachment #8499303 - Attachment is obsolete: true
Attachment #8499450 - Flags: review?(dteller)
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.
Attachment #8499450 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/1b532d05bccf
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.