Bug 86991

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ec2-cq-02	none
Archive of layout-test-results from ec2-cr-linux-02	none
Patch	arv: review-, arv: commit-queue-
patch for landing	none
patch for landing	none
patch for landing	webkit.review.bot: review-, haraken: commit-queue+
patch for landing	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cq-03	none
patch for landing	none

Kentaro Hara

Reported 2012-05-20 23:34:28 PDT

Original Chromium bug: http://code.google.com/p/chromium/issues/detail?id=128723 Consider the following html: <html> <body> <form> <input type="hidden" name="arguments"></input> <div onclick="onclicked()" id="divInsideForm">Click here</div> </form> </body> <script> function onclicked() { alert("onclicked"); } </script> </html> If we click "Click here", JavaScript throws "Uncaught TypeError: undefined has no properties".

Attachments
Patch (4.93 KB, patch) 2012-05-20 23:40 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cq-02 (545.91 KB, application/zip) 2012-05-21 01:01 PDT, WebKit Review Bot	no flags	Details
Archive of layout-test-results from ec2-cr-linux-02 (500.10 KB, application/zip) 2012-05-21 01:09 PDT, WebKit Review Bot	no flags	Details
Patch (5.26 KB, patch) 2012-05-21 17:10 PDT, Kentaro Hara	arv: review- arv: commit-queue-	Details Formatted Diff Diff
patch for landing (5.61 KB, patch) 2012-05-21 17:45 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
patch for landing (6.23 KB, patch) 2012-05-21 18:03 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
patch for landing (6.08 KB, patch) 2012-05-21 18:48 PDT, Kentaro Hara	webkit.review.bot: review- haraken: commit-queue+	Details Formatted Diff Diff
patch for landing (6.08 KB, patch) 2012-05-21 19:04 PDT, Kentaro Hara	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cq-03 (949.87 KB, application/zip) 2012-05-21 23:36 PDT, WebKit Review Bot	no flags	Details
patch for landing (6.65 KB, patch) 2012-05-22 00:06 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Kentaro Hara

Comment 1 2012-05-20 23:40:34 PDT

Created attachment 142950 [details] Patch

Kentaro Hara

Comment 2 2012-05-21 00:01:00 PDT

This is marked as P1 & Release-block in the Chromium bug. I am happy if anyone could review the patch quickly:)

Adam Barth

Comment 3 2012-05-21 00:03:44 PDT

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.

Kentaro Hara

Comment 4 2012-05-21 00:04:39 PDT

Comment on attachment 142950 [details] Patch thanks!

WebKit Review Bot

Comment 5 2012-05-21 01:01:33 PDT

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

WebKit Review Bot

Comment 6 2012-05-21 01:01:38 PDT

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

WebKit Review Bot

Comment 7 2012-05-21 01:09:51 PDT

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

WebKit Review Bot

Comment 8 2012-05-21 01:09:56 PDT

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

Erik Arvidsson

Comment 9 2012-05-21 09:26:46 PDT

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.

Adam Barth

Comment 10 2012-05-21 09:29:58 PDT

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

Erik Arvidsson

Comment 11 2012-05-21 09:36:43 PDT

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.

Erik Arvidsson

Comment 12 2012-05-21 09:37:54 PDT

(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 :'(

Adam Barth

Comment 13 2012-05-21 09:44:42 PDT

> 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?

Erik Arvidsson

Comment 14 2012-05-21 10:33:38 PDT

(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

Erik Arvidsson

Comment 15 2012-05-21 10:46:22 PDT

http://code.google.com/p/v8/issues/detail?id=2144

Daniel Clifford

Comment 16 2012-05-21 14:02:58 PDT

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.

Erik Arvidsson

Comment 17 2012-05-21 14:16:04 PDT

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

Erik Arvidsson

Comment 18 2012-05-21 14:55:15 PDT

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

Kentaro Hara

Comment 19 2012-05-21 17:10:29 PDT

Created attachment 143142 [details] Patch

Ojan Vafai

Comment 20 2012-05-21 17:12:31 PDT

Comment on attachment 143142 [details] Patch Glad we could find a good resolution to this.

Kentaro Hara

Comment 21 2012-05-21 17:13:50 PDT

Comment on attachment 143142 [details] Patch Thanks arv and vegorov! (The code looks so hacky though...)

Erik Arvidsson

Comment 22 2012-05-21 17:32:21 PDT

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](); });

Kentaro Hara

Comment 23 2012-05-21 17:45:19 PDT

Created attachment 143151 [details] patch for landing

Kentaro Hara

Comment 24 2012-05-21 17:45:48 PDT

arv: Does the patch look good?

Kentaro Hara

Comment 25 2012-05-21 18:03:17 PDT

Created attachment 143156 [details] patch for landing

Erik Arvidsson

Comment 26 2012-05-21 18:39:25 PDT

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.

Kentaro Hara

Comment 27 2012-05-21 18:48:58 PDT

Created attachment 143167 [details] patch for landing

Kentaro Hara

Comment 28 2012-05-21 18:49:28 PDT

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

Erik Arvidsson

Comment 29 2012-05-21 18:51:43 PDT

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

WebKit Review Bot

Comment 30 2012-05-21 19:03:01 PDT

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.

Kentaro Hara

Comment 31 2012-05-21 19:04:14 PDT

Created attachment 143169 [details] patch for landing

WebKit Review Bot

Comment 32 2012-05-21 23:35:58 PDT

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

WebKit Review Bot

Comment 33 2012-05-21 23:36:05 PDT

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

Kentaro Hara

Comment 34 2012-05-22 00:06:50 PDT

Created attachment 143204 [details] patch for landing

WebKit Review Bot

Comment 35 2012-05-22 00:50:09 PDT

Comment on attachment 143204 [details] patch for landing Clearing flags on attachment: 143204 Committed r117928: <http://trac.webkit.org/changeset/117928>

Note You need to log in before you can comment on or make changes to this bug.