354998 - Ensure that prototype is not enumerated for XML objects

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Although E4X specification not very clear if for-in loop over an XML object should enumerate the prototype chain, still I assume that the sections 12.2 and 12.3 of ECMA-357 do mean that only the object itself is enumerated as the only reasonable possibility. In particular, 12.2 contains at the end the following 2 paragraphs:

> The order of enumeration is defined by the object (steps 6 and 6a in the first algorithm and steps 7 and 7a in the second algorithm). When e evaluates to a value of type XML or XMLList, properties are enumerated in ascending sequential order according to their numeric property names (i.e., document order for XML objects). Properties of other objects are enumerated in an implementation dependent order.

> The mechanics of enumerating the properties (steps 6 and 6a in the first algorithm and steps 7 and 7a in the second) is implementation dependent. Properties of the object being enumerated may be deleted during enumeration. If a property that has not yet been visited during enumeration is deleted, then it will not be visited. If new properties are added to the object being enumerated during enumeration, the newly added properties are not guaranteed to be visited in the active enumeration. Enumerating the properties of an object includes enumerating properties of its prototype and the prototype of the prototype, and so on, recursively; but a property of a prototype is not enumerated if it is "shadowed" because some previous object in the prototype chain has a property with the same name.

I read these 2 paragraphs as that the second paragraph is only applies to non-xml objects. Which is good since this is what SpiderMonkey implements. The only problem is that this is happens since the enumerator when checking the properties in the prototype calls (see jsinter.c, line 2819) OBJ_LOOKUP_PROPERTY(cx, origobj, fid, &obj2, &prop). Since for XML objects this does not look for prototype chain, the property would never be found.

Thus it would be nice if enumerating of XML objects would never bother with prototypes avoiding OBJ_LOOKUP_PROPERTY overhead and the overhead of enumerating XML.prototype.methods. It would be even better if XML objects would provide own __iterator__ implementation removing the need to have special XML support in jsiter.c.

Comment 1

[Igor Bukanov]

Fixed as part of the cleanup patch in bug 354982.

Comment 2

[Bob Clary [:bc] (inactive)]

Igor, can you help me out with a test case for this? Thanks.

Comment 3

[Igor Bukanov]

Here is a test case:

var list = <><a/><b/></>;
Object.prototype.p1 = 1;
XMLList.prototype.p2 = 2;

var count = 0;
for (var i in list)
  ++count;

if (count != 2) {
  if (count < 2)
    throw "Enumerator has not looped over all properties, count="+count;
  throw "Enumerator has looped over prototype chain of xml, count="+count;
}


It works even with 1.8.0 currently, but there a useless lookup of Object.prototype properties is carried out.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-354998.js

I can't get this to fail. All branches even with September builds pass.

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> Created an attachment (id=241818) [edit]
> e4x/Regress/regress-354998.js
> 
> I can't get this to fail. All branches even with September builds pass.

But that is the reason why severity is an enhancement here. The current code works but it uses an extra expensive lookup. And that one can be observed via time measurements:

function test()
{
  var list = <><a/><b/></>;
  var count = 0;
  var now = Date.now;
  var time = now();
  for (var i in list) {
    ++count;
  }
  time = now() - time; 
  if (count != 2) {
    if (count < 2)
      throw "Enumerator has not looped over all properties, count="+count;
    throw "Enumerator has looped over prototype chain of xml, count="+count;
  }
  return time;
}

var time1 = test(); 

for (var i = 0; i != 1000*1000; ++i)
  Object.prototype[i] = i;

var time2 = test(); 
if (time1 * 10 < time2) {
  throw "Assigns to Object.prototype increased time of XML enumeration from "+
        time1+"ms to "+time2+"ms";
}

Comment 6

[Bob Clary [:bc] (inactive)]

Checking in regress-354998.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-354998.js,v  <--  regress-354998.js
initial revision: 1.1
done

Comment 7

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061012 windows/linux

Comment 8

[Bob Clary [:bc] (inactive)]

fyi, macppc trunk 20070226 failed this with uncaught exception: Assigns to Object.prototype increased time of XML enumeration from 0ms to 1ms. I don't care if you don't.

Comment 9

[Bob Clary [:bc] (inactive)]

delete the Object.prototype[i] properties to prevent side effects when enumerating properties in the framework.

/cvsroot/mozilla/js/tests/e4x/Regress/regress-354998.js,v  <--  regress-354998.js
new revision: 1.2; previous revision: 1.1

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-354998.js,v  <--  regress-354998.js
new revision: 1.4; previous revision: 1.3

Comment 11

[Bob Clary [:bc] (inactive)]

modify test to handle uncaught exception
http://hg.mozilla.org/mozilla-central/rev/64aeec8f4b1c

Comment 12

[Brendan Eich [:brendan]]

This test is now too slow, due to recent changes landed on m-c, probably at least these ones:

http://hg.mozilla.org/mozilla-central/rev/bb4f39064bf0 (for bug 473228)
http://hg.mozilla.org/mozilla-central/rev/0296ef3eef1a (followup bug 532096)

I changed jstests.list on mozilla-central to skip the test in debug browser builds. It's still run in shell builds -- let me know if that's a problem.

The test could be improved to allow it to be re-enabled, maybe. Igor, thoughts?

/be

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> This test is now too slow, due to recent changes landed on m-c, probably at
> least these ones:
> 
> http://hg.mozilla.org/mozilla-central/rev/bb4f39064bf0 (for bug 473228)
> http://hg.mozilla.org/mozilla-central/rev/0296ef3eef1a (followup bug 532096)

It could be due to excessive debug checks that alter the complexity of some operations for the following loop from the test case:

for (var i = 0; i != 1000*1000; ++i)
  Object.prototype[i] = i;

I will check this.