| Summary: | REGRESSION r110315: [V8] Event handler throws TypeError for an input element with name="arguments" | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||||||||||||||||||
| Component: | WebCore JavaScript | Assignee: | Kentaro Hara <haraken> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, arv, danno, dglazkov, japhet, ojan, webkit.review.bot | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Kentaro Hara
2012-05-20 23:34:28 PDT
Created attachment 142950 [details]
Patch
This is marked as P1 & Release-block in the Chromium bug. I am happy if anyone could review the patch quickly:) Comment on attachment 142950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142950&action=review > Source/WebCore/bindings/v8/V8LazyEventListener.cpp:143 > String code = "(function() {" \ > - "with (arguments[2]) {" \ > - "with (arguments[1]) {" \ > - "with (arguments[0]) {"; > + "with (this.ownerDocument ? this.ownerDocument : {}) {" \ > + "with (this.form ? this.form : {}) {" \ > + "with (this) {"; This code is so ridiculous. Comment on attachment 142950 [details]
Patch
thanks!
Comment on attachment 142950 [details] Patch Rejecting attachment 142950 [details] from commit-queue. New failing tests: fast/overflow/onscroll-layer-self-destruct.html fast/dom/inline-event-attributes-lookup-removed.html fast/dom/inline-event-attributes-lookup-removed-form.html fast/forms/form-action.html fast/forms/selected-index-value.html fast/forms/lazy-event-listener-scope-chain.html fast/dom/inline-event-attributes-lookup.html Full output: http://queues.webkit.org/results/12740348 Created attachment 142960 [details]
Archive of layout-test-results from ec2-cq-02
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 142950 [details] Patch Attachment 142950 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12739347 New failing tests: fast/overflow/onscroll-layer-self-destruct.html fast/dom/inline-event-attributes-lookup-removed.html fast/dom/inline-event-attributes-lookup-removed-form.html fast/forms/form-action.html fast/forms/selected-index-value.html fast/forms/lazy-event-listener-scope-chain.html fast/dom/inline-event-attributes-lookup.html Created attachment 142961 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 142950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142950&action=review This is wrong. See previous bugs that this fixed. >> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:143 >> + "with (this.ownerDocument ? this.ownerDocument : {}) {" \ >> + "with (this.form ? this.form : {}) {" \ >> + "with (this) {"; > > This code is so ridiculous. This is wrong. We must not use the JS properties here since they might be overridden. > This is wrong.
They're both wrong. At this point it seems to be a question of the lesser of two evils.
The right fix in the long term is probably to set up the scope chain using V8 APIs rather than concatenating source code. That's how JSC solves this problem.
As far as I know (and I spent a lot of time on this one) the only way to fix this is to change V8 to expose ObjectEnvironmentRecords. The problem is that we need to have an object environment that includes the original DOM objects but V8 has no API that exposes object environments so we have to revert to the with hack and to do the with hack we need to introduce one name. Whatever name we chose it will leak unless we use arguments which does not introduce any new name, but as you've found out it cause problems because the second with now uses document.arguments. Also, the patch Adam r+ is incomplete. The code that changed to using arguments did more things. It modifies the arguments etc. If we decide that we rather have the old bugs than the new bug we should at least remove that code. (In reply to comment #10) > The right fix in the long term is probably to set up the scope chain using V8 APIs rather than concatenating source code. That's how JSC solves this problem. When I talked to the V8 team they were not willing to add this :'( > When I talked to the V8 team they were not willing to add this :'(
Why not? Would they like to talk with the angry web site owner who is losing 6% of his revenue due to this bug?
(In reply to comment #13) > > When I talked to the V8 team they were not willing to add this :'( > > Why not? Would they like to talk with the angry web site owner who is losing 6% of his revenue due to this bug? I'll bring it up again Speaking for the V8 team, I think it's a wee bit of an exaggeration to say that we're not willing to add this. However, adding the API support in V8 isn't going to happen overnight, so it seems like a workaround, if possible, in the meantime is probably prudent. (In reply to comment #16) > Speaking for the V8 team, I think it's a wee bit of an exaggeration to say that we're not willing to add this. I apologize for the out of tone comment. Danno, thanks for taking a look at this. Vyacheslav Egorov came up with a solution that should work. Instead of relying on arguments we can rely on 'this' since 'this' cannot be shadowed in a with statement.
The generated code would be:
String code = "(function() {" \
"with (this[2]) {" \
"with (this[1]) {" \
"with (this[0]) {";
and then we invoke that code with [this, form, document] as the [[This]] pointer
Created attachment 143142 [details]
Patch
Comment on attachment 143142 [details]
Patch
Glad we could find a good resolution to this.
Comment on attachment 143142 [details]
Patch
Thanks arv and vegorov! (The code looks so hacky though...)
Comment on attachment 143142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143142&action=review > Source/WebCore/bindings/v8/V8LazyEventListener.cpp:150 > + code.append("\n};}}}}).call(arguments);})"); This should not use call here since someone might have replaced Function.prototype.call. See Vyacheslav Egorov latest code: // call with 4 arguments instead of 3, pass additional null as last parameter (function () { // by calling this function with 4 arguments we created a setter on arguments object // which would shadow property "3" on the prototype. arguments[3] = function () { with (this[2]) { with (this[1]) { with (this[0]) { return function (<evt_name>) { <listener body> }; } } } }; return arguments[3](); }); Created attachment 143151 [details]
patch for landing
arv: Does the patch look good? Created attachment 143156 [details]
patch for landing
Comment on attachment 143156 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=143156&action=review This is fine by me but there are a bunch of nits. > Source/WebCore/ChangeLog:27 > + 'arguments' to retrieve the execution contexts, since 'arguments' can be > + being overwritten by JavaScript. typo, be being > Source/WebCore/ChangeLog:30 > + This patch changes V8LazyEventListener so that it retrieves contexts > + by this.ownerDocument, this.form, and this. not accurate any more > Source/WebCore/bindings/v8/V8LazyEventListener.cpp:153 > + code.append("\n};}}}}; return arguments[3]();})"); Can you line break before return? > LayoutTests/fast/forms/form-input-named-arguments.html:13 > + layoutTestController.waitUntilDone(); No need for async test here. <script> var clicked = false; function onclicked() { clicked = true; } var event = document.createEvent("MouseEvents"); event.initMouseEvent("click", true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null); var div = document.getElementById("divInsideForm"); div.dispatchEvent(event); shouldBeTrue('clicked'); </script> That's it. No need to reference layoutTestController. Created attachment 143167 [details]
patch for landing
(In reply to comment #26) > (From update of attachment 143156 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143156&action=review > > This is fine by me but there are a bunch of nits. Thanks. Fixed. Comment on attachment 143167 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=143167&action=review Looks good to me > Source/WebCore/bindings/v8/V8LazyEventListener.cpp:154 > + code.append("\n};}}}};"); > + code.append("return arguments[3]();})"); I would have done code.append("\n};}}}};" \ "return arguments[3]();})"); but it doesn't matter Comment on attachment 143167 [details] patch for landing Rejecting attachment 143167 [details] from review queue. arv@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights. Created attachment 143169 [details]
patch for landing
Comment on attachment 143169 [details] patch for landing Rejecting attachment 143169 [details] from commit-queue. New failing tests: inspector/debugger/debugger-scripts.html Full output: http://queues.webkit.org/results/12732947 Created attachment 143201 [details]
Archive of layout-test-results from ec2-cq-03
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 143204 [details]
patch for landing
Comment on attachment 143204 [details] patch for landing Clearing flags on attachment: 143204 Committed r117928: <http://trac.webkit.org/changeset/117928> |