Closed Bug 609024 Opened 11 years ago Closed 10 years ago

JS_DeepFreezeObject impl is not correct

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: soubok, Assigned: jorendorff)

References

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 file, 1 obsolete file)

At the beginning of JS_DeepFreezeObject() you have the following comment:
 "Assume that non-extensible objects are already deep-frozen, to avoid divergence"

that contradict the related condition:
  if (obj->isExtensible())
    return true;
(In reply to comment #0)
> At the beginning of JS_DeepFreezeObject() you have the following comment:
>  "Assume that non-extensible objects are already deep-frozen, to avoid
> divergence"
> 
> that contradict the related condition:
>   if (obj->isExtensible())
>     return true;

The comment applies to that if statement:

    /* Assume that non-extensible objects are already deep-frozen, to avoid divergence. */
    if (obj->isExtensible())
        return true;

Are you complaining about the comment's wording, or (as the summary of this bug says) the code being incorrect?

This is based on the old JS_SealObject(..., true) API, and it made the same assumption. We are not trying to make this more complete or "correct" than it was for years. What problem are you trying to solve here, or are we missing some kind of regression that was recently introduced?

/be
Whoops.

I mentally put a ! in that if condition. Also when reviewing.

Waldo, help!

/be
Assignee: general → jwalden+bmo
Blocks: es5
Attached patch v1 (obsolete) — Splinter Review
Assignee: jwalden+bmo → jorendorff
Attachment #488017 - Flags: review?(jwalden+bmo)
Comment on attachment 488017 [details] [diff] [review]
v1

> BEGIN_TEST(testDeepFreeze_bug535703)
> {
>-    JSObject *obj = JS_NewObject(cx, NULL, NULL, NULL);
>-    CHECK(obj);
>-    JS_DeepFreezeObject(cx, obj);  // don't crash
>+    jsval v;
>+    EVAL("var x = {}; x;", &v);
>+    CHECK(JS_DeepFreezeObject(cx, JSVAL_TO_OBJECT(v)));  // don't crash
>+    EVAL("Object.isFrozen(x)", &v);
>+    CHECK_SAME(v, JSVAL_TRUE);

This doesn't test deepness, though -- can it?

/be
Comment on attachment 488017 [details] [diff] [review]
v1

What Brendan said, but otherwise fine (assuming JS_DeepFreezeObject's requirement that non-extensible always means frozen is actually well-documented).
Attachment #488017 - Flags: review?(jwalden+bmo) → review-
Attached patch v2Splinter Review
Attachment #488017 - Attachment is obsolete: true
Attachment #491526 - Flags: review?(jwalden+bmo)
Attachment #491526 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/tracemonkey/rev/8b5109ee9aca
Whiteboard: [fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/8b5109ee9aca
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.