289094 - buggy argument shadowing function property special case for lambdas

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jonas Jonsson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050225 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050319

http://www.fatbrain.org/js-bug.html has the indepth description of this problem.

Reproducible: Always

Steps to Reproduce:
1. Create a function => function fn() { <i>[2]</i> }
2. Create a literal object and store it in a var => var o = { [3] };
3. Add two properties to the object 'o' => d: function(x,y) {}, init: function()
{ [4] }
4. function 'init' add two properties to the this.d property => this.d.x =
function(x) {}; this.d.y = function(y) {};
5. Call the function o.init()
6. Call alert([typeof(o.d.x), typeof(o.d.y)]).
7. Call fn(); to see the error (alerts 'function, undefined' should be
'function, function')
Actual Results:  
Assigment of the this.d.y = function(y) {} fails. (In older Firefox versions the
this.d.x ) function(x) {} fails as well, and then crashes the browser)

Expected Results:  
this.d.y should contain a reference to the anonymous function: function(y) {}

Comment 1

[Bob Clary [:bc] (inactive)]

I see the problem in recent builds on winxp as well. 

Comment 2

[Brendan Eich [:brendan]]

Workaround: change the names of the arguments so they don't collide with the
names of the properties.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: too ugly...

... but better than the alternative design-flaw fix, which would be huge.  This
does the trick.

/be

Comment 4

[Brendan Eich [:brendan]]

It's important to summarize this correctly.  Also, here's a shell testcase:

function fn()
{
  var o = { d: function(x,y) {}, init: function() { this.d.x = function(x) {};
this.d.y = function(y) {}; } };
  o.init();
  alert([typeof(o.d.x), typeof(o.d.y)]);
}
alert = print;
fn();

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: js shell testcase, formatted a bit and attached

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 179752 [details] [diff] [review]
too ugly...

Reviewing myself, I see two things wrong here, one big:

1.  No need for prop, passed as an out param to js_DefineNativeProperty -- but
by passing it and then doing nothing to drop its lock on origobj after, we end
up with a stuck lock.

2.  No need for GC_POKE -- we're going from slot-less to slot-full, so there's
no slot to have held a GC-thing ref, so there can't be new garbage after
SetFunctionSlot completes, only more links to existing objects or other things.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: ugly fix, v2

So to recap from IRC:

<brendan> it's all about giving a function object a property with a slot when
that property pre-exists as an argument or local var (which are slot-less)
<brendan> the problem with the current code is that it assumes all args or vars
are in the same scope
<brendan> but with lambdas, you get a prototype function whose scope is at
first shared with the clone on which the first property is set
<brendan> once that's done, the clone has its own scope
<brendan> and just looping over that scope's properties won't find the second
arg
<brendan> which is in the proto-scope, and now needs to be shadowed
(js_DefineNativeProperty with JSPROP_SHARED cleared)

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: ugly fix, v3

Shaver doesn't even *have to review*, just knowing he's looking makes me dance
;-).

This busts out of the loops (setting done to exit the outer one, breaking from
the inner one) on setter/shortid match, whether or not the property is
JSPROP_SHARED. If it has already been promoted to slot-full, we don't want to
keep looping in the current scope, and we *definitely* don't want to go up the
proto-chain, or we may find the shadowed proto-property and waste cycles
re-creating it via js_DefineNativeProperty.

/be

Comment 9

[Bob Clary [:bc] (inactive)]

Checking in regress-289094.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-289094.js,v  <--  regress-289094.js
initial revision: 1.1

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 179761 [details] [diff] [review]
ugly fix, v3

r=shaver, thanks for the walk-through on IRC.

Comment 11

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.x and trunk.