Closed Bug 312196 Opened 16 years ago Closed 16 years ago

Trouble extending E4X XML objects with __noSuchMethod__ (e4x/extensions/regress-312196.js)

Categories

(Core :: JavaScript Engine, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.8rc1

People

(Reporter: sean, Assigned: brendan)

References

Details

(Keywords: fixed1.8, js1.6)

Attachments

(4 files, 2 obsolete files)

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 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>
what comment #2 was supposed to be
Attachment #199306 - Attachment is obsolete: true
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
Attached patch proposed fix (obsolete) — Splinter Review
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-
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)
Attached patch fixSplinter Review
Attachment #199573 - Flags: superreview?(shaver)
Attachment #199573 - Flags: review?(mrbkap)
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 on attachment 199573 [details] [diff] [review]
fix

r=mrbkap
Attachment #199573 - Flags: review?(mrbkap) → review+
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: 16 years ago
Flags: blocking1.8rc1?
Resolution: --- → FIXED
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?
(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 → ---
*** Bug 312766 has been marked as a duplicate of this bug. ***
Status: REOPENED → ASSIGNED
Flags: blocking1.8rc1?
This is dense, but it beats adding two more opcodes.

/be
Attachment #199978 - Flags: superreview?(shaver)
Attachment #199978 - Flags: review?(mrbkap)
*** Bug 313033 has been marked as a duplicate of this bug. ***
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+
I checked attachment 199978 [details] [diff] [review] into the trunk.  Now to prepare a consolidated 1.8
branch patch.  Asa, don't panic!

/be
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?
Flags: blocking1.8rc1?
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.
/cvsroot/mozilla/js/tests/e4x/Regress/regress-312196.js,v  <--  regress-312196.js
initial revision: 1.1
done
Flags: testcase+
(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
Attachment #200301 - Flags: approval1.8rc1? → approval1.8rc1+
Fixed on branch as well as trunk now.  Thanks, all!

/be
Status: ASSIGNED → RESOLVED
Closed: 16 years ago16 years ago
Flags: blocking1.8rc1? → blocking1.8rc1+
Keywords: fixed1.8, js1.6
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8rc1
Summary: Trouble extending E4X XML objects with __noSuchMethod__ → Trouble extending E4X XML objects with __noSuchMethod__ (e4x/extensions/regress-312196.js)
Depends on: 355258
Blocks: 371033
You need to log in before you can comment on or make changes to this bug.