[V8] Incorrect handling of JavaScript properties in DOMStringMap

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kent Tamura]

[V8] Incorrect handling of JavaScript properties in DOMStringMap

[Kent Tamura]

Created attachment 80893 [details]
Patch

[anton muhin]

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.

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 110090 [details]
Patch 2

[WebKit Review Bot]

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

[Kent Tamura]

Created attachment 110100 [details]
Patch 3

[WebKit Review Bot]

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

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 110481 [details]
Patch 4

Fix namedPropertySetter behavior.

[Kent Tamura]

Would anyone review this please?

[Adam Barth]

Comment on attachment 110481 [details]
Patch 4

Great!  I'm glad to see this code go.

[Kent Tamura]

Comment on attachment 110481 [details]
Patch 4

Thank you for reviewing.

[WebKit Review Bot]

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

[Kent Tamura]

Created attachment 115102 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 115102 [details]
Patch for landing

Clearing flags on attachment: 115102

Committed r100247: <http://trac.webkit.org/changeset/100247>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.