70358 - call to DefineProperty w/ getter or setter recurs in Resolver

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[John Bandhauer]

I can't see how people are supposed to call JS_DefineProperty for a property 
with a getter or setter from their Resolve hook. If either the 
getter or setter flag is set then js_DefineProperty calls js_LookupProperty and 
that recurs into the Resolve hook. Am I missing something or is this a legit 
bug?

I've worked around this in my xpconnect changes. But, my fix is a hack and only 
safe because I have a TLS struct handy to help me deal with the recursion 
without risking screwing up anything else.

Comment 1

[Brendan Eich [:brendan]]

Yeah, recursion in resolve has been a thorn in people's sides for too long. I
remember [e-mail] (long departed) hacking around it in the plugins and
mimeTypes reflections in 4.x.  I'll try to address it centrally.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 3

[Brendan Eich [:brendan]]

Looking for review.  Instead of maintaining cx->resolving only under
JS_ResolveStandardClass, which is called from global class resolve hooks, we use
cx->resolving in js_LookupProperty to protect against recursion for all resolve
hook calls.

/be

Comment 4

[Brendan Eich [:brendan]]

The resolve logic in js_LookupProperty is a bit ugly now, but it will be split
out into a common subroutine for bug 72354 soon (make me do it now if you like).

/be

Comment 5

[John Bandhauer]

I don't think I buy this. IIRC, you used a table in the ResolveStandardClasses 
code to deal with nested resolves of different ids. That was fine there were you 
*know* that all the resolves are happening on the same object. But more 
generally, I think you risk failing to call resolve on some object when you 
should be calling it. Say you are looking up 'foo' on object A and A's resolver 
decides to lookup foo on object B. Wouldn't you then be skipping the call to 
resolve 'foo' on B?

It seems to me you need to track {id,obj} in the table. No?

Comment 6

[Brendan Eich [:brendan]]

Argh, you're right.  New patch coming (probably tomorrow).

/be

Comment 7

[Brendan Eich [:brendan]]

I suck, but I will fix this soon -- its fix is needed for bug 72354.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: better fix (also patches the xpcdom js_SetProperty hack to not leak)

Comment 9

[Brendan Eich [:brendan]]

I don't understand how that TEMPORARY HACK does not result in stuck locks and
property leaks.  Jband, can you breakpoint your resolve function(s) and show
them being called from that js_LookupProperty?

Comments and testing help on the patch much appreciated.  It should relieve the
XPConnect code of having to check for resolve recursion.

/be

Comment 10

[John Bandhauer]

brendan:

Components.classes["@mozilla.org/js/xpc/test/Echo;1"].
    createInstance(Components.interfaces.nsIEcho).
        SomeValue = 5;

If you save the above to foo.js and run 'xpcshell foo.js' you'll see one and 
only one call to js_SetProperty. This is not a case where xpconnect would 
resolve on a prototype (since the echo object does not implement nsIClassInfo 
and thus enable xpconnect proto sharing) - but the resolver would need to 
resolve on the instance in order to get the cloned setter function.

Tracing in the debugger with the TEMPORARY HACK... it *does* do the resolve in 
the lookup hack inside setproperty and it does end up calling the setter in the 
xpcom object. I see the unbalance in the sprop->nrefs. I'm not clear on the 
leakage model here for the sprops. The JSObject is clearly cleaned up else 
xpconnect would leak the wrapper. The lock is effectively nop'd in this case 
since the object is only accessed on one JSContext (right?). I suspect we are 
just 'lucky' in the browser as far as stuck locks go because we call very few 
(but more than zero) xpconnect setters. Perhaps our normal paths have not hit 
any cross JSContext previously unresolved setter calls? I dunno.

I'll dig into your patch. Thanks!

Comment 11

[Brendan Eich [:brendan]]

Attachment: better patch, tighten up non-native object resolve logic

Comment 12

[John Bandhauer]

I'm not finding any problems with this. I'm struggling through all the 
code paths. I like the new MAP_IS_NATIVE check. I'm wondering how we got away 
without the lock juggling before. The resolving table stuff looks right to me. 
It seems to all run right. I see you're being clear that this is another place 
where funny things can happen if obj and obj2 don't have a proto relationship. 
I'm glad I backed off from trying to use that pattern in my code.

I'm ready to go for a r/sr=jband with the confession that I'm giving up on  
verifying proper cleanup on all the code paths. Maybe shaver or rogerl can give 
better verification.

Comment 13

[Brendan Eich [:brendan]]

In the 'if (resolve != JS_ResolveStub) {...}' block, there are only the
following exits:

1.  goto out if cx->resolving alraedy contains (obj,id), after unlocking obj.
2.  goto outofmem, see below.
3.  the outofmem: labeled code, which also unlocks obj and returns.
4.  goto cleanup after newresolve failure, with ok == false and obj unlocked.
5.  goto cleanup after the non-native OBJ_LOOKUP_PROPERTY, with ok false or
*propp non-null.  [*]
6.  goto cleanup after resolve failure, with ok == false and obj unlocked.

Cases 4-6 rely on cleanup: returning after cleaning up cx->resolving, if !ok or
*propp.

There is the possibility (see [*] above) that OBJ_LOOKUP_PROPERTY could return
true with null *propp, meaning not-found.  This would be a case where newresolve
lied, or a racing deleter won.  In this case, the code at cleanup would not
return early and we'd fall into the main do-while loop update clause that starts
with 'proto = LOCKED_OBJ_GET_PROTO(obj); JS_UNLOCK_OBJ(cx, obj);' -- but obj was
unlocked in this case before we called out to OBJ_LOOKUP_PROPERTY.  D'oh!

New patch coming up, thanks for making me look.

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: best patch yet -- final one for this bug, I hope

Comment 15

[John Bandhauer]

r/sr=jband. looks good. worksforme.

Comment 16

[Johnny Stenback (:jst)]

I had a look at the changes and based on the somewhat limited knowledge about JS
engine internals that I have the changes look good to me, one thing I noticed in
jsobj.cp was:

@@ -2322,6 +2441,8 @@
     /* XXX - TEMPORARY HACK */
     if (!js_LookupProperty(cx, obj, id, &tmp, (JSProperty **)&sprop))
         return JS_FALSE;
+    if (sprop)
+        OBJ_DROP_PROPERTY(cx, tmp, (JSProperty *)sprop);
 
     rt = cx->runtime;
     JS_LOCK_OBJ(cx, obj);

The XXX comment was added by jband, right? Is now not a good time to remove that
comment?

Other than that I don't see any problems with the patch, I'm offering my r=jst
here, but since I'm not all that familiar with this code it's up to you to
decide if you wanto check in with my r= here or not.

Comment 17

[John Bandhauer]

The XXX comment still applies. The OBJ_DROP_PROPERTY bit is just to make my 
hack less evil. I'm sure Brendan is planning a better fix for that bug.

Comment 18

[Brendan Eich [:brendan]]

I've got a revised js_SetProperty patch nearly done, too tired to push it now.

Thanks, jst; more review the merrier.  I think I'll wait for shaver's r/sr= here
too, he loves this code.

/be

Comment 19

[Brendan Eich [:brendan]]

See bug 72354 for the further evolution of jsobj.c.  The last patch here is fine
for fixing this bug, and I truly hope and wish that shaver will bless it, either
here or in the context of 72354's larger patch, which includes it.

/be

Comment 20

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 21

[Phil Schwartau]

Marking Verified -