131964 - function declaration bug? delete on global function should not delete the function

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Georg Maaß]

Thomas Much wrote in netscape.public.mozilla.jseng:
> Hi,
> 
> I think I've found a bug in Mozilla's JS engine (Moz 0.9.9 Mac) - at
> least an incompatibility to ECMA 262-3. If you run this in global code
> 
>   var a;
>   delete a; // "a" still there - OK
> 
> But if you run this (again in global code)
> 
>   function test() {}
>   delete test; // "test" is gone...
> 
> the test property is actually deleted.
> 
> ECMA 262-3, section 13 says that function declarations create a property
> of the current variable object as specified in 10.1.3.
> 10.1.3 says that for function declarations, the property's attributes
> are determined by the type of the code.
> 10.2.1 says that for global code, variable instantiation is performed
> [...] using property attributes { DontDelete }.
> 
> So, IMHO "test" should not have been deleted in the example above.

Comment 1

[Brendan Eich [:brendan]]

This is real, thanks for reporting it.  Kenton, can you patch it for 1.0? 
One-line change to jsinterp.c, the JSOP_DEFFUN case:

                                     (attrs & JSFUN_SETTER)
                                     ? (JSPropertyOp) obj
                                     : NULL,
-                                    attrs | JSPROP_ENUMERATE,
+                                    attrs | JSPROP_ENUMERATE |
+                                    JSPROP_PERMANENT,
                                     NULL);
            if (!ok)
                goto out;

(Hacked patch, not a real patch.)

/be

Comment 2

[Phil Schwartau]

Attachment: HTML testcase

Comment 3

[Kenton Hanson (gone)]

Attachment: Patch to set correct attributes

Here is the patch. Should I r=?

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 75076 [details] [diff] [review]
Patch to set correct attributes

sr=shaver; it's easier than reading ECMA!

Comment 5

[Phil Schwartau]

Testcase added to JS testsuite:

          mozilla/js/tests/ecma_3/Function/regress-131964.js

At present, it tests four scenarios:

1. f defined and deleted in global scope
2. f defined and deleted in function scope
3. f defined and deleted in eval scope
4. f defined in eval scope, but deleted in global scope

According to my understanding of ECMA, in scenarios 1 and 2, f is
{DontDelete}. SpiderMonkey is failing scenario 1 (hence this bug),
but is passing scenario 2. All good so far.

In scenario 4, I have an eval string |s| containing the definition of f.
Then in global scope, I do

                            eval(s);
                            delete f;

and f does NOT get deleted.

By contrast, in scenario 3, BOTH the definition of f and the delete
are inside the eval string |s|. Then I do eval(s) in global scope.
In this case, f DOES get deleted. 

I don't understand the ECMA spec on this well enough to know whether
both scenario 3 and 4 are performing according to spec. Currently, 
I have the test set to expect f to be deleted in both 3 and 4.

Need advice!

Comment 6

[Brendan Eich [:brendan]]

Worksforme:

Starting program: /home/brendan/src/trunk/mozilla/js/src/Linux_All_DBG.OBJ/js 
js> s = 'function f(){return 42}'
function f(){return 42}
js> eval(s)
js> f

function f() {
    return 42;
}

js> delete f
true
js> f
5: ReferenceError: f is not defined
js> eval(s + '\ndelete f')
true
js> f
7: ReferenceError: f is not defined
js> 

Program exited with code 03.
(gdb) q

What's your test for steps 3 and 4, exactly?

/be

Comment 7

[Phil Schwartau]

Here are scenarios 3 and 4. In each case, I expect the function 
to be deleted, but I'm not sure that is correct -

/*
 * Try the test in eval scope -
 */
status = inSection(3);
s = '';
s += 'function h()';
s += '{ ';
s += '  return "h lives!";';
s += '}';
s += 'delete h;';

s += 'try';
s += '{';
s += '  actual = h();';
s += '}';
s += 'catch(e)';
s += '{';
s += '  actual = "h was deleted";';
s += '}';

eval(s);
expect = 'h was deleted';


/*
 * Define the function in eval scope, but delete it in global scope -
 */
status = inSection(4);
s = '';
s += 'function k()';
s += '{ ';
s += '  return "k lives!";';
s += '}';
eval(s);

try
{
  actual = k();
}
catch(e)
{
  actual = 'k was deleted';
}

expect = 'k was deleted';

Comment 8

[Phil Schwartau]

Test output: scenario 3 passes in the current JS shell, but 4 fails:

FAILED!: Section 4 of test -
FAILED!: Expected value 'k was deleted', Actual value 'k lives!'

Probably because I'm expecting the wrong thing, but I'm not sure...

Comment 9

[Phil Schwartau]

I will now try the test with the patch applied, and report below.

Comment 10

[Phil Schwartau]

Oh dear - now that it's posted here, I see my blunder:
I forgot to do the delete in scenario 4. Sorry !!!

Comment 11

[Phil Schwartau]

OK, now that I've fixed my blunder, these are the results I get 
from the test after I apply the patch:

FAILED!: Section 3 of test -
FAILED!: Expected value 'h was deleted', Actual value 'h lives!'

FAILED!: Section 4 of test -
FAILED!: Expected value 'k was deleted', Actual value 'k lives!'


I had to apply the above patch by hand; I think I did it right.

Comment 12

[Phil Schwartau]

From ECMA-262 Edition 3 Final:

10.2.1 Global Code
• The scope chain is created and initialised to contain the global object
  and no others.
• Variable instantiation is performed using the global object as the
  variable object and using property attributes { DontDelete }.
• The this value is the global object.

10.2.2 Eval Code
When control enters an execution context for eval code, the previous active 
execution context, referred to as the calling context, is used to determine
the scope chain, the variable object, and the this value. If there is no
calling context, then initialising the scope chain, variable instantiation,
and determination of the this value are performed just as for global code.

• The scope chain is initialised to contain the same objects,
  in the same order, as the calling context's scope chain.
  This includes objects added to the calling context's scope chain
  by with statements and catch clauses.
• Variable instantiation is performed using the calling context's
  variable object and using empty property attributes.
• The this value is the same as the this value of the calling context.

10.2.3 Function Code
• The scope chain is initialised to contain the activation object
  followed by the objects in the scope chain stored in the [[Scope]] property
  of the Function object.
• Variable instantiation is performed using the activation object as the         
  variable object and using property attributes { DontDelete }.
• The caller provides the this value. If the this value provided by the
  caller is not an object (including the case where it is null), then the
  this value is the global object.

Comment 13

[Phil Schwartau]

Is my testcase incorrect for eval scope? Section 10.2.2 above says 

• Variable instantiation is performed using the calling context's
  variable object and using empty property attributes.

That means { DontDelete } should be empty, right? That's why in
sections 3 and 4 of the test, I expect the functions to be deletable -

Comment 14

[Kenton Hanson (gone)]

Comment on attachment 75076 [details] [diff] [review]
Patch to set correct attributes

r=khanson

Comment 15

[Phil Schwartau]

-------------------    SUMMARY OF TEST RESULTS    ---------------------------
The test checks delete of functions defined in

1. global scope
2. function scope
3. eval scope (delete also done within eval scope)
4. eval scope (delete done in global scope)


-------------------    OUTPUT BEFORE PATCH    ---------------------------
FAILED!: Section 1 of test -
FAILED!: Expected value 'f lives!', Actual value 'f was deleted'


-------------------    OUTPUT AFTER PATCH    ----------------------------
FAILED!: Section 3 of test -
FAILED!: Expected value 'h was deleted', Actual value 'h lives!'

FAILED!: Section 4 of test -
FAILED!: Expected value 'k was deleted', Actual value 'k lives!'


So the patch has fixed the bug in global scope, but has changed the
behavior in eval scope. It used to be that functions defined in 
eval scope were deletable; now apparently they are not -

Comment 16

[Brendan Eich [:brendan]]

Attachment: i whiffed it last time, here's the right proposed fix

See JSOP_DEFVAR's case, where JSFRAME_EVAL is tested.  Thanks to shaver, who
looked on and says sr=him.

/be

Comment 17

[Phil Schwartau]

The latest patch passes the testcase for this bug. Furthermore, it passes
the entire JS testsuite on WinNT, in both the debug and optimized shell -

Comment 18

[Mike Shaver (:shaver emeritus)]

Comment on attachment 75322 [details] [diff] [review]
i whiffed it last time, here's the right proposed fix

A comment and everything! sr=shaver.

Comment 19

[Kenton Hanson (gone)]

Comment on attachment 75322 [details] [diff] [review]
i whiffed it last time, here's the right proposed fix

r=khanson

Comment 20

[Brendan Eich [:brendan]]

Got the patch, taking the bug.

/be

Comment 21

[Scott Collins]

Comment on attachment 75322 [details] [diff] [review]
i whiffed it last time, here's the right proposed fix

a=scc

Comment 22

[Brendan Eich [:brendan]]

Fixed, thanks all.

/be

Comment 23

[Phil Schwartau]

Marking Verified - the above testcase passes in the debug/optimized
JS shell on WinNT, Linux, and Mac9.1