609024 - JS_DeepFreezeObject impl is not correct

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Franck]

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;

Comment 1

[Brendan Eich [:brendan]]

(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

Comment 2

[Brendan Eich [:brendan]]

Whoops.

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

Waldo, help!

/be

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 4

[Brendan Eich [:brendan]]

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 5

[Jeff Walden [:Waldo]]

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).

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v2

Comment 7

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/8b5109ee9aca

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8b5109ee9aca