355145 - JS_GetMethodById() on XML objects no longer works

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[shutdown]

regression from bug 355075.

var obj = <x/>, exp = "foo", act;
obj.function::__iterator__ = function() { yield exp; };
for(var val in obj) act = val;
print("expect: " + exp);
print("actual: " + act);

jsapi.c rev 3.280:
  expect: foo
  actual: foo
jsapi.c rev 3.281:
  expect: foo
  actual: undefined

Comment 1

[Igor Bukanov]

Attachment: Fix v1

Instead of reintroducing __iterator__ for XML which would require special checks to ensure that it is a default iterator, the fix calls js_LookupProperty on XML objects.

Comment 2

[Brendan Eich [:brendan]]

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

>+    /*
>+     * The function should not give any warnings if the method does not exist.
>+     * Hence we do extra OBJ_LOOKUP_PROPERTY to check for that as otherwise
>+     * OBJ_GET_PROPERTY when it calls js_GetProperty would produce a warning,
>+     * see bug 355075.
>+     *
>+     * For XML case we use js_LookupProperty since there OBJ_LOOKUP_PROPERTY
>+     * query property namespace, not function namespace, see bug 355145.

Nit: full stop, then "See bug ..." instead of ", see bug ..." run-ons.

>+     */
> #if JS_HAS_XML_SUPPORT
>     if (OBJECT_IS_XML(cx, obj)) {
>         JSXMLObjectOps *ops;
> 
>-        ops = (JSXMLObjectOps *) obj->map->ops;
>-        obj = ops->getMethod(cx, obj, id, vp);
>-        if (!obj)
>+        if (!js_LookupProperty(cx, obj, id, &obj2, &prop))
>             return JS_FALSE;
>+        if (!prop) {
>+            *vp = JSVAL_VOID;
>+        } else {
>+            /* No OBJ_DROP_PROPERTY as js_LookupProperty does not have one. */

js_LookupProperty does leave obj2 locked on return with prop non-null, so you do need JS_UNLOCK_OBJ(cx, obj2) if not OBJ_DROP_PROPERTY here.  Bypassing JSObjectOps via js_LookupProperty suggests using the same style and calling JS_UNLOCK_OBJ(cx, obj2).

>+            ops = (JSXMLObjectOps *) obj->map->ops;
>+            obj = ops->getMethod(cx, obj, id, vp);
>+            if (!obj)
>+                return JS_FALSE;
>+        }

/be

Comment 3

[Igor Bukanov]

Attachment: Fix v2

I was so wrong about DROP_PROPERTY up to now. My impression was that JSObject does not define own method since the macro explicitly checks for null. In fact  js_ObjectOps speaks quite loudly about js_DropProperty.

Comment 4

[shutdown]

(In reply to comment #3)
> Created an attachment (id=241076) [edit]
> Fix v2

This will not work because XML/XMLList object may delegate
the method invocation to the XML/string object it contains.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsxml.c&rev=3.126&cvsroot=/cvsroot#5251

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> (In reply to comment #3)
> > Created an attachment (id=241076) [edit]
> > Fix v2
> 
> This will not work because XML/XMLList object may delegate
> the method invocation to the XML/string object it contains.

Very good point. So the best way to fix this would be to suppress the warning in js_GetProperty but for now I suggest to leave the bug open as a rather obscure problem.

Comment 6

[Brendan Eich [:brendan]]

The warning can be avoided in js_GetProperty with some ingenuity.  The cost of an added test there is small, and should be born only by those who turn on the strict option.

/be

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> The warning can be avoided in js_GetProperty with some ingenuity.  The cost of
> an added test there is small, and should be born only by those who turn on the
> strict option.

A trivial hack: do not warn about __iterator__ property.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (In reply to comment #6)
> > The warning can be avoided in js_GetProperty with some ingenuity.  The cost of
> > an added test there is small, and should be born only by those who turn on the
> > strict option.
> 
> A trivial hack: do not warn about __iterator__ property.

Sure.

/be

Comment 9

[Brendan Eich [:brendan]]

Given that trivial fix to js_GetProperty, jsapi.c can go back to rev 3.280.

/be

Comment 10

[Igor Bukanov]

Attachment: Fix v3

This stops warnings about __iterator__ in js_GetProperty and reverts jsapi.c to the state before the commit from bug 355075.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 241110 [details] [diff] [review]
Fix v3

>+                 * XXX: do not warn about missing __iterator__ as the function

Ultra-nit: XXX doesn't need a : after it.

Very safe fix to a last-minute regression for 1.8.1 if there's still time, else for 1.8.1.1.

/be

Comment 12

[Igor Bukanov]

Attachment: Fix v3b

Patch to commit with comment text fix.

Comment 13

[Brendan Eich [:brendan]]

Looking for blocking1.8.1- and blocking1.8.1.1+, or ideally blocking1.8.1+ and remove the blocking1.8.1.1? nomination.

/be

Comment 14

[Igor Bukanov]

I committed the patch from comment 12 to the trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.282; previous revision: 3.281
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.291; previous revision: 3.290
done

Comment 15

[Igor Bukanov]

Comment on attachment 241119 [details] [diff] [review]
Fix v3b

Asking approval for the committed patch.

Comment 16

[Bob Clary [:bc] (inactive)]

Checking in regress-355145.js;
/cvsroot/mozilla/js/tests/js1_7/regress/regress-355145.js,v  <--  regress-355145.js
initial revision: 1.1
done

Comment 17

[Mike Connor [:mconnor]]

we already have respun RC2 bits in hand, will get this into 1.8.1.1

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 241119 [details] [diff] [review]
Fix v3b

please renom once there's a 1.8.1.1 flag

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #18)
> (From update of attachment 241119 [details] [diff] [review] [edit])
> please renom once there's a 1.8.1.1 flag

Let's have that flag sooner rather than later, eh?

/be

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 241119 [details] [diff] [review]
Fix v3b

Annnd, stay down. Igor, please nominate this for 1.8.1.1 when the flag comes into existence.

Comment 21

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061005 windows/linux

Comment 22

[Igor Bukanov]

*** Bug 356129 has been marked as a duplicate of this bug. ***

Comment 23

[Igor Bukanov]

*** Bug 355913 has been marked as a duplicate of this bug. ***

Comment 24

[Igor Bukanov]

Given that this already got 2 dups, I suggest to reconsider this for 1.8.1. The patch effectively restore JS_GetMethodById to the state it had before all the rest patch flood and ads very safe check not to warn about __iterator__.

Comment 25

[Igor Bukanov]

Comment on attachment 241119 [details] [diff] [review]
Fix v3b

Nominating for 1.8.1 to avoid more bug reports about the strict warning before 1.8.1.1

Comment 26

[Igor Bukanov]

*** Bug 355913 has been marked as a duplicate of this bug. ***

Comment 27

[Mike Schroepfer]

Comment on attachment 241119 [details] [diff] [review]
Fix v3b

Approved for RC3.

Comment 28

[Igor Bukanov]

I committed the patch from comment 12 to MOZILLA_1_8_BRANCH:

/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.214.2.28; previous revision: 3.214.2.27
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.37; previous revision: 3.208.2.36
done

Comment 29

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20061010 windows/mac*/linux

Comment 30

[Reed Loden [:reed]]

Clearing blocking flag for 1.8.1.1, as this made 1.8.1. If this is incorrect, please fwap me.