14462 - JS getter/setter callouts not foolproof or fully threadsafe

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Brendan Eich [:brendan]]

1.  Now that JS users can implement property getters and setters, the #ifdef
JS_THREADSAFE code in jsobj.c around SPROP_GET and SPROP_SET callouts should be
unifdef'd -- single-threaded embeddings must not crash if a luser deletes the
property being got from within its getter function.

2.  The callouts from property cache hit code in jsinterp.c have always been
done without addref'ing the JSScopeProperty.  This is thread-unsafe even without
user-defined getters and setters.

/be

Comment 1

[Brendan Eich [:brendan]]

Not an M13 stopper; will do early in M14.

/be

Comment 2

[Brendan Eich [:brendan]]

Let's try that milestone move again.

/be

Comment 3

[leger]

Setting all Javacript bugs to rginda QA Contact.

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix, with a few extra nits picked

Comment 5

[Brendan Eich [:brendan]]

In order, changes in the patch are:

- Rename JSSLOT_ITR_STATE to be JSSLOT_ITER_STATE (avoid cybercrud abbreviation 
as cbrcrd, no more six-char id limits!).

- Property cache tests must occur with the object's scope-lock held, to close a 
race with delete (js_DestroyProperty, always called with the property's scope 
locked).  Once the cache has been hit, and before the lock is released, the 
property's refcount must be bumped.  This requires re-acquisition of the lock 
and js_DropScopeProperty afterward.

- Reworked js_FindProperty to use a do-while loop, as cx->fp->scopeChain must be 
non-null.  This avoids a gratuitous lastobj init done to "Suppress gcc warning" 
in the old revision.

- Akin to the property cache hit cases in jsinterp.c and jsobj.c's 
js_FindProperty, code to hold and drop the scope-property by its refcount that 
was #ifdef JS_THREADSAFE must be unconditional, now that user-defined getters 
and setters may delete the property id they're getting or setting.

- Fixed overlong continuation line in jsobj.h.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: revised jsinterp.c patch, axe debugger hook "thread safety"

Comment 7

[Brendan Eich [:brendan]]

Adding jband -- the most recent patch, for jsinterp.c alone (it replaces that 
file's part of the original patch), removes what seems to me to be inadequate 
thread safety of debugger hook loading, via an extra re-loading step and null 
test.  Jband, can you say what the thread hazard is there?  I think for now it's 
best to just load and use such pointers, and their hook-data arguments.

/be

Comment 8

[Brendan Eich [:brendan]]

Rogerl has kindly offered to get this checked in, r=[e-mail].

/be

Comment 9

[John Bandhauer]

The debugger threadsafety issue was that the debugger might be setting and 
clearing hooks on some arbitrary other thread. What I didn't want to do was 
something like:

if(rt->throwHook) {
...
  rt->throwHook(...)
...
}

...because rt->throwHook could become null after the test and before the call. 
That is why I added the local copy of the pointer. The rule in JSD is that if 
you clear a hook the old hook address needs to be able to handle being called 
and fail gracefully if appropriate. You've preserved that local copy of the 
pointer. Which is good.

The double test that you did remove was just a (probably misguided) attempt to 
minimize the impact of having the hook when the hook is null (the normal case). 
I just wanted to avoid the store to local unless a hook was really present. But 
it was doing a second test to deal with the case where the hook might get 
cleared between the first test and the store. 
If you are willing to incur the potential cost of a rarely necessary store 
before the test then that's fine with me. I suppose that a good optimizing 
compiler could make the store only happen if the test succeeds - because the 
local is only using inside the 'if' block.

Nevertheless, I don't see any problem with the changes you made here.

Comment 10

[rogerl (gone)]

Brendan's fixes checked in.