72773 - Crash in debug JS shell

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Phil Schwartau]

Using standalone JS shells built 2001-03-20 on WinNT. Consider this:

function Cow(name){this.name=name;}
function Calf(str){this.name=str;}
Calf.prototype= Cow;
new Calf().toString();


I get this error in the optimized shell -

  TypeError: Function.prototype.toString called on incompatible object


But I CRASH in the debug shell. Either one of the following modifications, 
however, gives me the warning instead of the crash:

1. Cow(str){this.name=str;}     instead of   Cow(name){this.name=name;}
2. new Calf.toString()          instead of   new Calf().toString()


And either of the following modifications gives no crash or warning:

3. Calf.prototype = Cow();      instead of   Calf.prototype = Cow;
4. Calf.prototype = new Cow();  instead of   Calf.prototype = Cow;


The latter two make sense to me: they make Calf.prototype an Object object
instead of a Function object, so Function.prototype.toString is not involved(?)

Comment 1

[Phil Schwartau]

Attachment: WinNT stack trace + some extra info

Comment 2

[Brendan Eich [:brendan]]

There are a bunch of related bugs.  Here's another one:

function Cow(){return arguments}
function Calf(){print(this.length)}
Calf.prototype = Cow();
new Calf.toString();

The general problem is that many getters and setters in the engine, written long
ago, fail to check that their obj parameter is of the right class for them to
safely get the right type of private data structure from JSSLOT_PRIVATE.  Patch
coming up.

ECMA dodges this incompatible class problem by not specifying inheritance of
[[Get]] and [[Put]].  SpiderMonkey does inherit getters and setters in order to
avoid creating direct properties in numerous objects, e.g. length in each and
every arguments object (instead, Arguments.prototype.length is shared among all
instances, conserving space).

The original example runs afoul of SpiderMonkey's extension to define formal
parameter names as properties of their function object (Cow.name is inherited by
Calf instances, so setting this.name = str in Calf invokes the setter for name,
js_SetArgument, which fails to check for compatible 'this'/'obj' class).

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 5

[Brendan Eich [:brendan]]

What, I double-clicked on Submit in the bugzilla attachments page again?

Pollmann, I'm gonna bug you unless you say something fast.  Submit must become
insensitive after the first click.

Anyway, ignore those patches.  I'm reattaching a better one.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: real proposed fix

Comment 7

[Brendan Eich [:brendan]]

Checkin comments:

- [jsemit.c]  Fix horrid stupid bugs generating JSOP_ARGCNT and JSOP_ARGSUB,
  where any occurrence of arguments.length or arguments[0], e.g., would be
  "optimized" to use those bytecodes.  This is just wrong if the occurrence
  is an operand of delete, ++, --, or the left-hand-side of an assignment
  operator!

- [jsfun.c, jsinterp.c]  args_getProperty etc. must use JS_GetInstancePrivate,
  not JS_GetPrivate, as the arguments object is exposed, and can be made a
  prototype of other objects that do not have private data, or private data
  that's a JSStackFrame*.  Same goes for fun_getProperty, js_GetArgument, etc.

- [jsfun.c, jsobj.c, jsstr.c]  No need to specialize fun_delProperty and
  str_delProperty to help convince users and ECMA conformance tests that
  fun.length and str.length are not direct properties of instances, but are
  instead delegated to Function.prototype.length and String.prototype.length.
  This special case is done universally in js_DeleteProperty for all SHARED
  and PERMANENT proto-properties.

- [jshash.c]  Sneaking this followup-fix for bug 69271 in: use JS_HASH_BITS
  rather than hardcoded 32.

- [jsobj.c, jsscope.[ch]]  Fix misnamed js_HashValue (it takes a jsid, so it
  is now js_HashId).

- [jsscript.c] script_compile needs to call JS_InstanceOf, to ensure that obj
  is a Script object.

r/sr= soon, please!

/be

Comment 8

[Brendan Eich [:brendan]]

I'm mortified by the [jsemit.c] bug.  It makes

  print((function (){return ++arguments[0])})(42));

print 42, not 43.  Code buddies, save me!

There's still an ECMA deviation here: our Arguments class does not match 10.1.8,
which says arguments objects have as their prototype the original value of the
Object.prototype property.  What's more, we delegate properties such as length,
0, 1, etc. to Arguments.prototype.  So in SpiderMonkey,

  function f(){delete arguments.length; return arguments.length}
  print(f(1,2,3));

prints 3, rather than undefined as it should per ECMA.  To fix this, I'd like
another bug.  Phil, can you pitch one at me (and update the testsuite to cover
all the horrid bugs here)?  Thanks.

/be

Comment 9

[Phil Schwartau]

I have filed bug 72884 for this issue, and will add test coverage -

Comment 10

[Eric Pollmann]

Brendan, double click -> double submission is now filed as bug 72906.

Comment 11

[rogerl (gone)]

r=rogerl

Comment 12

[Brendan Eich [:brendan]]

pollmann: thanks.

shaver or jband, how about an sr=?

/be

Comment 13

[John Bandhauer]

sr=jband  I trust you ran the tests.

Comment 14

[rogerl (gone)]

(i did on WinNT, 0 errors)

Comment 15

[Brendan Eich [:brendan]]

Fix checked in -- thanks all (yes, I run the fine tests, too).

/be

Comment 16

[Phil Schwartau]

Testcase added to JS testsuite: 

              js/tests/ecma_3/Object/regress-72773.js


Testcase passes on WinNT,Linux in debug/optimized JS shells; 
marking bug VERIFIED -