Bug 35296 - TreeWalker is not calling acceptNode function in filter object
Summary: TreeWalker is not calling acceptNode function in filter object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Simon Fraser (smfr)
URL: http://code.google.com/p/chromium/iss...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-02-23 08:13 PST by James M. Greene
Modified: 2019-02-06 09:03 PST (History)
6 users (show)

See Also:


Attachments
test case (should alert PASS) (229 bytes, text/html)
2010-02-23 09:36 PST, Alexey Proskuryakov
no flags Details
Patch (7.15 KB, patch)
2010-08-20 20:37 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (9.67 KB, patch)
2010-08-21 13:26 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (4.19 KB, patch)
2010-08-23 17:48 PDT, Simon Fraser (smfr)
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James M. Greene 2010-02-23 08:13:03 PST
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
Comment 1 Alexey Proskuryakov 2010-02-23 09:36:18 PST
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?
Comment 2 James M. Greene 2010-02-23 10:04:26 PST
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.
Comment 3 Alexey Proskuryakov 2010-02-23 10:52:31 PST
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).
Comment 4 James M. Greene 2010-02-23 12:05:46 PST
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.
Comment 5 Darin Adler 2010-02-23 13:50:51 PST
Test case fails in Safari 4.0.4, so not a regression from Safari 4.0.4.
Comment 6 Darin Adler 2010-02-23 13:51:31 PST
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.
Comment 7 Simon Fraser (smfr) 2010-08-20 16:42:13 PDT
<rdar://problem/4953561>
Comment 8 Simon Fraser (smfr) 2010-08-20 16:55:19 PDT
WebKit works if the NodeFilter param is a bare function, but fails if it's an object with an acceptNode() property.
Comment 9 Simon Fraser (smfr) 2010-08-20 20:37:05 PDT
Created attachment 65019 [details]
Patch
Comment 10 Simon Fraser (smfr) 2010-08-21 12:42:50 PDT
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).
Comment 11 Simon Fraser (smfr) 2010-08-21 13:26:36 PDT
Created attachment 65033 [details]
Patch
Comment 12 Alexey Proskuryakov 2010-08-23 11:02:14 PDT
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?
Comment 13 Simon Fraser (smfr) 2010-08-23 11:23:51 PDT
(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.
Comment 14 Alexey Proskuryakov 2010-08-23 11:31:17 PDT
Comment on attachment 65033 [details]
Patch

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

r=me
Comment 15 Simon Fraser (smfr) 2010-08-23 12:24:48 PDT
http://trac.webkit.org/changeset/65824
Comment 16 Dumitru Daniliuc 2010-08-23 15:56:05 PDT
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?
Comment 17 Simon Fraser (smfr) 2010-08-23 16:29:49 PDT
(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.
Comment 18 Dumitru Daniliuc 2010-08-23 16:46:11 PDT
This patch has also broken the chromium bots, because the V8 bindings were not updated...
Comment 19 Darin Adler 2010-08-23 16:54:01 PDT
(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.
Comment 20 Simon Fraser (smfr) 2010-08-23 17:34:28 PDT
(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.
Comment 21 Simon Fraser (smfr) 2010-08-23 17:38:52 PDT
Reopen for cleanup.
Comment 22 Simon Fraser (smfr) 2010-08-23 17:48:23 PDT
Created attachment 65192 [details]
Patch
Comment 23 Alexey Proskuryakov 2010-08-23 18:31:31 PDT
Comment on attachment 65192 [details]
Patch

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

Shadowing?

r=me
Comment 24 Simon Fraser (smfr) 2010-08-23 18:49:39 PDT
http://trac.webkit.org/changeset/65853
Comment 25 Lucas Forschler 2019-02-06 09:03:15 PST
Mass moving XML DOM bugs to the "DOM" Component.