Last Comment Bug 312196 - Trouble extending E4X XML objects with __noSuchMethod__ (e4x/extensions/regress-312196.js)
: Trouble extending E4X XML objects with __noSuchMethod__ (e4x/extensions/regre...
Status: RESOLVED FIXED
: fixed1.8, js1.6
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: P2 normal (vote)
: mozilla1.8rc1
Assigned To: Brendan Eich [:brendan]
:
Mentors:
: 312766 313033 (view as bug list)
Depends on: 355258
Blocks: 371033
  Show dependency treegraph
 
Reported: 2005-10-12 09:56 PDT by Sean McMurray
Modified: 2011-08-05 21:30 PDT (History)
8 users (show)
brendan: blocking1.8rc1+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
adds a __noSuchMethod__ handler to E4X XML objects (641 bytes, text/html)
2005-10-12 09:58 PDT, Sean McMurray
no flags Details
html file with sample js (793 bytes, text/html)
2005-10-13 17:09 PDT, Sean McMurray
no flags Details
proposed fix (16.35 KB, patch)
2005-10-13 23:51 PDT, Brendan Eich [:brendan]
shaver: superreview-
Details | Diff | Splinter Review
fix (16.40 KB, patch)
2005-10-14 10:55 PDT, Brendan Eich [:brendan]
mrbkap: review+
shaver: superreview+
Details | Diff | Splinter Review
overload a srcnote to distinguish explicit function:: (10.44 KB, patch)
2005-10-18 13:48 PDT, Brendan Eich [:brendan]
mrbkap: review+
shaver: superreview+
Details | Diff | Splinter Review
branch roll-up patch (27.10 KB, patch)
2005-10-20 17:54 PDT, Brendan Eich [:brendan]
brendan: review+
brendan: superreview+
asa: approval1.8rc1+
Details | Diff | Splinter Review

Description Sean McMurray 2005-10-12 09:56:17 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

Assigning a __noSuchMethod__ property to an XML object does not seem to work.

Reproducible: Always

Steps to Reproduce:
1. Load sample.
Actual Results:  
JS error: ws.sample is not a function

Expected Results:  
invoked the __noSuchMethod__ handler.
Comment 1 Sean McMurray 2005-10-12 09:58:00 PDT
Created attachment 199306 [details]
adds a __noSuchMethod__ handler to E4X XML objects
Comment 2 Sean McMurray 2005-10-13 17:05:49 PDT
Comment on attachment 199306 [details]
adds a __noSuchMethod__ handler to E4X XML objects

><HTML><HEAD/><BODY>
>        <SCRIPT type="text/javascript;e4x=1">
>			function ajaxObj(content){
>				var ajax = new XML(content);
>				ajax.function::__noSuchMethod__ = autoDispatch;
>				return ajax;
>			}
>
>			function autoDispatch(id){
>				if (! this.@uri)
>					return;
>				var request = &lt;request method={id}/&gt;;
>				for(var i=0; i&lt;arguments[1].length; i++){
>					request += &lt;parameter&gt;{arguments[1][i]}&lt;/parameter&gt;;
>				}
>				var xml = this;
>				xml.request = request;
>				alert(xml.toString());
>			};
>
>			var ws = new ajaxObj('&lt;object uri="http://localhost"/&gt;');
			var test = ws.function::sample('this', 'is', 'a',
'test');
>			var test = ws.sample('this', 'is', 'a', 'test');
>		</SCRIPT>
		Open js console and reload this page.<p/>
		The offending js is visible in view source.
>	
></BODY></HTML>
Comment 3 Sean McMurray 2005-10-13 17:09:35 PDT
Created attachment 199487 [details]
html file with sample js

what comment #2 was supposed to be
Comment 4 Brendan Eich [:brendan] 2005-10-13 19:17:02 PDT
E4X sucks, in that it hides methods and always wraps property values in an
XMLList.  The __noSuchMethod__ you define is not working because while it is
well-defined in the function:: namespace, it is not invoked by that qualified
name.  This will be ugly to fix -- initial plan is to hack in an OBJECT_IS_XML
test and qualify for that ugly case.

/be
Comment 5 Brendan Eich [:brendan] 2005-10-13 23:51:34 PDT
Created attachment 199523 [details] [diff] [review]
proposed fix

Not a minimal patch, but the right one.  E4X really did a number on method name
lookup....

/be
Comment 6 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-14 10:13:54 PDT
Comment on attachment 199523 [details] [diff] [review]
proposed fix


>+          BEGIN_LITOPX_CASE(JSOP_SETMETHOD, 0)
>+            /* Get an immediate atom naming the property. */
>+            id   = ATOM_TO_JSID(atom);
>+            rval = FETCH_OPND(-1);
>+            FETCH_OBJECT(cx, -2, lval, obj);
>+            SAVE_SP(fp);
>+
>+            /* Special-case XML object method lookup, per ECMA-357. */
>+            if (OBJECT_IS_XML(cx, obj)) {
>+                JSXMLObjectOps *ops;
>+
>+                ops = (JSXMLObjectOps *) obj->map->ops;
>+                ok = ops->setMethod(cx, obj, id, &rval);
>+            } else {
>+                CACHED_GET(OBJ_SET_PROPERTY(cx, obj, id, &rval));
>+            }

CACHED_SET instead?

More review after my meetings this AM.
Comment 7 Brendan Eich [:brendan] 2005-10-14 10:52:10 PDT
Comment on attachment 199523 [details] [diff] [review]
proposed fix

Oops.  Another fix in the next patch to this one: set vp[1] to root thisp on
successful return from getMethod in js_Invoke's __noSuchMethod__ block.

/be
Comment 8 Brendan Eich [:brendan] 2005-10-14 10:55:25 PDT
Created attachment 199573 [details] [diff] [review]
fix
Comment 9 Brendan Eich [:brendan] 2005-10-14 11:14:48 PDT
Comment on attachment 199573 [details] [diff] [review]
fix

Interdiff says it all:
https://bugzilla.mozilla.org/attachment.cgi?oldid=199523&action=interdiff&newid
=199573&headers=1

/be
Comment 10 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-14 17:24:34 PDT
Comment on attachment 199573 [details] [diff] [review]
fix

sr=shaver, though mrbkap should look closely at the decompiler because I
didn't!
Comment 11 Blake Kaplan (:mrbkap) 2005-10-14 23:35:09 PDT
Comment on attachment 199573 [details] [diff] [review]
fix

r=mrbkap
Comment 12 Brendan Eich [:brendan] 2005-10-15 00:34:16 PDT
Fixed on the trunk.  Would like to fix for 1.8, or something that releases soon
enough off it into the Firefox 1.5+ patch-stream.  Thinking about going for rc1
approval.  Nominating on that basis.

The bug is that E4X, new in 1.8/fx1.5, can't be used with the pre-existing hook
for handling a call to an undefined method on an object, __noSuchMethod__.  The
combination is powerful, and the lack can't be worked around at all.  The patch
is totally confined to __noSuchMethod__ and E4X (function::foo property get and
set) code paths, so it's not going to break anything else in JS.

/be
Comment 13 Dorando 2005-10-16 04:57:10 PDT
This seems to cause "(function a() {this.b();}).toSource();" to return
"(function a() {this.function::b();})" (was "(function a() {this.b();})") if
entered in the JavaScript Console, was this intended?
Comment 14 Brendan Eich [:brendan] 2005-10-17 14:58:09 PDT
(In reply to comment #13)
> This seems to cause "(function a() {this.b();}).toSource();" to return
> "(function a() {this.function::b();})" (was "(function a() {this.b();})") if
> entered in the JavaScript Console, was this intended?

It's benign, but it is neither backward-compatible nor minimal.  Reopening.

/be
Comment 15 Bob Clary [:bc:] 2005-10-17 15:03:44 PDT
*** Bug 312766 has been marked as a duplicate of this bug. ***
Comment 16 Brendan Eich [:brendan] 2005-10-18 13:48:37 PDT
Created attachment 199978 [details] [diff] [review]
overload a srcnote to distinguish explicit function::

This is dense, but it beats adding two more opcodes.

/be
Comment 17 Brendan Eich [:brendan] 2005-10-19 15:09:22 PDT
*** Bug 313033 has been marked as a duplicate of this bug. ***
Comment 18 Blake Kaplan (:mrbkap) 2005-10-19 18:27:50 PDT
Comment on attachment 199978 [details] [diff] [review]
overload a srcnote to distinguish explicit function::

I think I get it!
Comment 19 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-20 12:21:38 PDT
Comment on attachment 199978 [details] [diff] [review]
overload a srcnote to distinguish explicit function::

I wonder if the future archaeologists will revere the decompiler as the
Platonic ideal of Internet-era Baroque.

sr=shaver
Comment 20 Brendan Eich [:brendan] 2005-10-20 12:44:44 PDT
I checked attachment 199978 [details] [diff] [review] into the trunk.  Now to prepare a consolidated 1.8
branch patch.  Asa, don't panic!

/be
Comment 21 Brendan Eich [:brendan] 2005-10-20 17:54:21 PDT
Created attachment 200301 [details] [diff] [review]
branch roll-up patch

To recap: SpiderMonkey supports __noSuchMethod__ as a "default method handler",
for calling obj.foo() where foo is not defined.  E4X, per spec, hids methods in
what is effectively an unnamed namespace in the XML.prototype object.  Our E4X
implementation allows methods to be defined in an explicit function::-qualified
namespace, both to access the E4X prototype methods, and to shadow them in any
XML object that delegates to that prototype.

This patch fixes bugs in the interaction of the two features.  It affects code
paths peculiar to those features, and a common set of paths involving "method"
property access.  It tests well, and the changes to common code paths are small
enough to inspect.  Without the fix, there is no workaround, either for using
__noSuchMethod__ with E4X (something AJAX developers want), or for overriding
methods with function::-qualified property assignments.

The lack of workarounds motivates this request to get fixed in 1.8.  The major
trunk patch landed five days ago.  The followup fix is for a decompiler change
that is not backward compatible, and is verbose (always emitting function::,
even when it wasn't used, for method calls).  That's in too, and it tests as
fixed.

The risks at this point are minimal: something else wrong with the decompiler's
output that we don't test for and haven't noticed.

/be
Comment 22 shutdown 2005-10-21 00:08:49 PDT
var o = <x/>;
alert(typeof(o.copy)+"/"+typeof(o.function::copy)); // "xml/function"
o.function::copy = function(){/*...*/};
alert(typeof(o.copy)+"/"+typeof(o.function::copy)); // "function/function"

http://lxr.mozilla.org/seamonkey/source/js/src/jsxml.c#3850
3850         /* XXXbe really want a separate scope for function::*. */

Fix for this bug exposed the above problem.
Comment 23 Bob Clary [:bc:] 2005-10-21 02:12:14 PDT
/cvsroot/mozilla/js/tests/e4x/Regress/regress-312196.js,v  <--  regress-312196.js
initial revision: 1.1
done
Comment 24 Brendan Eich [:brendan] 2005-10-21 09:42:48 PDT
(In reply to comment #22)
> var o = <x/>;
> alert(typeof(o.copy)+"/"+typeof(o.function::copy)); // "xml/function"
> o.function::copy = function(){/*...*/};
> alert(typeof(o.copy)+"/"+typeof(o.function::copy)); // "function/function"
> 
> http://lxr.mozilla.org/seamonkey/source/js/src/jsxml.c#3850
> 3850         /* XXXbe really want a separate scope for function::*. */
> 
> Fix for this bug exposed the above problem.

That's right, but that is a lesser bug that we can fix later.  The likelihood of
name collisions between functions and XML children is low and controllable.  The
lack of a workaround for this bug makes the probability of failure to extent XML
objects with default method handlers 1.

/be
Comment 25 Brendan Eich [:brendan] 2005-10-21 16:31:12 PDT
Fixed on branch as well as trunk now.  Thanks, all!

/be
Comment 26 Bob Clary [:bc:] 2006-12-01 11:15:50 PST
fyi see Bug 355258

Note You need to log in before you can comment on or make changes to this bug.