Bug 80329 – [V8] Fix object scope for inline event attribute handlers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Erik Arvidsson]

[V8] Use an interceptor to do the lookup of references in inline event attribute handlers

[Erik Arvidsson]

Created attachment 130208 [details]
Patch

[Ojan Vafai]

Comment on attachment 130208 [details]
Patch

The logic all looks correct to me. Someone more familiar with the V8 code should probably r+ this though.

[Adam Barth]

Comment on attachment 130208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130208&action=review

> Source/WebCore/ChangeLog:9
> +        With this change we introduce an interceptor object in a single with statement instead of using three
> +        nested with statements. This interceptor knows how to correctly lookup the right variable reference.

Is this faster or more correct?  I'm not sure I understand why we're making this change.  (I don't doubt that there's a good reason---I'd just like to understand.)

[Ojan Vafai]

It's more correct. Also, more importantly, it will allow us to start gathering metrics of how sites use this in order to identify what changes we can safely make to the platform. We want to minimize this "feature" as much as possible since it hampers our ability to add new methods to HTMLElement and Document.

[Erik Arvidsson]

Yes this is more correct. Sorry I should have pointed to the Chrome bugs:

http://code.google.com/p/chromium/issues/detail?id=80911
http://code.google.com/p/chromium/issues/detail?id=80912

[WebKit Review Bot]

Comment on attachment 130208 [details]
Patch

Attachment 130208 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11839034

New failing tests:
inspector/debugger/debugger-scripts.html
tables/hittesting/filltable-rtl.html
tables/hittesting/filltable-outline.html
tables/hittesting/filltable-emptycells.html
fast/forms/form-action.html
tables/hittesting/filltable-levels.html
tables/hittesting/filltable-stress.html
fast/events/no-blur-on-page-leave.html
fast/forms/lazy-event-listener-scope-chain.html
fast/harness/results.html

[Adam Barth]

Comment on attachment 130208 [details]
Patch

Looks reasonable.  It looks like you have some test failures to work through.

[Erik Arvidsson]

Yeah, I have to add one more level of indirection to get reassignment to work.

[Erik Arvidsson]

An interceptor does not work because we get the wrong this pointer.

[Erik Arvidsson]

Created attachment 130955 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130955 [details]
Patch

Attachment 130955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11903345

New failing tests:
inspector/debugger/debugger-scripts.html

[Erik Arvidsson]

Committed r110315: <http://trac.webkit.org/changeset/110315>

[Erik Arvidsson]

This will need a rebaseline.