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

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