512389 - Remove JSStackFrame::xmlNamespace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Here's my attempt at this after talking to Brendan. It passes the E4X tests.

I added an assert in js_GetDefaultXmlNamespace because it seemed like obj could not be NULL - if I'm wrong, please let me know what to use there instead.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 396375 [details] [diff] [review]
patch

>+++ b/js/src/jsparse.cpp
>@@ -5410,6 +5410,8 @@
>                                         JSMSG_BAD_DEFAULT_XML_NAMESPACE);
>             return NULL;
>         }
>+        /* Is this an E4X dagger I see before me? */
>+        tc->flags |= TCF_FUN_HEAVYWEIGHT;

Hee hee -- but add a blank line before the comment.

Need at least

[defaults]
diff = -U 8 -p

and probably

[diff]
git = 1
showfunc = true

in your .hgrc still:

>@@ -7577,11 +7577,6 @@
>     jsval v;
> 
>     fp = js_GetTopStackFrame(cx);
> 
>     obj = NULL;
>     for (tmp = fp->scopeChain; tmp; tmp = OBJ_GET_PARENT(cx, tmp)) {
>@@ -7591,23 +7586,21 @@
>         if (!tmp->getProperty(cx, JS_DEFAULT_XML_NAMESPACE_ID, &v))
>             return JS_FALSE;
>         if (!JSVAL_IS_PRIMITIVE(v)) {
>-            fp->xmlNamespace = JSVAL_TO_OBJECT(v);
>             *vp = v;
>             return JS_TRUE;
>         }
>         obj = tmp;
>     }
> 
>+    JS_ASSERT(obj);

Don't add the assert, we'd prefer a null deref crash below -- but you are right that the loop exits only when tmp is null where tmp = OBJ_GET_PARENT(cx, obj), *and* (crucial point, so it could be a do-while but test-at-top is best here) fp->scopeChain is never null, so obj must be non-null.

>     ns = js_ConstructObject(cx, &js_NamespaceClass.base, NULL, obj, 0, NULL);
>     if (!ns)
>         return JS_FALSE;
>     v = OBJECT_TO_JSVAL(ns);
>-    if (obj &&
>-        !obj->defineProperty(cx, JS_DEFAULT_XML_NAMESPACE_ID, v, JS_PropertyStub, JS_PropertyStub,
>+    if (!obj->defineProperty(cx, JS_DEFAULT_XML_NAMESPACE_ID, v, JS_PropertyStub, JS_PropertyStub,
>                              JSPROP_PERMANENT, NULL)) {
>         return JS_FALSE;

Good change, better than being paranoid as I was in 2004 about null scope chains.

>@@ -7628,15 +7621,11 @@
> 
>     fp = js_GetTopStackFrame(cx);
>     varobj = fp->varobj;
>-    if (varobj) {
>-        if (!varobj->defineProperty(cx, JS_DEFAULT_XML_NAMESPACE_ID, v,
>-                                    JS_PropertyStub, JS_PropertyStub, JSPROP_PERMANENT, NULL)) {
>-            return JS_FALSE;
>-        }
>-    } else {
>-        JS_ASSERT(fp->fun && !JSFUN_HEAVYWEIGHT_TEST(fp->fun->flags));
>-    }
>-    fp->xmlNamespace = JSVAL_TO_OBJECT(v);
>+    JS_ASSERT(varobj);
>+    if (!varobj->defineProperty(cx, JS_DEFAULT_XML_NAMESPACE_ID, v,
>+                                JS_PropertyStub, JS_PropertyStub, JSPROP_PERMANENT, NULL)) {
>+        return JS_FALSE;
>+    }

Ditto, again no need to assert non-null just before dereferencing.

r=me with these nitpicks. Thanks for doing this!

/be

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #2)
> [diff]
> git = 1
> showfunc = true

Was missing this, thanks.

Pushed http://hg.mozilla.org/tracemonkey/rev/0d4455046e03 with nits.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/0d4455046e03

Comment 5

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4027504879f2