Summary: | TreeWalker is not calling acceptNode function in filter object | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James M. Greene <james.m.greene> | ||||||||||
Component: | DOM | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, cdumez, darin, dumi, sam, simon.fraser | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
URL: | http://code.google.com/p/chromium/issues/detail?id=36502 | ||||||||||||
Attachments: |
|
Description
James M. Greene
2010-02-23 08:13:03 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?
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. 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). 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. Test case fails in Safari 4.0.4, so not a regression from Safari 4.0.4. 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. WebKit works if the NodeFilter param is a bare function, but fails if it's an object with an acceptNode() property. Created attachment 65019 [details]
Patch
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).
Created attachment 65033 [details]
Patch
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? (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 on attachment 65033 [details]
Patch
Now I'm wondering if JSEventListener.cpp gets the precedence wrong.
r=me
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?
(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. This patch has also broken the chromium bots, because the V8 bindings were not updated... (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. (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. Reopen for cleanup. Created attachment 65192 [details]
Patch
Comment on attachment 65192 [details]
Patch
+ Fix incorrect aliasing of the 'function' variable, which could result
Shadowing?
r=me
Mass moving XML DOM bugs to the "DOM" Component. |