Bug 19933 – nodeIterator with filter fails on documents not in a frame

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Simon Fraser (smfr)]

I'm trying to use node iterators on an XML document fetched via XMLHttpRequest, like this:

function subElementWithNodeName(inElement, inName)
{
  var upperName = inName.toUpperCase();
  var iter = document.createNodeIterator(inElement, NodeFilter.SHOW_ELEMENT, null);
  var curNode;
  while (curNode = iter.nextNode())
  {
    if (curNode.nodeName.toUpperCase() == upperName)
      return curNode;
  }
  return null;
}

However, iter.nextNode() always returns null right away, without giving back child elements of 'inElement' (which do exist).

Some debugging reveals this:

short JSNodeFilterCondition::acceptNode(Node* filterNode, JSValue*& exception) const
{
    // FIXME: It makes no sense for this to depend on the document being in a frame!
    Frame* frame = filterNode->document()->frame();
    if (!frame)
        return NodeFilter::FILTER_REJECT;

....

which is where it fails.

[Simon Fraser (smfr)]

Created attachment 22139 [details]
Patch to pass ExecState down to node filters

Here's a first cut at a patch, with testcases. Some things that I'm not clear about:
* Where should we call exec->clearException()?
* Which methods need to be null-exec safe? We should assert in the others.

[Simon Fraser (smfr)]

<rdar://problem/6054693>

[Darin Adler]

Comment on attachment 22139 [details]
Patch to pass ExecState down to node filters

Great!

I would have made the ExecState* the first argument because that's the usual convention in code that involves ExecState, but I think it's OK to have it as the last one.

+    if (!exec)
+        return NodeFilter::FILTER_REJECT;

I think this merits a comment. Specifically that this case will only arise when calling from a non-JavaScript language to a JavaScript filter, when the document in question is not associated with a frame.

     if (exec->hadException()) {
-        exception = takeException(exec);
         return NodeFilter::FILTER_REJECT;
     }

One-line if statement bodies don't get braces in the WebKit coding style. Please remove the braces in these cases.

+        virtual short acceptNode(Node*, KJS::ExecState* exec) const;

We don't give the argument name in function declarations when the type makes the argument's purpose clear. So the name "exec" should be omitted here and in the various other similar places.

+    short result = impl()->acceptNode(toNode(args[0]), exec);
     return jsNumber(exec, result);

Why not merge this into a single line?

+    if (exec->hadException()) {
+        exec->clearException();
         return jsUndefined();
     }

The clearException here is incorrect. We do want to pass the exception on to the caller. This mistake should have made tests fail. I don't know why it didn't. I guess we don't have sufficient test coverage for our existing exception behavior!

+short ObjCNodeFilterCondition::acceptNode(Node* node, ExecState* exec) const

Although we haven't turned the warning on, we do omit the names of unused arguments. Please do that here.

+    if (node && node->document()) {
+        Frame* frame = node->document()->frame();
+        if (frame)
+            return frame->script()->globalObject()->globalExec();
+    }
+    return 0;

We normally prefer the early return style. I'd write it like this:

    if (!node)
        return 0;
    Document* document = node->document();
    if (!document)
        return 0;
    Frame* frame = document->frame();
    if (!frame)
        return 0;
    return frame->script()->globalObject()->globalExec();

Also, I'm not sure we're guaranteed globalObject() can't be 0. What happens if JavaScript is disabled?

I think you need to do the execStateFromNode thing for NodeIterator as well as TreeWalker. You should move that function to Traversal and do it for both. Also I think it can be a static member function. But also, since you're using it for NodeFilter too, maybe you can find a place to put it where it can be one shared copy.

+        exec->clearException();  // rignt thing to do?

This is not the right thing to do, although it will do no harm. You can just leave this line of code out (and fix the typo that way).

+    if (exec)
+        exec->clearException();

Similarly, these are not needed, but will do no harm.

Please check Acid3 to make sure we still have 100/100.

review- for the incorrect clearException calls (the ones that will do harm) and the lack of execStateFromNode in NodeIterator.

[Simon Fraser (smfr)]

Created attachment 22145 [details]
Revised patch addressing comments

New patch addressing review comments

[Simon Fraser (smfr)]

Note that previous patch did break exception tests; with this patch test succeed, and Acid3 passes.

[Darin Adler]

Comment on attachment 22145 [details]
Revised patch addressing comments

     /* exec should only be null when calling from a non-JavaScript language to
        a JavaScript filter, when the document in question is not associated
        with a frame
     */

We normally use "//" comments, not "/*". I also would prefer that the comment be a sentence with a capital letter and period. Here's my suggested wording:

    // The exec argument here should only be null if this was called from a
    // non-JavaScript language, and this is a JavaScript filter, and the document
    // in question is not associated with the frame. In that case, we're going to
    // behave incorrectly, and just reject nodes instead of calling the filter function.
    // To fix that we'd need to come up with a way to find a suitable JavaScript
    // execution context for the filter function to run in.

I know that's pretty wordy, sorry.

    /* static */
    ExecState* NodeFilter::execStateFromNode(Node* node)

We don't normally add a comment like this before a static member function.

r=me

[Simon Fraser (smfr)]

Created attachment 22146 [details]
Final patch

Address final comments. Transferring r=darin

[Dean Jackson]

committed by me - r35054

[Dean Jackson]

Missed a test file. Now in r35064