Bug 65523 – Redeclaration of var should have explicit test case.

• Component: Tools / Tests
• Status: UNCONFIRMED

[Jakob Kummerow]

According to EcmaScript 5 Section 15.1.1.3, "undefined" is a read-only property of the global object and as such cannot be re-declared as a global variable. Some layout tests, however, do precisely this. This has become an issue because V8 does, as of now, properly set the read-only flag of "undefined", and therefore throws an exception when the re-declaration attempt occurs. Fortunately, the fix is easy: since "undefined" is available as a value anyway, the conflicting redeclarations can simply be removed.

Affected tests:
* editing/pasteboard/data-transfer-items
* editing/pasteboard/onpaste-text-html
* fast/events/ondrop-text-html
* fast/js/char-at

[Jakob Kummerow]

Created attachment 102628 [details]
Proposed patch

[Adam Barth]

Comment on attachment 102628 [details]
Proposed patch

This looks fine, but the patch isn't based in the correct directory, so we won't be able to land this patch automatically.  Please see these instructions for preparing patches:

http://www.webkit.org/coding/contributing.html

If you're using a Chromium checkout, you might not be able to create correct WebKit patches.  Please see http://www.webkit.org/building/checkout.html for instructions on creating a WebKit working copy.

Thanks for the patch.

[Jakob Kummerow]

Created attachment 102658 [details]
Proposed patch (with correct file paths)

Re-created the patch based on a WebKit (rather than Chromium) checkout. No other changes.

[Alexey Proskuryakov]

CC'ing potential reviewers. The patch makes sense, but it doesn't make sense to shortcut the review process by not letting ECMAScript experts weigh in.

[Adam Barth]

Comment on attachment 102658 [details]
Proposed patch (with correct file paths)

No one's short circuiting any review process.  There are a couple of questions here:

1) Should we add a test for this behavior specifically?  None of these tests appear to be testing this behavior intentionally.
2) Should we change JSC to match Firefox (and shortly V8) in throwing an exception here.

Neither of these questions need to block this patch.

[Oliver Hunt]

Redeclaration should only throw an exception in strict mode -- why is an exception being thrown now?

[Adam Barth]

@jkummerow, you say that Firefox throws as well, but the following code shows the alert in Firefox 5.0.1:

<script>
var undefined;
alert('done');
</script>

[Jakob Kummerow]

(In reply to comment #7)

I said: Firefox throws when you redeclare a const, and V8 doesn't distinguish internally between consts and read-only properties of the global object, so it'll throw also when redeclaring a read-only property of the global object.

This code will throw in Firefox:
<script>
const undefined;
var undefined;  // <- exception
alert("boo");
</script>

Regardless of this, the proposed patch doesn't break anything, as the removed declarations are ignored anyway (or at least *should* be ignored, and indeed are at least on Firefox and bleeding-edge V8; I'm at home now and don't have Safari around for testing):
<script>
var undefined = 42;
alert("undefined: " + undefined);  // prints "undefined: undefined"
</script>

[Adam Barth]

So these declarations don't cause problems in any browser, but they will in bleeding edge V8?  That sounds like a compat problem with bleeding edge V8 that should be fixed.  We want to match the behavior of other browsers.  If no one throws on this input, then we shouldn't either.

[Gavin Barraclough]

Based on the above comments it seems that these tests are currently correctly passing, and in doing so are ensuring correct behavior in the browser (that non-strict code can redeclare a var).

I guess an argument could be made that checking of this behavior shouldn't be scattered amongst pasteboard/events tests, so we could create a new test case to explicitly test this.

Otherwise this bug is probably invalid.

[Adam Barth]

> Otherwise this bug is probably invalid.

That's my sense at the moment as well, but we should confirm with jkummerow to make sure we're not misunderstanding.

[Jakob Kummerow]

You have a valid point regarding throwing behavior (comment #9). Somehow this has slipped through. We have reverted the V8 patch in question and will come up with some other solution.

Nonetheless, the tests affected by my proposed patch contain unnecessary and ignored variable declarations. My suggestion to remove these still stands. As comment #10 says, a dedicated test would make more sense, if this behavior needs to be tested at all. But I don't feel strongly about this -- feel free to abandon/close this bug if you prefer.

[Gavin Barraclough]

Seems like a sensible thing to do.  Renaming bug as appropriate.

[Eric Seidel (no email)]

Comment on attachment 102628 [details]
Proposed patch

Cleared Adam Barth's review+ from obsolete attachment 102628 [details] so that this bug does not appear in http://webkit.org/pending-commit.