170193 - adding a property after middle-delete of a function with duplicate formal args

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

hangs or crashes the JS engine.  The testcase is easy, given an understanding of
the bug, which I found by reviewing code (for the umpteenth time).  This was run
with the upcoming patch applied, so it shows the correct behavior:

js> function f(a,a,b){}
js> f.c=42
42
js> f.d=43
43
js> delete f.c  // "middle delete"
true
js> f.e=44
44
js> 

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix

Without the tmp variable to count the remaining ancestors in this "middle
delete in the scope of a function with duplicate formals" case, sprop had to be
restored to its value before the do { i++; } while ((sprop = sprop->parent) !=
NULL) loop. But it can't be restored to its pre-loop value by loading spvec[i],
which used to be spvec[0] -- that saved property pointer points to a descendent
(not necessarily the immediate child) of the pre-loop sprop.

Better to use a temporary and leave sprop alone.  I don't know what I was
thinking, but re-reading this case shows the glaring 'sprop = spvec[i];\n  }\n
spvec[--i] = sprop;' sequence, which would store sprop in two adjacent slots in
spvec, leading to a nasty iloop.

Shaver, you know this code -- can you r=?  Thanks.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: precise fix

Let's get i exactly right when correcting the original estimate of the net
ancestor line height (net of middle-deleted properties -- see the big comment
in jsscope.h near the top).  The previous patch assumed that there were no more
middle-deletes lurking on the ancestor line after scope->entryCount net
ancestors had been enumerated into spvec.

/be

Comment 3

[Brendan Eich [:brendan]]

The second patch is really future proofing (an unlikely future, at that): you
can't delete argument properties of function objects (a SpiderMonkey extension),
because they're JSPROP_PERMANENT.  But you'd have to delete a property above
(defined earlier) duplicate formal parameter properties on the ancestor line. 
There ain't no other such property -- native Function.prototype properties are
all permanent too.

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 100169 [details] [diff] [review]
precise fix

r=shaver.

Comment 5

[Brendan Eich [:brendan]]

Fixed, thanks for the fast review!

/be

Comment 6

[Phil Schwartau]

Testcase added to JS testsuite -

      mozilla/js/tests/js1_5/Regress/regress-170193.js

Comment 7

[Phil Schwartau]

Verified FIXED.

The testcase used to hang, but now executes successfully
in both the debug and optimized JS shell on WinNT -