776536 - Move NodeIterator to Paris bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WIP

I've got a patch, but it fails <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/tests/mochitest/test_bug502959.html?force=1>. IIRC, doublewrapped.acceptNode was undefined.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> IIRC, doublewrapped.acceptNode was undefined.

That's... quite odd.  That should be taking the normal xpconnect codepaths....

Comment 2

[Boris Zbarsky [:bzbarsky]]

I need this and TreeWalker to fix bug 838686.  Stealing.

Comment 3

[Boris Zbarsky [:bzbarsky]]

On the other hand, there's a good chance that without bug 838686 fixing this bug will just make things fail too, because the WebIDL call won't prevent XPConnect from reporting the exception.

I need to think about patch ordering here a bit.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Rename nsNodeIterator to mozilla::dom::NodeIterator.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Rename nsTreeWalker to mozilla::dom::TreeWalker.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add a WebIDL API to NodeIterator and TreeWalker.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Turn on WebIDL bindings for NodeIterator and TreeWalker. behavior here is a bit weird because Document is still not a

WebIDL object, so calling createNodeIterator or createTreeWalker via
an Xray will call the XPCOM versions of those methods.  That means
that I can't just disable XPCOM-based wrapping for TreeWalker and
NodeIterator altogether, unfortunately, which means a web page could
try stashing a TreeWalker in something like userdata and then getting
it back and end up wrapping it as an XPCOM object the second time.

I could "fix" that by adding a wrapper cache and whatnot, I guess, if
desired...  But the problem will go away once we convert Document in
any case.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=84f47dbdf6ce

The M2 bits are all unexpected pass.  I'll adjust the failure annotations to deal with that.

The M4 bit I'm looking into.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So the failures in js/xpconnect/tests/mochitest/test_bug502959.html are because it expects this:

26 var iter = document.createNodeIterator(document, NodeFilter.SHOW_ELEMENT, foo);
27 var doublewrapped = iter.filter;

to make doublewrapped an XPCWrappedNative around an XPCWrappedJS. But per WebIDL, doublewrapped === foo in this case.

So we should probably adjust the test... or something.  Blake, any idea how?

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 712209 [details] [diff] [review]
part 3.  Add a WebIDL API to NodeIterator and TreeWalker.

Review of attachment 712209 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/NodeIterator.cpp
@@ +183,5 @@
>  
>  /* readonly attribute nsIDOMNode root; */
>  NS_IMETHODIMP NodeIterator::GetRoot(nsIDOMNode * *aRoot)
>  {
> +    return CallQueryInterface(Root(), aRoot);

Maybe

NS_ADDREF(*aRoot = Root().AsDOMNode());

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 712210 [details] [diff] [review]
part 4.  Turn on WebIDL bindings for NodeIterator and TreeWalker.   behavior here is a bit weird because Document is still not a

Review of attachment 712210 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +5378,5 @@
>                                               rv).get();
>    return rv.ErrorCode();
>  }
>  
> +already_AddRefed<mozilla::dom::NodeIterator>

Nit: I don't think you need the namespace qualifications in the .cpp.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> NS_ADDREF(*aRoot = Root().AsDOMNode());

Done, with Root()->AsDOMNode(); and a "return NS_OK;"

> Nit: I don't think you need the namespace qualifications in the .cpp.

Indeed, done.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> So we should probably adjust the test... or something.  Blake, any idea how?

Reading the test, I wonder if we can start ripping some of the code out as well as the tests. As I understand it, WebIDL mandates that in cases where we currently hand out double wrapped objects we should instead hand out the original object. Does that mean that we won't have to worry about double wrapped objects in content once we switch entirely over to the new bindings?

Comment 14

[Boris Zbarsky [:bzbarsky]]

For WebIDL objects, there is no double-wrapping.  So if we completely stop exposing all JS objects not exposed via WebIDL to web pages, then you're probably right...

But that's a pretty long-term thing.  In the short term, what do I do with this test?  ;)

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

I'd say nuke it unless you can think of a non-converted callback. They're getting harder and harder to find (and therefore less and less important).

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I'd say nuke it unless you can think of a non-converted callback.

I nuked it as part of part 4.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Pushed:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/405218398a2d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a61336552762
   https://hg.mozilla.org/integration/mozilla-inbound/rev/0092982b496d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/9573386c7adf

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, I had to backout because something in this push caused a spike in mochitest-a11y assertions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb572a342efa

https://tbpl.mozilla.org/php/getParsedLog.php?id=20189591&tree=Mozilla-Inbound

A quick skim of the log shows them to be of the type:
###!!! ASSERTION: bad size recorded: 'aInstanceSize == 0 || entry->GetClassSize() == aInstanceSize', file ../../../xpcom/base/nsTraceRefcntImpl.cpp, line 441

Comment 19

[Boris Zbarsky [:bzbarsky]]

Apparently refcount logging requires that all class names passed to it be unique.  Since there's a "TreeWalker" in a11y code already, have to pass dom::TreeWalker to the addref/release macros.  Did that, and:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf95d0e6941
   https://hg.mozilla.org/integration/mozilla-inbound/rev/5e0a9285a7aa
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff820824d40
   https://hg.mozilla.org/integration/mozilla-inbound/rev/54d365f29ee5

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbf95d0e6941
https://hg.mozilla.org/mozilla-central/rev/5e0a9285a7aa
https://hg.mozilla.org/mozilla-central/rev/2ff820824d40
https://hg.mozilla.org/mozilla-central/rev/54d365f29ee5