Bug 35296 – TreeWalker is not calling acceptNode function in filter object

• Component: DOM
• Status: RESOLVED FIXED

[James M. Greene]

Within the last 1 month (4 weeks), the TreeWalker in WebKit (confirmed on Safari for Mac and Google Chrome on Windows) is no longer calling its Filter object's acceptNode function.  There is a related bug on Chromium:  http://code.google.com/p/chromium/issues/detail?id=36502

[Alexey Proskuryakov]

Created attachment 49298 [details]
test case (should alert PASS)

Attaching a test case adapted from one provided in Chromium bug. It fails in both Safari 4.0.4 and ToT for me.

You said that this started about a month ago for you. Do you have a different test case that passes in shipping Safari/WebKit, but fails in nightlies?

[James M. Greene]

I don't know when it STARTED, I just know that it worked 4 weeks ago. I pushed out an unrelated JavaScript fix in my product for WebKit browsers in the section of our code that uses the TreeWalker about 4 weeks ago, at which point it was tested thoroughly in all supported browsers.

So, it must've started within the last 4 weeks.


I'm not using nightly builds, just whatever is the most currently published/pushed versions of Safari and Chrome.

[Alexey Proskuryakov]

There were no Safari releases in the last 4 weeks.

This is a bug, of course, but there is no indication of it being a regression in WebKit (regressions typically get much higher priority, which is why I was asking).

[James M. Greene]

Perhaps my version of Safari was not up-to-date at that time?  I was utilizing a rarely used MacBook to do my Safari testing, so it's quite possible.  I would expect that my copy of Chrome was up-to-date, though, but maybe not.

It was working in the versions of Safari and Chrome that I had installed on Saturday, 1/16/2010 @ 12:00pm CST.

[Darin Adler]

Test case fails in Safari 4.0.4, so not a regression from Safari 4.0.4.

[Darin Adler]

If someone can find an old version of Safari or WebKit nightly build where this test passes, it would be good to know the exact version number.

[Simon Fraser (smfr)]

<rdar://problem/4953561>

[Simon Fraser (smfr)]

WebKit works if the NodeFilter param is a bare function, but fails if it's an object with an acceptNode() property.

[Simon Fraser (smfr)]

Created attachment 65019 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 65019 [details]
Patch

With this patch our behavior differs from Gecko if the filter object does not have an acceptNode function (it should throw an exception).

[Simon Fraser (smfr)]

Created attachment 65033 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 65033 [details]
Patch

What happens if the object is both callable, and has a callable acceptNode member? The priority here is opposite from what we have for handleEvent() in JSEVentListener.cpp.

> With this patch our behavior differs from Gecko if the filter object does not have an acceptNode function (it should throw an exception).

Is there a good reason to be different? Doe Mozilla track this as a bug to be fixed?

[Simon Fraser (smfr)]

(In reply to comment #12)
> (From update of attachment 65033 [details])
> What happens if the object is both callable, and has a callable acceptNode member? The priority here is opposite from what we have for handleEvent() in JSEVentListener.cpp.

The object is called, This is what Gecko does. The testcase tests this.

> > With this patch our behavior differs from Gecko if the filter object does not have an acceptNode function (it should throw an exception).
> 
> Is there a good reason to be different? Doe Mozilla track this as a bug to be fixed?

The final patch fixes this so we match Gecko.

[Alexey Proskuryakov]

Comment on attachment 65033 [details]
Patch

Now I'm wondering if JSEventListener.cpp gets the precedence wrong.

r=me

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/65824

[Dumitru Daniliuc]

Comment on attachment 65033 [details]
Patch

WebCore/bindings/js/JSNodeFilterCondition.cpp:65
 +          JSValue function = m_filter.get(exec, Identifier(exec, "acceptNode"));
does 'function' need to be re-declared here? if it does, then do we need the declaration above? can't we just pass m_filter to JSC::call() in that case?

[Simon Fraser (smfr)]

(In reply to comment #16)
> (From update of attachment 65033 [details])
> WebCore/bindings/js/JSNodeFilterCondition.cpp:65
>  +          JSValue function = m_filter.get(exec, Identifier(exec, "acceptNode"));
> does 'function' need to be re-declared here? if it does, then do we need the declaration above? can't we just pass m_filter to JSC::call() in that case?

You're right; I aliased 'function' by mistake.

[Dumitru Daniliuc]

This patch has also broken the chromium bots, because the V8 bindings were not updated...

[Darin Adler]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 65033 [details] [details])
> > WebCore/bindings/js/JSNodeFilterCondition.cpp:65
> >  +          JSValue function = m_filter.get(exec, Identifier(exec, "acceptNode"));
> > does 'function' need to be re-declared here? if it does, then do we need the declaration above? can't we just pass m_filter to JSC::call() in that case?
> 
> You're right; I aliased 'function' by mistake.

And I think this also shows that there is insufficient test coverage, because the code won't work right.

[Simon Fraser (smfr)]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (From update of attachment 65033 [details] [details] [details])
> > > WebCore/bindings/js/JSNodeFilterCondition.cpp:65
> > >  +          JSValue function = m_filter.get(exec, Identifier(exec, "acceptNode"));
> > > does 'function' need to be re-declared here? if it does, then do we need the declaration above? can't we just pass m_filter to JSC::call() in that case?
> > 
> > You're right; I aliased 'function' by mistake.
> 
> And I think this also shows that there is insufficient test coverage, because the code won't work right.

I don't know how to test this difference. I'd expect it to break the case where the filter has an 'acceptNode' function, but that works fine. I suspect that it continues to work because 'callData' got the right data.

[Simon Fraser (smfr)]

Reopen for cleanup.

[Simon Fraser (smfr)]

Created attachment 65192 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 65192 [details]
Patch

+        Fix incorrect aliasing of the 'function' variable, which could result

Shadowing?

r=me

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/65853

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.