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

RESOLVED FIXED in mozilla1.8rc1

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Sean McMurray, Assigned: brendan)

Tracking

({fixed1.8, js1.6})

Trunk
mozilla1.8rc1
x86
Linux
fixed1.8, js1.6
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8rc1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
Created attachment 199306 [details]
adds a __noSuchMethod__ handler to E4X XML objects
(Reporter)

Comment 2

12 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 = &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>
(Reporter)

Comment 3

12 years ago
Created attachment 199487 [details]
html file with sample js

what comment #2 was supposed to be
Attachment #199306 - Attachment is obsolete: true
(Assignee)

Comment 4

12 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

12 years ago
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
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

12 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

12 years ago
Created attachment 199573 [details] [diff] [review]
fix
Attachment #199573 - Flags: superreview?(shaver)
Attachment #199573 - Flags: review?(mrbkap)
(Assignee)

Comment 9

12 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 on attachment 199573 [details] [diff] [review]
fix

r=mrbkap
Attachment #199573 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 12

12 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
Last Resolved: 12 years ago
Flags: blocking1.8rc1?
Resolution: --- → FIXED

Comment 13

12 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

12 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

12 years ago
*** Bug 312766 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Status: REOPENED → ASSIGNED
Flags: blocking1.8rc1?
(Assignee)

Comment 16

12 years ago
Created attachment 199978 [details] [diff] [review]
overload a srcnote to distinguish explicit function::

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

/be
Attachment #199978 - Flags: superreview?(shaver)
Attachment #199978 - Flags: review?(mrbkap)
(Assignee)

Comment 17

12 years ago
*** 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+
(Assignee)

Comment 20

12 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

12 years ago
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
Attachment #200301 - Flags: superreview+
Attachment #200301 - Flags: review+
Attachment #200301 - Flags: approval1.8rc1?
(Assignee)

Updated

12 years ago
Flags: blocking1.8rc1?

Comment 22

12 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

12 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

12 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

12 years ago
Attachment #200301 - Flags: approval1.8rc1? → approval1.8rc1+
(Assignee)

Comment 25

12 years ago
Fixed on branch as well as trunk now.  Thanks, all!

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Flags: blocking1.8rc1? → blocking1.8rc1+
Keywords: fixed1.8, js1.6
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8rc1

Comment 26

11 years ago
fyi see Bug 355258

Updated

10 years ago
Summary: Trouble extending E4X XML objects with __noSuchMethod__ → Trouble extending E4X XML objects with __noSuchMethod__ (e4x/extensions/regress-312196.js)

Updated

9 years ago
Depends on: 355258

Updated

9 years ago
Blocks: 371033
You need to log in before you can comment on or make changes to this bug.