call to DefineProperty w/ getter or setter recurs in Resolver

VERIFIED FIXED in mozilla0.9.1

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: John Bandhauer, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla0.9.1
js1.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
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.
(Assignee)

Comment 1

18 years ago
Yeah, recursion in resolve has been a thorn in people's sides for too long. I
remember bing@netscape.com (long departed) hacking around it in the plugins and
mimeTypes reflections in 4.x.  I'll try to address it centrally.

/be
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla0.9
Priority: -- → P2
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 2

17 years ago
Created attachment 30392 [details] [diff] [review]
proposed fix
(Assignee)

Comment 3

17 years ago
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
(Assignee)

Updated

17 years ago
Keywords: patch, review
(Assignee)

Comment 4

17 years ago
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
(Reporter)

Comment 5

17 years ago
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?
(Assignee)

Comment 6

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

/be
(Assignee)

Comment 7

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

/be
Blocks: 72354
Keywords: mozilla0.9 → mozilla0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Comment 8

17 years ago
Created attachment 34080 [details] [diff] [review]
better fix (also patches the xpcdom js_SetProperty hack to not leak)
(Assignee)

Comment 9

17 years ago
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
(Reporter)

Comment 10

17 years ago
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!
(Assignee)

Comment 11

17 years ago
Created attachment 34119 [details] [diff] [review]
better patch, tighten up non-native object resolve logic
(Reporter)

Comment 12

17 years ago
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.
(Assignee)

Comment 13

17 years ago
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
(Assignee)

Comment 14

17 years ago
Created attachment 34130 [details] [diff] [review]
best patch yet -- final one for this bug, I hope
(Reporter)

Comment 15

17 years ago
r/sr=jband. looks good. worksforme.
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.
(Reporter)

Comment 17

17 years ago
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.
(Assignee)

Comment 18

17 years ago
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
(Assignee)

Comment 19

17 years ago
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
(Assignee)

Comment 20

17 years ago
Fixed.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 21

17 years ago
Marking Verified - 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.