551763 - sputnik S13_A11_T1 - 'delete arguments' in a function deletes global.arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Sputnik test S13_A11_T1 fails in SpiderMonkey. Here's a slightly different test I wrote that shows exactly what's going wrong:

var arguments = 7;
function f() {
    assertEq(delete arguments, false);   // fails
}
f();
assertEq(arguments, 7);  // fails; we deleted the global
print(" PASSED!");

The easiest thing might be to compile `delete arguments` to `false` in the front end.

Comment 1

[Jason Orendorff [:jorendorff]]

Related to bug 530650?

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> Related to bug 530650?

Yes, I was about to point that one out. Luke knows about it too, since the patch in that bug classifies functions that use arguments (or could use arguments), and flags them.

/be

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Arguments deletion fix.

We're only broken on lightweight functions; i.e.

js> function f() { print(delete arguments) } 
js> function g() { print(delete arguments); with(this) {} }
js> f(), g()
true
false

The heavyweight lookup resolves 'arguments appropriately via JSOP_DELNAME through js_FindProperty. We could just flip it to heavyweight if we see somebody is trying to delete 'arguments. Alternatively, we could return false when we see a deletion of 'arguments in a lightweight function invocation in the JSOP_DELNAME implementation.

We actually mark as heavyweight on assignment to arguments in the parser's NoteLValue function, so doing the same on deletion seems reasonable. Patch including test cases is attached.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 432252 [details] [diff] [review]
Arguments deletion fix.

Nice fix. The only nit is that trace-tests are for testing the tracing JIT.

So please put the test someplace like js/src/tests/js1_8_5/regressions/regress_551763.js, add a line to the jstests.list file in that directory, and to make the test pass in the test-runner, add

  print(" PASSED!");

at the end.

(You could instead call reportCompare -- but then your test depends on a function provided by the test-runner, and ideally tests should also run standalone in the shell, if possible.)

Comment 5

[Robert Sayre]

(In reply to comment #4)
>
> test-runner, add
> 
>   print(" PASSED!");
> 
> at the end.
> 
> (You could instead call reportCompare -- but then your test depends on a
> function provided by the test-runner, and ideally tests should also run
> standalone in the shell, if possible.)

Will that work when the test is run in the browser? print() does something altogether different there. I think reportCompare is needed anyway for that situation.

Comment 6

[Jason Orendorff [:jorendorff]]

browser.js defines print() to do a document.write.

I don't know how the jsreftest runner works, but browser.js doesn't overload the reportCompare function.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Fix arguments deletion.

Puts test in the right place.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Arguments deletion fix and tests.

Update -- this has been sitting for a while.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 442849 [details] [diff] [review]
Arguments deletion fix and tests.

Separate each test into its own file -- in particular, at the very least the two global tests both test the same thing, because the scope of a |var| in global scope is the entire script -- *not* just after it in source order.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/24a3be30208e

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/24a3be30208e