Bug 86991 - REGRESSION r110315: [V8] Event handler throws TypeError for an input element with name="arguments"
Summary: REGRESSION r110315: [V8] Event handler throws TypeError for an input element ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-20 23:34 PDT by Kentaro Hara
Modified: 2012-05-22 00:52 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 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".
Comment 1 Kentaro Hara 2012-05-20 23:40:34 PDT
Created attachment 142950 [details]
Patch
Comment 2 Kentaro Hara 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:)
Comment 3 Adam Barth 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.
Comment 4 Kentaro Hara 2012-05-21 00:04:39 PDT
Comment on attachment 142950 [details]
Patch

thanks!
Comment 5 WebKit Review Bot 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
Comment 6 WebKit Review Bot 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
Comment 7 WebKit Review Bot 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
Comment 8 WebKit Review Bot 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
Comment 9 Erik Arvidsson 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.
Comment 10 Adam Barth 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.
Comment 11 Erik Arvidsson 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.
Comment 12 Erik Arvidsson 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 :'(
Comment 13 Adam Barth 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?
Comment 14 Erik Arvidsson 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
Comment 15 Erik Arvidsson 2012-05-21 10:46:22 PDT
http://code.google.com/p/v8/issues/detail?id=2144
Comment 16 Daniel Clifford 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.
Comment 17 Erik Arvidsson 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.
Comment 18 Erik Arvidsson 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
Comment 19 Kentaro Hara 2012-05-21 17:10:29 PDT
Created attachment 143142 [details]
Patch
Comment 20 Ojan Vafai 2012-05-21 17:12:31 PDT
Comment on attachment 143142 [details]
Patch

Glad we could find a good resolution to this.
Comment 21 Kentaro Hara 2012-05-21 17:13:50 PDT
Comment on attachment 143142 [details]
Patch

Thanks arv and vegorov! (The code looks so hacky though...)
Comment 22 Erik Arvidsson 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]();
});
Comment 23 Kentaro Hara 2012-05-21 17:45:19 PDT
Created attachment 143151 [details]
patch for landing
Comment 24 Kentaro Hara 2012-05-21 17:45:48 PDT
arv: Does the patch look good?
Comment 25 Kentaro Hara 2012-05-21 18:03:17 PDT
Created attachment 143156 [details]
patch for landing
Comment 26 Erik Arvidsson 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.
Comment 27 Kentaro Hara 2012-05-21 18:48:58 PDT
Created attachment 143167 [details]
patch for landing
Comment 28 Kentaro Hara 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.
Comment 29 Erik Arvidsson 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
Comment 30 WebKit Review Bot 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.
Comment 31 Kentaro Hara 2012-05-21 19:04:14 PDT
Created attachment 143169 [details]
patch for landing
Comment 32 WebKit Review Bot 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
Comment 33 WebKit Review Bot 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
Comment 34 Kentaro Hara 2012-05-22 00:06:50 PDT
Created attachment 143204 [details]
patch for landing
Comment 35 WebKit Review Bot 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>