370016 - with (obj) function:: does not work with non-xml objects (e4x/Regress/regress-370016.js)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

The following session with js shell demonstrates the problem:

js> with (Math) print(function::sin(0))
with (Math) print(function::sin(0))
typein:1: ReferenceError: reference to undefined XML name @mozilla.org/js/function::sin


That is, function::name under "with" statement works only with XML-objects.

Comment 1

[Igor Bukanov]

Attachment: Fix v1

The patch changes js_FindXMLProperty to query xml objects for the function case.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> 
> The patch changes js_FindXMLProperty to query xml objects for the function
> case. 

I meant non-xml objects.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 255949 [details] [diff] [review]
Fix v1

>Index: jsinterp.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsinterp.c,v
>retrieving revision 3.332
>diff -U7 -p -r3.332 jsinterp.c
>--- jsinterp.c	21 Feb 2007 22:11:00 -0000	3.332
>+++ jsinterp.c	21 Feb 2007 22:39:04 -0000
>@@ -5596,42 +5596,44 @@ interrupt:
>             }
>             sp--;
>             STORE_OPND(-1, STRING_TO_JSVAL(str));
>           END_CASE(JSOP_ADDATTRNAME)
> 
>           BEGIN_CASE(JSOP_BINDXMLNAME)
>             lval = FETCH_OPND(-1);
>+            JS_ASSERT(JSVAL_IS_OBJECT(lval));   /* lval is qname here */

May as well define VALUE_IS_QNAME(cx,v) in jsxml.h and use it here and below.

/be

Comment 4

[Igor Bukanov]

Attachment: Fix v2

The new version of the patch delegates all the assertion checks to js_FindXMLProperty: VALUE_IS_QNAME would not work as id can be AnyName or AttributeName.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 256056 [details] [diff] [review]
Fix v2


>+    /*
>+     * FIXME This is inconsistent with GetFunction but consistent with
>+     * xml_lookupProperty.

Followup bug to cite in FIXME: comment?

> static JSBool
> xml_lookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
>                    JSProperty **propp)
> {
>+    jsval v;
>+    JSBool found;
>+    JSXML *xml;
>+    uint32 i;
>+    JSXMLQName *qn;
>+    jsid funid;
>     JSScopeProperty *sprop;
> 
>-    if (!HasProperty(cx, obj, ID_TO_VALUE(id), objp, propp))
>-        return JS_FALSE;
>+    v = ID_TO_VALUE(id);
>+    xml = (JSXML *) JS_GetPrivate(cx, obj);
>+    if (js_IdIsIndex(v, &i)) {
>+        found = HasIndexedProperty(xml, i);
>+    } else {
>+        qn = ToXMLName(cx, v, &funid);
>+        if (!qn)
>+            return JS_FALSE;
>+        if (funid)
>+            return js_LookupProperty(cx, obj, funid, objp, propp);
> 
>-    if (*propp == FOUND_XML_PROPERTY) {
>+        found = HasNamedProperty(xml, qn);
>+    }

Extra blank line here?

>+/*
>+ * nameval muts be either QName, AttributeNmae or AnyName.
>+ */
> extern JSBool
>-js_FindXMLProperty(JSContext *cx, jsval name, JSObject **objp, jsval *namep);

"must" typo, and perhaps start sentence with "NB: " or "Note: ".

/be

Comment 6

[Igor Bukanov]

Attachment: Fix v2b

Addressing the nits and replacing FIXME comments with the text explaining the diffrence with GetFunction.

The initial FIXME came from my wrong attempt to  make sure that HasFunctionProperty would return true only when GetFunction finds something. But that does not work as GetFunction can also create new objects while looking for functions/this pairs.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 256059 [details] [diff] [review]
Fix v2b

r=me, thanks for doing this -- jsxml.c is now much cleaner (I should not have tried to implement the bogo-spec and minimize changes for SpiderMonkey extensions to it).

/be

Comment 8

[Igor Bukanov]

Attachment: Fix v2c

Patch to commit with better English grammar in the just added comment. I plan to commit this later today where later means Newfoundland time - I am visiting a friend in St. Johns until 2007-03-04.

Comment 9

[Igor Bukanov]

I committed the patch from comment 8 to the trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.334; previous revision: 3.333
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.144; previous revision: 3.143
done
Checking in jsxml.h;
/cvsroot/mozilla/js/src/jsxml.h,v  <--  jsxml.h
new revision: 3.17; previous revision: 3.16
done

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-370016.js,v  <--  regress-370016.js

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 20070226 windows/mac*/linux