Bug 53578 - [V8] Incorrect handling of JavaScript properties in DOMStringMap
Summary: [V8] Incorrect handling of JavaScript properties in DOMStringMap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Minor
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 53752
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-01 23:11 PST by Kent Tamura
Modified: 2011-11-15 00:37 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.14 KB, patch)
2011-02-01 23:16 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (4.83 KB, patch)
2011-10-06 22:37 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (4.84 KB, patch)
2011-10-07 00:26 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (4.83 KB, patch)
2011-10-10 23:47 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch for landing (4.82 KB, patch)
2011-11-14 23:09 PST, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-02-01 23:11:24 PST
[V8] Incorrect handling of JavaScript properties in DOMStringMap
Comment 1 Kent Tamura 2011-02-01 23:16:22 PST
Created attachment 80893 [details]
Patch
Comment 2 anton muhin 2011-02-02 03:56:46 PST
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.
Comment 3 Kent Tamura 2011-02-03 19:31:07 PST
(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.
Comment 4 Kent Tamura 2011-10-06 22:37:31 PDT
Created attachment 110090 [details]
Patch 2
Comment 5 WebKit Review Bot 2011-10-06 23:02:11 PDT
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
Comment 6 Kent Tamura 2011-10-07 00:26:39 PDT
Created attachment 110100 [details]
Patch 3
Comment 7 WebKit Review Bot 2011-10-07 00:55:07 PDT
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
Comment 8 Kent Tamura 2011-10-07 00:59:48 PDT
(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.
Comment 9 Kent Tamura 2011-10-10 23:47:54 PDT
Created attachment 110481 [details]
Patch 4

Fix namedPropertySetter behavior.
Comment 10 Kent Tamura 2011-11-14 20:22:53 PST
Would anyone review this please?
Comment 11 Adam Barth 2011-11-14 22:55:11 PST
Comment on attachment 110481 [details]
Patch 4

Great!  I'm glad to see this code go.
Comment 12 Kent Tamura 2011-11-14 22:56:45 PST
Comment on attachment 110481 [details]
Patch 4

Thank you for reviewing.
Comment 13 WebKit Review Bot 2011-11-14 22:59:46 PST
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
Comment 14 Kent Tamura 2011-11-14 23:09:23 PST
Created attachment 115102 [details]
Patch for landing
Comment 15 WebKit Review Bot 2011-11-15 00:37:14 PST
Comment on attachment 115102 [details]
Patch for landing

Clearing flags on attachment: 115102

Committed r100247: <http://trac.webkit.org/changeset/100247>
Comment 16 WebKit Review Bot 2011-11-15 00:37:20 PST
All reviewed patches have been landed.  Closing bug.