Bug 85001 – Clean up tests in preparation for ES5.2 global scope fix

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Erik Arvidsson]

Clean up tests in preparation for ES5.2 global scope fix

[Erik Arvidsson]

Created attachment 139063 [details]
Patch

[Erik Arvidsson]

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.

[Oliver Hunt]

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.

[Erik Arvidsson]

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

[Erik Arvidsson]

Anyway, fixing these tests is harmless.

[Erik Arvidsson]

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

[Alexey Proskuryakov]

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

[Erik Arvidsson]

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

[Alexey Proskuryakov]

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.

[Ojan Vafai]

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

[Gavin Barraclough]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 139063 [details]
Patch

Clearing flags on attachment: 139063

Committed r115470: <http://trac.webkit.org/changeset/115470>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.