Implement NodeIterator

[Joris-van-der-Wel]

Using https://dom.spec.whatwg.org/#interface-nodeiterator
Related to #317

I wrote my own test cases, I used istanbul to verify coverage (line and branch coverage is 100%).

The most tricky part about NodeIterator is its behavior when you remove nodes it iterates over. Each active NodeIterator must be notified about every node that is removed ("removing steps").

The DOM specification assumes implementations have control over object lifecycles (destructors, weak references, etc). To implement it properly, without leaks, you would need to use weak references between the Document and NodeIterators. We can not use such things in javascript (WeakMap and WeakSet are not adequate in this case) unless we would add a dependency on a compiled module (again :P). It looks like Ecma added this limitation on purpose to prevent side channel attacks.

Instead, I added an option that specifies the maximum number of NodeIterators that removing steps will occur for. If this limit is reached, the oldest NodeIterator will no longer be tracked and it will start to throw whenever you try to access its state.

I tried to work out an alternative by asynchronously resolving the state of NodeIterators. jsdom would store a lot of data about removals & insertions applied on nodes and the NodeIterator would use that data the next time you access it. This is pretty complex, but more importantly it would be performance heavy because you would have to store a lot of state. Also the combination of iterating + modification would probably become something like O(n^2) instead of O(n).

[domenic]

This is really amazing work. Left some nitpicky comments, but major <3 for implementing this. Very clean overall. And I think the max-node-iterators thingy is a pretty reasonable solution.

I asked in IRC if we should be trying to pass https://github.com/w3c/web-platform-tests/tree/13bff083fba249ed260966bca65319b1b35d3f34/dom/traversal/unfinished despite its "unfinished" name. Otherwise we should really consider submitting these test to web-platform-tests at some point.

[Joris-van-der-Wel]

+- config.concurrentNodeIterators: the maximum amount of NodeIterators that you can use at the same time. The default is 10, setting this to a high value will hurt performance.

Will it hurt performance or memory usage?

Strictly both. If concurrentNodeIterators was set to Infinity, each call to createNodeIterator would leak a little memory (until you stop using the entire Document). The performance is probably a bigger issue, since each removeChild() call has to go through all the NodeIterators.

[Joris-van-der-Wel]

Why a single symbol instead of one per property?

While Symbol performance is good, it is still a little bit slower than regular properties.
I also think the code would look ugly with so many symbols, and annoying to type.

Why use a well-known symbol instead of just Symbol()?

For debug-ability; It would be really annoying to have to use obj[Object.getOwnPropertySymbols(obj)[0]]. E.g. if I would want to log something internal to jsdom from within my own application.

[domenic]

Hmm. I would rather not make it that easy to get at the internals. In browsers you can't get at them at all; ideally we'd use weak maps and hide them entirely, but they're too slow. So let's just use Symbol().

Are you sure amortized one-time symbol + repeated prop lookup is slower than repeated symbol lookup?

[Joris-van-der-Wel]

http://jsperf.com/symbol-vs-prop-vs-def-prop/13
(on my machine Symbol is 13% slower in chrome 42)

I did not test the "+ repeated prop lookup" part specifically though. Do you think it would be worth it to create a benchmark?

[domenic]

I guess I don't mind too much. One day we'll figure out our overall story for private state and unify everything but in the meantime this is fine.

[Joris-van-der-Wel]

Alright, i am done with the fixes.

I played around some more with Symbol. NodeIterator no longer exposes any jsdom specific properties. I added a private class called NodeIteratorInternal which contains all the interesting stuff (properties and methods). NodeIterator is now just a small class forwarding stuff to NodeIteratorInternal.

[domenic]

Merged; thanks so much!