Bug 17146 - global variables vs element id's in global scope break JS validation @ code.google.com
Summary: global variables vs element id's in global scope break JS validation @ code.g...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://code.google.com/hosting/create...
Keywords: GoogleBug, HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2008-02-01 17:22 PST by Oliver Hunt
Modified: 2008-02-21 02:20 PST (History)
3 users (show)

See Also:


Attachments
Testcase (694 bytes, text/html)
2008-02-01 17:30 PST, Oliver Hunt
no flags Details
ID (not NAME) creates discrepent behavior with variable binding (885 bytes, text/html)
2008-02-02 13:37 PST, Garrett Smith
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2008-02-01 17:22:34 PST
There is a bug in WebKit that gets exposed due to a bug in http://code.google.com/hosting/createProject projectname validation.
The code that causes the issue problem is:
_exposeExistingLabelFields('edit');
var submit = document.getElementById('submit');
submit.disabled='disabled';
var projectname = document.getElementById('projectname');
var licensekey = document.getElementById('license_key');
var summary = document.getElementById('summary');
var description = document.getElementById('description');
var cg = document.getElementById('cg');
projectname.focus();
var solelyDigits = /^[-0-9]+$/
var hasUppercase = /[A-Z]/
var projectRE = /^[a-z0-9][-a-z0-9]*$/
function checkprojectname() {
    name = projectname.value;
    feedback = document.getElementById('projectnamefeedback');
    submit.disabled='disabled';
    feedback.style.color = 'red';
    if (name == '') {
        feedback.innerHTML = '';
    } else if (hasUppercase.test(name)) {
        feedback.innerHTML = 'Must be all lowercase';
    } else if (solelyDigits.test(name)) {
        feedback.innerHTML = 'Must include a lowercase letter';
    } else if (!projectRE.test(name)) {
        feedback.innerHTML = 'Invalid project name';
    } else if (name.length > 50) {
        feedback.innerHTML = 'Project name is too long';
    } else {
        feedback.innerHTML = '';
        feedback.style.color = '';
        
        checksubmit()
    }
}

At the point _exposeExistingLabelFields is called it does not exist so throws an exception;
this results in "var projectname = document.getElementById('projectname');"  and "var hasUppercase = /[A-Z]/" not being executed 
Later on a user types something into the project name field triggering checkprojectname()
  Due to the "var projectname = ..." statement IE, Opera, and Firefox all shadow the element "projectname", but the first exception prevents projectname from being initialised.  This means that "name = projectname.value;" throws another exception, resulting in the checkprojectname() function exiting without ever validating input.
  In WebKit projectname does not get shadowed, and the first exception prevents the assignment to projectname, so projectname retains its reference to the projectname input element, so we don't throw.
  At this point the submit button gets disabled
  Name now contains content (that's what triggered the call to checkprojectname) so we go on to "} else if (hasUppercase.test(name)) {"
  The original exception has caused hasUppercase to be undefined, so at this point WebKit throws -- so the validation fails in webkit too, but *after* the submit button has been disabled.

So the correct behaviour is to ensure that var declarations shadow those properties on window that are defined by an element with a name attribute.
Comment 1 Oliver Hunt 2008-02-01 17:30:35 PST
Created attachment 18864 [details]
Testcase
Comment 2 Oliver Hunt 2008-02-01 17:37:11 PST
<rdar://problem/5720959>
Comment 3 Garrett Smith 2008-02-02 13:19:31 PST
bar == null is only true due to type conversion. bar should have the value |undefined|

It's not name that's a problem, it's ID. 

In IE, you can't assign to a variable that was created from an element. Unless you use |var| and declare a variable first. 

javascript:void( (function() { try { hork  = 1; } catch(ex) { alert(ex.message) } })());

// fails because IE doesn't support |name|.
javascript:void( (function() { try { hork  = 1; } catch(ex) { alert(ex.name) } })()); 

javascript:alert(Object.prototype.hasOwnProperty.call(window, "hork")); // true

Webkit may have tried to emulate this on a safer level. Create a property for an element with an ID as part of window. Mark it ReadOnly.

Auto-window element props of ID:
 Webkit: ReadOnly
 IE: ReadOnly

Assigning to Auto-window props:
 Webkit: do nothing
 IE: throw Error

Binding Variables to Global Object props:
This means that in global scope/context  |this| is the global object. Variables of the global object become properties:
 var x = 1;
   equivalent to:
 this.x = 1;

magic-prop: a property that was created automagically by the browser from an element's ID.

Binding Variables of same-named magic-props to Global Object as Properties (use var)
 IE: replace the default attributes that the variable was given. It's not readonly after binding. 
 Webkit: Do not bind.

Assigning to window auto-properties (no var)
 IE: When assigning to a of window auto-prop, fail silently, as if it were ReadOnly. 
 Webkit: allow assignment.

That is why usevar is available in Webkit. Webkit doesn't bind it as a property.

Comment 4 Garrett Smith 2008-02-02 13:37:39 PST
Created attachment 18874 [details]
ID (not NAME) creates discrepent behavior with variable binding

The first half of my last comment was inaccurate. Please disregard (I can't edit that)

====

Binding Variables to Global Object props:
This means that in global scope/context  |this| is the global object. Variables
of the global object become properties:
 var x = 1;
   equivalent to:
 this.x = 1;

magic-prop: a property that was created automagically by the browser from an
element's ID.

Binding Variables of same-named magic-props to Global Object as Properties (use
var)
 IE: replace the default attributes that the variable was given. It's not
readonly after binding. 
 Webkit: Do not bind.

Assigning to window auto-properties (no var)
 IE: When assigning to a of window auto-prop, fail silently, as if it were
ReadOnly. 
 Webkit: allow assignment.

That is why usevar is available in Webkit. Webkit doesn't bind it as a
property.
Comment 5 Oliver Hunt 2008-02-02 14:37:19 PST
I think what you're missing is the 
"var x = 1" is not actually equivalent to "window.x = 1", it's a subtle but important difference, it is actually equivalent to
  var x;
  x = "foo"; // where x will be on the current variable object, at the global scope, that will be window

now this is what causes the badness:
try {
throw "my awesome exception";
var x = "foo";
} catcgh (e) {}
alert(x. toString()); //or whatever

is actually equivalent to
var x;
try {
throw "my awesome exception"
x = "foo";
} catch(e) {}
alert(x.toString());

Because all var declarations have to be moved to the beginning of the script.  The rules for this are that the var declarations must all happen first, and that must initialise the declared property to undefined.  So the behaviour is:
var x;  // set window.x to undefined
try {
throw "my awesome exception" // throw
} catch(e) {}
alert(foo. toString());

So when you go to use x, x will be undefined.  *Unless* there was already a global property called x and you don't allow shadowing.  If shadowing is allowed then var x will obliterate any existing property named x; if shadowing is not allowed then "var x" will not convert x to null.  Voila you now different behaviour between browsers: If you allow  shadowing "foo. toString()" will throw due to it being undefined, and if you don't allow shadowing (and x already existed with a defined value) you won't throw.  Woo!

The bug in google code has nothing to do with type conversion -- the bug is that _exposeExistingLabelFields does not exist, causing projectname and hasUppercase (and others) to not be initialised, so checkprojectname throws immediately.   Because WebKit doesn't allow shadowing projectname is not undefined so it throws when it tries to access hasUppercase, thus leading to death and infamy.

The code that does "submit.disabled='disabled'" will work as expected for entirely the wrong reasons, disabled is a bool, so standard js conversion rules apply -- 0-length string = false, non-zero length string = true.



Comment 6 Oliver Hunt 2008-02-02 15:21:39 PST
Ah right, it is id -- although the problem is still the fact that we don't allow shadowing.
Comment 7 Geoffrey Garen 2008-02-02 16:50:13 PST
Oliver, I don't think your analysis of Firefox is 100% accurate. Firefox shadows the property at assignment time, not declaration time. So, if "var projectname =..." never executes, Firefox doesn't shadow projectname.
Comment 8 Garrett Smith 2008-02-02 17:03:47 PST
 Webkit:
loading: loading
'projectname' in window: true
 projectname: undefined
 novar 1
 hork [object HTMLInputElement]
 usevar: [object HTMLInputElement] 

IE:
loading: interactive
'projectname' in window: true
 projectname: undefined
 novar [object]
 hork [object]
 usevar: undefined

IE, novar is [object] -- that's the element that would be projectname in Google'  createProject.

IE doesn't find |novar| on the Variable object; it goes to assign it to window, but this fails because |novar| is some weird readonly magic-prop. If IE is adding members to the global object, it removes the ReadOnly. That is why we have |"projectname"in window|

// projectname got added to the window from a the Variable object, and
// in the process, lost its ReadOnly 
javascript:alert( Object.prototype.hasOwnProperty.call(window, "projectname"))

You can try over and over: hork = 1;

No matter what, IE will keep hork as [object].

var hork = 1 is different in IE. hork is added as a member of the global object (window.hork == 1); 

Webkit, novar is 1. It succeeds in assignment.
But usevar is [HTMLInputElement]. |usevar| did not get added to the window. 

Webkit did add |projectname| to the Variable object, and it was added as a member of the global object.

So why did |usevar| not get created as a window property? You call this "not shadowing" but there is no prototype chain from window > Variable object; nothing to shadow.

In IE |usevar| is undefined. This is because IE7 uses properties of the Variable object as properties of the global object.

(In reply to comment #6)
> Ah right, it is id -- although the problem is still the fact that we don't
> allow shadowing.

Variables are init'd with |undefined|, correct.

Here's my understanding: 
Global Variables don't shadow window properties. Global variables are created as members of the global object; they're created as variables and bound as properties.

There's no shadowing. Values get replaced, but in IE, the ReadOnly attribute gets changed in the process.

> 
Comment 9 Geoffrey Garen 2008-02-02 17:06:34 PST
Even though Firefox doesn't allow var declarations to shadow pre-existing window properties, it honors the "var projectname" declaration in this case because it pretends that window.projectname doesn't exist.

Try this data URL in Firefox, to see what I mean:

data:text/html,<div id="bar"></div><script>alert("bar" in window); alert(window.bar)</script>

Comment 10 Geoffrey Garen 2008-02-04 11:04:26 PST
(Not a WebKit regression.)
Comment 11 Geoffrey Garen 2008-02-04 11:10:55 PST
* STEPS TO REPRODUCE:
1. goto http://code.google.com/hosting/createProject
2. type a character in the project name field

* RESULTS
The submit button becomes disabled, and never gets re-enabled.
Comment 12 Oliver Hunt 2008-02-04 11:47:12 PST
This site breakage is a regression -- this used to work in S2
Comment 13 Oliver Hunt 2008-02-21 02:20:44 PST
Fixed by Google