370372 - function::name assignments do not work under with (xmllist) (e4x/Regress/regress-370372.js)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Consider the following test case:

var tests = [<></>, <><a/></>, <><a/><b/></>]

function do_one(x)
{
  with (x) {
    function::toString = function::f = function() { return "test"; }
  }

  if (String(x) !== "test")
    throw "Failed to set toString";

  if (x.f() !== "test")
    throw "Failed to set set function f";

  if (x.function::toString != x.function::f)
    throw "toString and f are different";
}


for (var i  = 0; i != tests.length; ++i)
  do_one(tests[i]);


Currently it fails with:

~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/x.js
/home/igor/s/x.js:9: ReferenceError: reference to undefined XML name @mozilla.org/js/function::toString

The reason for this is that PutProperty from jsxml.c does not treat function namespaces specially.

Comment 1

[Igor Bukanov]

The above test is incorrect as it assumes that assignments to non-existing property function::f is OK under with. This is wrong and here is the right one:

var tests = [<></>, <><a/></>, <><a/><b/></>]

function do_one(x)
{
  x.function::f = Math.sin;
  with (x) {
    function::toString = function::f = function() { return "test"; }
  }

  if (String(x) !== "test")
    throw "Failed to set toString";

  if (x.f() !== "test")
    throw "Failed to set set function f";

  if (x.function::toString != x.function::f)
    throw "toString and f are different";
}


for (var i  = 0; i != tests.length; ++i)
  do_one(tests[i]);

Comment 2

[Igor Bukanov]

Attachment: Implementation v1

The patch refactors HasProperty/PutProperty to follow the same idea that was implemented for GetProperty and dispatch on the id type, not xml class. I altered the already refactored GetProperty to follow the same pattern that is present in HasNamedProperty as this is more convenient to use and saves code.

Comment 3

[Igor Bukanov]

Attachment: Implementation v1 (diff -w)

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 255489 [details] [diff] [review]
Implementation v1

>+        return found;
>+    } else if (xml->xml_class == JSXML_CLASS_ELEMENT) {

Please fix the else after return non-sequitur.

Looks good to me; be great of waldo could vouch too, I'm too tired atm.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 255489 [details] [diff] [review]
Implementation v1

r=me with nit fixed and jwalden r+.

/be

Comment 6

[Jeff Walden [:Waldo]]

I have some work I've been punting so far this weekend that I need to do first, so I think I can fit this in Monday night.

Comment 7

[Igor Bukanov]

Attachment: Implementation v1.b

This is the previous patch with the nit addressed.

Comment 8

[Igor Bukanov]

Attachment: Implementation v1.b (diff -w)

Comment 9

[Igor Bukanov]

Attachment: Implementation v1.b (diff -w) for real

Comment 10

[Igor Bukanov]

Attachment: Implementation v1.b (diff -w) really for real

Too many patch files :(

Comment 11

[Igor Bukanov]

Attachment: Here is diff -w of Implementation v1.b

I wish bugzilla has an option to remove all those "real" attachments.

Comment 12

[Igor Bukanov]

Ping: Jeff, can you review the patch? I need it for a few other bugs. 

Comment 13

[Jeff Walden [:Waldo]]

Sorry, I got/am a little bogged down -- I'll make sure to get to it later tonight.

Comment 14

[Jeff Walden [:Waldo]]

Still bogged down -- but I'm reviewing now, so this should be done hopefully fairly quickly.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 255540 [details] [diff] [review]
Implementation v1.b (diff -w) really for real

>+HasNamedProperty(JSXML *xml, JSXMLQName* nameqn)

Nit: position of * by nameqn doesn't seem to match house style.

Is there a way for an XMLList to contain another XMLList?  The obvious way of |list = new XMLList(list);| doesn't work (thinking about possible stack overflow from HasNamedProperty recurring), and I've not read the spec closely enough to know if a way exists.

Comment 16

[Igor Bukanov]

(In reply to comment #15)
> Is there a way for an XMLList to contain another XMLList?

No: XMLList is an ordered set of XML trees.

>  The obvious way of
> |list = new XMLList(list);| doesn't work.

That just creates a copy of the original list.

Comment 17

[Igor Bukanov]

Attachment: Implementation v1.c

Patch to commit with the nit from comment 15 addressed.

Comment 18

[Igor Bukanov]

I committed the patch from comment 17 to the trunk:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.142; previous revision: 3.141
done

Comment 19

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-370372.js,v  <--  regress-370372.js

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 20070226 windows/mac*/linux