35738 - Prototype Overwrite Problem

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Till Vollmer]

This is the famous prototype overwite problem.

I will once more explain the problem. A test program that only uses libjs and 
nspr4 is attached.

Setup

1) Two JSScipts containing the following scripts:
Script1:

function X() 
{}
function a()
{
  x.__debug__() ;
}
X.prototype.doIt=a ;

Script2:
var l = new X() ;
a();
l.doIt() ;
x.__debug__() ;

2) a runtime and a compile context are created
3) a compile global is created
3) scripts are compiled (against the compile global in the compile context)
4) two threads are created (each with own execution global(linked via prototype 
chain to the compile global) and with own execution context
Also a global host object x is introduced, with private data. The pointer to 
the private data can be displayed with .__debug__().
5) run the two scripts SLICED in a loop. This means that after the script1 of 
the first thread the script1 of the second thread is run and then the script2 
of the first thread is run and then the script2 of the second thread. Do this a 
couple of times.

The output shows the following:

HWJS_Server::server_debug() 0x807c860
HWJS_Server::server_debug() 0x807c860
HWJS_Server::server_debug() 0x807c860
GC start
GC end
HWJS_Server::server_debug() 0x8089df0
HWJS_Server::server_debug() 0x807df98 <-------
HWJS_Server::server_debug() 0x8089df0
GC start
GC end
HWJS_Server::server_debug() 0x807df98
HWJS_Server::server_debug() 0x808b640 <-------
HWJS_Server::server_debug() 0x807df98
GC start
GC end
finished
HWJS_Server::server_debug() 0x808b640
HWJS_Server::server_debug() 0x808b640
HWJS_Server::server_debug() 0x808b640
GC start
GC end
finished


The <---- values are wrong. It seems that when using a prototype method the 
lookup is wrong.

Please note also that the softlocking is disabled in the testprogram, which 
means that only one thread can enter the whole JS engine at once! Switching 
back to the use of JS_BeginRequest/EndRequest does not change anything.

Comment 1

[Till Vollmer]

Attachment: The test program

Comment 2

[Brendan Eich [:brendan]]

Hi Till, sorry I didn't get to this before returning from sabbatical.  Thanks 
for your long-suffering patience.

Here are my conclusions:

1.  The test does not "slice" or interleave execution as you write, because 
there is no usleep() call at the bottom of the loop body (after script2's 
execution).  So the order is first-thread-script-1, second-thread-script-1, 
first-thread-script-2, first-thread-script-1(-2nd-time), second-thread-script-2, 
second-thread-script-1(-2nd-time), etc.  Note that "first" and "second" above 
might not refer to the thread creation order, due to LIFO scheduling.

2.  I've hacked on your test program a bit to make it log thread ids and other 
useful information, so that the order of events can be more clearly seen.  See 
upcoming attachment.

3.  The bug is an interaction between an ancient optimization in jsobj.c's 
js_LookupProperty, and the more recent function-clone 'prototype' resolution 
code in fun_resolve.  The fun_resolve code is never called on cloned function 
objects, after being called once on the first use of X.prototype (where X is the 
first clone of the function created when the compiler saw function X(){}).  My 
proposed fix is coming up in a second attachment -- stay tuned.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: updated test program

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 5

[Brendan Eich [:brendan]]

Adding shaver, he has history here.  Adding jband cuz he has helped with jsobj.c 
code review before.  Someone give me an r= and I'll get this checked in.

The fix to jsfun.c is independent of this bug, but needed if ever a function 
clone-parent were finalized before someone tried to resolve 'prototype' against 
a clone-child.

The fix to jsobj.c undoes an old optimization: since JS was born, prototype 
objects and unmutated "child objects" that delegate to the prototype all share 
the same reference-counted scope ((JSScope*)obj->map).  But the scope's object 
back-pointer points only to the prototype (((JSScope*)obj)->scope->object==obj 
iff obj is the prototype, or rather, iff obj has been mutated).  You can create 
an unmutated clone object via 'new C' where C is any constructor function that 
does not set this.newProperty = 42, e.g. -- any constructor that does not mutate 
its 'this' parameter.

So when looking for properties up the prototype chain (ECMA [[HasProperty]] and 
[[Get]]), JS has taken advantage of the scope sharing among unmutated clones 
lower on the prototype chain, and their scope-owning prototype above.  jsobj.c's 
js_LookupProperty would go directly to the prototype's scope from the clone, and 
not try the JSClass.resolve hook against the clone.  What's more, if it saw the 
same scope in two objects adjacent on the prototype chain, but had still not 
found the property in question, it would skip (goto skip in the old revision) 
the second futile scope-search.

There's the bug: this scope-skipping, and the shortcut of going straight up to 
the prototype's scope from the prototype's clone-child object, rendered resolve 
a second-class operation.  A resolve implementor could not be sure that each 
object being directly addressed, or indirectly if not at the bottom of a proto 
chain, would be given a chance to create the asked-for property.  fun_resolve in 
particular could not create a .prototype property for each function object clone 
created when doing "brutal sharing" (compiling scripts once, executing against 
several disjoint scopes -- see also ECMA ed. 3's "joined function objects").

I fixed by removing the redundant scope-skipping code in js_LookupProperty, and 
more important, by calling resolve without shortcutting to the prototype scope 
if the current object on the proto chain is sharing a scope that it does not own 
(is sharing its prototype's, or grand-prototype's, etc. scope).

/be

Comment 6

[clayton]

Adding Norris to cc list.

Comment 7

[Brendan Eich [:brendan]]

Got an r=[e-mail], I'm going in.

Till, have you tried the patch yet?

/be

Comment 8

[Brendan Eich [:brendan]]

Fix checked in.

/be