Closed
Bug 312196
Opened 19 years ago
Closed 19 years ago
Trouble extending E4X XML objects with __noSuchMethod__ (e4x/extensions/regress-312196.js)
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.8rc1
People
(Reporter: sean, Assigned: brendan)
References
Details
(Keywords: fixed1.8, js1.6)
Attachments
(4 files, 2 obsolete files)
793 bytes,
text/html
|
Details | |
16.40 KB,
patch
|
mrbkap
:
review+
shaver
:
superreview+
|
Details | Diff | Splinter Review |
10.44 KB,
patch
|
mrbkap
:
review+
shaver
:
superreview+
|
Details | Diff | Splinter Review |
27.10 KB,
patch
|
brendan
:
review+
brendan
:
superreview+
asa
:
approval1.8rc1+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
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 = <request method={id}/>; > for(var i=0; i<arguments[1].length; i++){ > request += <parameter>{arguments[1][i]}</parameter>; > } > var xml = this; > xml.request = request; > alert(xml.toString()); > }; > > var ws = new ajaxObj('<object uri="http://localhost"/>'); 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>
Reporter | ||
Comment 3•19 years ago
|
||
what comment #2 was supposed to be
Attachment #199306 -
Attachment is obsolete: true
Assignee | ||
Comment 4•19 years ago
|
||
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
Assignee: general → brendan
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 5•19 years ago
|
||
Not a minimal patch, but the right one. E4X really did a number on method name lookup.... /be
Attachment #199523 -
Flags: superreview?(shaver)
Attachment #199523 -
Flags: review?(mrbkap)
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.
Attachment #199523 -
Flags: superreview?(shaver) → superreview-
Assignee | ||
Comment 7•19 years ago
|
||
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
Attachment #199523 -
Attachment is obsolete: true
Attachment #199523 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•19 years ago
|
||
Attachment #199573 -
Flags: superreview?(shaver)
Attachment #199573 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•19 years ago
|
||
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 on attachment 199573 [details] [diff] [review] fix sr=shaver, though mrbkap should look closely at the decompiler because I didn't!
Attachment #199573 -
Flags: superreview?(shaver) → superreview+
Comment 11•19 years ago
|
||
Comment on attachment 199573 [details] [diff] [review] fix r=mrbkap
Attachment #199573 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 12•19 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 19 years ago
Flags: blocking1.8rc1?
Resolution: --- → FIXED
Comment 13•19 years ago
|
||
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?
Assignee | ||
Comment 14•19 years ago
|
||
(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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 15•19 years ago
|
||
*** Bug 312766 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•19 years ago
|
Status: REOPENED → ASSIGNED
Flags: blocking1.8rc1?
Assignee | ||
Comment 16•19 years ago
|
||
This is dense, but it beats adding two more opcodes. /be
Attachment #199978 -
Flags: superreview?(shaver)
Attachment #199978 -
Flags: review?(mrbkap)
Assignee | ||
Comment 17•19 years ago
|
||
*** Bug 313033 has been marked as a duplicate of this bug. ***
Comment 18•19 years ago
|
||
Comment on attachment 199978 [details] [diff] [review] overload a srcnote to distinguish explicit function:: I think I get it!
Attachment #199978 -
Flags: review?(mrbkap) → review+
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
Attachment #199978 -
Flags: superreview?(shaver) → superreview+
Assignee | ||
Comment 20•19 years ago
|
||
I checked attachment 199978 [details] [diff] [review] into the trunk. Now to prepare a consolidated 1.8 branch patch. Asa, don't panic! /be
Assignee | ||
Comment 21•19 years ago
|
||
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
Attachment #200301 -
Flags: superreview+
Attachment #200301 -
Flags: review+
Attachment #200301 -
Flags: approval1.8rc1?
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8rc1?
Comment 22•19 years ago
|
||
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•19 years ago
|
||
/cvsroot/mozilla/js/tests/e4x/Regress/regress-312196.js,v <-- regress-312196.js initial revision: 1.1 done
Flags: testcase+
Assignee | ||
Comment 24•19 years ago
|
||
(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
Updated•19 years ago
|
Attachment #200301 -
Flags: approval1.8rc1? → approval1.8rc1+
Assignee | ||
Comment 25•19 years ago
|
||
Fixed on branch as well as trunk now. Thanks, all! /be
Comment 26•18 years ago
|
||
fyi see Bug 355258
Updated•17 years ago
|
Summary: Trouble extending E4X XML objects with __noSuchMethod__ → Trouble extending E4X XML objects with __noSuchMethod__ (e4x/extensions/regress-312196.js)
You need to log in
before you can comment on or make changes to this bug.
Description
•