[V8] Incorrect handling of JavaScript properties in DOMStringMap
Created attachment 80893 [details] Patch
Comment on attachment 80893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80893&action=review > Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:54 > + if (map->contains(nameString)) I am complete ignoramus of WebCore::DOMStringMap internals, but is there a contract what DOMStringMap::item should return if there is no binding for the name? If it returns empty string (WebCore::String::isEmpty()), it might be more efficient to look the result up and check for string emptiness. Feel free to ignore if you think it's premature optimization. > Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:88 > INC_STATS("DOM.DOMStringMap.NamedPropertySetter"); Shouldn't NamedPropertyDelete be updated as well? > Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp:91 > + return notHandledByInterceptor(); I suspect (but not 100% sure) it doesn't do the right thing: I would expect that in your test case you would get real JS property 'foobarbaz' on JS wrapper itself, and this update won't be stored in the instance of WebCore::DOMStringMap. Can we extend layout test to check if it's indeed the case? If I am right, I think you should use v8::Object::HasRealNamedProperty instead of both GetRealNamedPropertyInPrototypeChain and HasRealNamedCallback.
(In reply to comment #2) Thank you for the comments. Now I believe the current JSC behavior is also incorrect. I'll try to correct it, then will update the V8 behavior.
Created attachment 110090 [details] Patch 2
Comment on attachment 110090 [details] Patch 2 Attachment 110090 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10012107 New failing tests: fast/dom/dataset.html fast/dom/dataset-xhtml.xhtml
Created attachment 110100 [details] Patch 3
Comment on attachment 110100 [details] Patch 3 Attachment 110100 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9995163 New failing tests: fast/dom/dataset.html fast/dom/dataset-xhtml.xhtml
(In reply to comment #7) > New failing tests: > fast/dom/dataset.html > fast/dom/dataset-xhtml.xhtml Hmm, why they fail? They passed on my Mac.
Created attachment 110481 [details] Patch 4 Fix namedPropertySetter behavior.
Would anyone review this please?
Comment on attachment 110481 [details] Patch 4 Great! I'm glad to see this code go.
Comment on attachment 110481 [details] Patch 4 Thank you for reviewing.
Comment on attachment 110481 [details] Patch 4 Rejecting attachment 110481 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ucceeded at 1 with fuzz 3. patching file LayoutTests/platform/chromium/test_expectations.txt Hunk #1 FAILED at 3762. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/10485236
Created attachment 115102 [details] Patch for landing
Comment on attachment 115102 [details] Patch for landing Clearing flags on attachment: 115102 Committed r100247: <http://trac.webkit.org/changeset/100247>
All reviewed patches have been landed. Closing bug.