53268 - unexpected sideeffect with proto and scopeprops

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[John Bandhauer]

Sorry I don't know what to call this. It will take a while to explain...

When xpconnect wraps a native object it creates a JSObject to reflect the 
object into JS. That object is created in 
nsXPCWrappedNativeClass::NewInstanceJSObject...

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappednativecla
ss.cpp#1267

See the comment there where it calls JS_NewObject with the global object as the 
proto and then calls JS_SetPrototype(cx, jsobj, nsnull). It does this to avoid a 
lookup.

I've discovered a situation where this dance does not have the same effect as if 
we'd called JS_NewObject with a null proto in the first place.

The patch attached to bug 52936 includes a fix for bug 52591. This fix sets up a 
new Components object for each JS Component Loader global object.

What I've discovered is that with that patch applied then I see a case where a 
JS Component gets: Components.interfaces.nsIFactory = undefined.

I traced this to the code in nsXPCComponents_Interfaces::GetProperty where we 
try to dynamically cache properies of Components.interfaces...
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpccomponents.cpp#1
59

The thing that is failing is the underlying js_SetProperty. 
http://lxr.mozilla.org/seamonkey/source/js/src/jsobj.c#2180

It gets into the block:

if (!sprop || (proto = scope->object) != obj)
  if (sprop)
    if (attrs & JSPROP_READONLY)

But the object in question is supposed to not have a proto. It is just a plain 
JSObject (using the xpc ops and jsclass) constructed as above. It implements 
nsIXPCScriptable to allow getproperty, but should not be hitting this code.

The attr we find is 0x7 which happens to be the same used when the Components 
object is added to the global in nsXPCComponents::AttachNewComponentsObject:
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpccomponents.cpp#1
949

Let me say something about how the global objects work in the JS Component 
Loader (based on my patch bug 52936)...

The SuperGlobal is a plain JSObject made using the jsclass in the component 
loader. We attach an xpconnect Components object to it. A Components object is a 
wrapped native that is attached as linked above. This is done in 
mozJSComponentLoader::ReallyInit

For each .js file we want to read we make a global object. This global is a 
wrapped native. After creation and wrapping, its parent is the super global. We 
then manually use the jsapi to set its parent to null and and its proto to the 
superglobal. We then attach a Components object to this global using 
xpc->InitClasses. This is all in mozJSComponentLoader::ModuleForLocation

So, what we should have is:

super_global: parent = null, proto = null
super_global.Components: parent = super_global, proto = null
global : parent = null, proto = super_global
global.Components: parent = global, proto = null

OK. Here is the test case...

I go to tinderbox and click on the "Add tinderbox to your sidebar" link. This is 
supposed to use the JS component in nsSidebar.js. It fails in 
sidebarModule.getClassObject. By sticking in some dump statements I determined 
it is failing because Components.interfaces.nsIFactory = undefined.

By tracing I see that nsXPCComponents_Interfaces::GetProperty's call to 
CacheDynaProp succeeds in constructing the nsIJSIID object but fails in 
js_SetProperty because it(apparently) thinks this is a readonly property on a 
prototype.

Two work arounds will keep this from happening:

1) don't do the dance in nsXPCWrappedNativeClass::NewInstanceJSObject of 
creating the JSObject using a prototype and then switching to null.

OR

2) don't create a Components object for each global in the JS component loader.

I think there is a deeper engine problem here. Number '1' above shows that the 
assumption behind the proto switching dance is either broken or tripping over 
broken code.

Help!

Comment 1

[Brendan Eich [:brendan]]

I can't keep up.  What is the name of the sprop with the JSPROP_READONLY bit set
in its attributes, and what is the class of the object it was found in (proto in
that js_SetProperty code, at the point you quote)?

*(JSString*)(sprop->id - 4) should give you the ucs2 string for the name of the
property.

*(JSClass *)(proto->slots[2]-1) is the class.

/be

Comment 2

[John Bandhauer]

The sprops name is "nsIFactory" as expected.
The classname of both proto and obj is XPCWrappedNativeWithCall.

I have additional interesting info...

The file we are loading is:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/sidebar/src/nsSidebar.js

That file sets const globals like (most interestingly):

   const nsIFactory       = Components.interfaces.nsIFactory;

So when the Components.interfaces object gets first created with JS_NewObject 
and with proto set to the global object, it really *does* have a proto with a 
readonly, permanent, and enumerable "nsIFactory" prop. The surprise is that 
JS_SetPrototype(cx, jsobj, nsnull) does not fully undo this. jsobj.slot[0] 
certainly ends up being 0. And, though this pseudo-proto property is not 
setable, it is certainly not readable either.

Starting to smell like a fixable bug? It may be that an all JS test case is 
constructable.

Comment 3

[John Bandhauer]

js> const foo = 5
js> foo
5
js> bar = {}
[object Object]
js> bar.__proto__ = this
[object global]
js> bar.foo
5
js> bar.__proto__ = null
null
js> bar.foo
js> bar.foo = 1
1
js> bar.foo
js> bar.baz = 2
2
js> bar.baz
2
js>

Comment 4

[Brendan Eich [:brendan]]

Thanks, you found the big bad hole in my fix for the bug XBL found in JS scope 
sharing.  An empty JS object, when changing prototypes, needs to share the new 
proto's scope, and drop the old one.  But what if the new proto is null?  D'oh!

Patch coming up, looking to get this in today.  Thanks again.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix (beware CRLF at end of lines)

Comment 6

[Brendan Eich [:brendan]]

Adding retroactive dependency for historical purposes.

/be

Comment 7

[John Bandhauer]

r=jband

I'll attach a patch to make xpconnect do the right thing during proto and parent 
set. a= please?

Comment 8

[John Bandhauer]

Attachment: patch to make xpconnect make use of the fix

Comment 9

[Brendan Eich [:brendan]]

trade you a= for r= on my patch, and we're done.  Right?

/be

Comment 10

[John Bandhauer]

checked in xpconnect patch. thanks.

Comment 11

[Phil Schwartau]

Attachment: Testcase that will be added to the JS test suite -

Comment 12

[Phil Schwartau]

Marking Verified - the above testcase passes on WinNT, Linux, and Mac.
Using JS shell built from CVS source 2000-09-21 8PM Pacific Time.

(Note: the testcase has a dependency on js/tests/ecma_3/shell.js.)

I will add the testcase to the ecma_3 directory on Monday, in a subdirectory 
called "Scope". If that is the wrong subject heading, please let me know -

Comment 13

[Phil Schwartau]

Testcase has been added to suite as follows:

                     js/tests/js1_5/Scope/scope-001.js