Closed Bug 354998 Opened 15 years ago Closed 15 years ago

Ensure that prototype is not enumerated for XML objects

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

Details

Attachments

(1 obsolete file)

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.
Severity: normal → enhancement
Fixed as part of the cleanup patch in bug 354982.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Igor, can you help me out with a test case for this? Thanks.
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.
Attached file e4x/Regress/regress-354998.js (obsolete) —
I can't get this to fail. All branches even with September builds pass.
(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";
}

Attachment #241818 - Attachment is obsolete: true
Checking in regress-354998.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-354998.js,v  <--  regress-354998.js
initial revision: 1.1
done
Flags: in-testsuite+
verified fixed 1.9 20061012 windows/linux
Status: RESOLVED → VERIFIED
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.
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

/cvsroot/mozilla/js/tests/e4x/Regress/regress-354998.js,v  <--  regress-354998.js
new revision: 1.4; previous revision: 1.3
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
(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.
You need to log in before you can comment on or make changes to this bug.