Bug 85001 - Clean up tests in preparation for ES5.2 global scope fix
Summary: Clean up tests in preparation for ES5.2 global scope fix
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on:
Blocks: 75575
  Show dependency treegraph
 
Reported: 2012-04-26 13:50 PDT by Erik Arvidsson
Modified: 2012-04-27 12:30 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.38 KB, patch)
2012-04-26 13:53 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (5.67 MB, application/zip)
2012-04-27 00:32 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2012-04-26 13:50:39 PDT
Clean up tests in preparation for ES5.2 global scope fix
Comment 1 Erik Arvidsson 2012-04-26 13:53:16 PDT
Created attachment 139063 [details]
Patch
Comment 2 Erik Arvidsson 2012-04-26 13:56:13 PDT
The following code pattern is broken.

    var ident = window.ident || expr;

because it is semantically the same as

    var ident;
    ident = window.ident || expr;

which is the same as

    var ident = undefined;
    ident = window.ident || expr;

which leads to ident being set to expr unless window.ident is read only.
Comment 3 Oliver Hunt 2012-04-26 14:06:35 PDT
Are we sure that this is a web-safe change?  I'm not, I'm fairly sure I've seen this code in the wild.
Comment 4 Erik Arvidsson 2012-04-26 14:27:54 PDT
(In reply to comment #3)
> Are we sure that this is a web-safe change?  I'm not, I'm fairly sure I've seen this code in the wild.

Web safe. Yes. This is how Firefox behaves. WebKit walled garden safe? I'm not sure :'(
Comment 5 Erik Arvidsson 2012-04-26 14:28:09 PDT
Anyway, fixing these tests is harmless.
Comment 6 Erik Arvidsson 2012-04-26 14:41:40 PDT
One important point is that this change in behavior is only for the global scope. The following stil works:

(function() {
  var document = window.document || new MyDocument;
  ...
})();
Comment 7 Alexey Proskuryakov 2012-04-26 16:29:31 PDT
> Anyway, fixing these tests is harmless.

I think that we should achieve a reasonable level of agreement that we want to change behavior here. Losing test coverage for behavior we don't want to change would be detrimental.
Comment 8 Erik Arvidsson 2012-04-26 16:51:09 PDT
(In reply to comment #7)
> > Anyway, fixing these tests is harmless.
> 
> I think that we should achieve a reasonable level of agreement that we want to change behavior here. Losing test coverage for behavior we don't want to change would be detrimental.

How is this losing test coverage?
Comment 9 Alexey Proskuryakov 2012-04-26 17:11:00 PDT
Somehow, it always happens so that if you have 50 tests with seemingly exactly the same code, only one of these will catch a regression one day. Just a law of nature, I guess.
Comment 10 Ojan Vafai 2012-04-26 18:53:40 PDT
(In reply to comment #9)
> Somehow, it always happens so that if you have 50 tests with seemingly exactly the same code, only one of these will catch a regression one day. Just a law of nature, I guess.

All but one of these 12 failures are in relatively new tests, so that gives me some hope that using this pattern in the global scope is not common in the real world.

The spec has changed to require this behavior and I think it's a good thing if we can get away with it. Mainly this allows code to reason about what the value of a variable will be. With WebKit's current implementation, things like JS minifiers need to know about all the IDs on the page to avoid reusing those variable names. Over the years, I've witnessed this cause a number of live production bugs in Google products that were really difficult to track down and to fix.

With the spec-required behavior, at least the crazy behavior is isolated to your one line of code, and only if your one line is in the global scope. You don't have a completely different part of your codebase magically breaking your code.

We'd like to try this in Chromium. If we ship it and there isn't a significant compat problem, then other ports can follow suit. In the meantime, this patch makes it so these tests keep testing what they intend to test for all ports.

Sound ok?
Comment 11 Gavin Barraclough 2012-04-26 20:09:28 PDT
Comment on attachment 139063 [details]
Patch

This change is the right thing to do.  When we make changes to correctly support ES5.2, we should add test cases to defend the new behavior.  If ES5.2 isn't compatible with the web, and we need to revert, then we can retain the new tests but change them to defend the desired web-compatible behavior.
Comment 12 WebKit Review Bot 2012-04-27 00:32:11 PDT
Comment on attachment 139063 [details]
Patch

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

New failing tests:
fast/images/gif-large-checkerboard.html
Comment 13 WebKit Review Bot 2012-04-27 00:32:17 PDT
Created attachment 139142 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 14 WebKit Review Bot 2012-04-27 12:30:39 PDT
Comment on attachment 139063 [details]
Patch

Clearing flags on attachment: 139063

Committed r115470: <http://trac.webkit.org/changeset/115470>
Comment 15 WebKit Review Bot 2012-04-27 12:30:51 PDT
All reviewed patches have been landed.  Closing bug.