112152 - (xpcthreadsafety) Cleanup of nsXPCWrappedJS needs some additional thread safety measures

Status: RESOLVED INVALID

(Core :: XPConnect, defect, P3)

Description

[David Bradley]

There are cases where destructed or partially destructed nsXPCWrappedJS
instances have been returned by nsXPCWrappedJS::GetNewOrUsed. Also they have
ended up in the dying array.

Comment 1

[David Bradley]

This is going to take some effort to get right. So given the amount work, risk,
etc. moving out past 1.0. 

So in summary the system that manages the nsXPCWrappedJS instances need to be
reworked and made thread save.

Inserting jbands email that sums things up pretty well: 
I thought about this a little more yesterday - but did not write any code. I
kept coming up with sub-areas that are a mess. For instance, we don't lock
around traversals of the wrapper chain - e.g. in QI. This is bad if we are to
continue unlinking and deleting and wrappers in their release methods.

I think the double refcount stuff will go. The weakref support needs to be
managed differently. The JS rooting ought to go. As I said before, explicit
marking instead. The delete in the release call ought to go. This can happen in
the gccallback stuff. The locking on the map lock I proposed will change because
the objects will never be destoryed exceptin in gc. So, the GetNewOrUsed call
(with an active callcontext) will be able to assume that the root can;t die. I'm
coming to believe that we can not safely do this all without using monitor
notify stuff. There is the period betwen the time we mark and the time we detect
which objects will be collected when we can't reasonably hold a lock, but also
can not tell any caller with certainty whether the  wrapper will be valid or
not. I think we'll end up (potentially) blocking while waiting for notification
at that point before succeeding or failing for addref, queryReferent, and
(perhaps) QI calls.

Before it gets past the point of detecting all the wrappers that will be dying,
it will remove them from the maps to make them non-findable.

In case you didn't notice, I'm thinking to likely keep alive all wrappers whose
JSObjects are alive - even when their refcounts go to zero and thy *don't* have
outstanding weakrefs. This may or may not be worthwhile. We can go eitherway on
that point by implmennting our own weakref getter (with locking) rather than
using the mixin.

It is going be be necessary to have some additional test scenerios. The one
locking test we have does not do any:

- weakref stuff
- multiple wrappers per chain
- aggregated to native

We should not get too fancy here without writing tests that cover the cases.

Nevertheless, this is looking bigger and messier (to me) than we originally
thought. I'm thinking that since we are not likely to see this as a problem in
the products at hand, the priority is lower compared to some other things on
your list.

Comment 2

[David Bradley]

After further discussion it was decided 1.0.1 makes more sense as a post 1.0
milestone.

Comment 3

[Brendan Eich [:brendan]]

jband: s/monitor notify stuff/condvar notify stuff/

/be

Comment 4

[David Bradley]

Retargetting to the proper post 1.0 milestone

Comment 5

[David Bradley]

Moving out to 1.3. If this needs to be in before 1.3 please comment.

Comment 6

[David Bradley]

Moving to 1.4 Alpha

Comment 7

[David Bradley]

Moving out

Comment 8

[David Bradley]

Moving out

Comment 9

[David Bradley]

Moving out

Comment 10

[timeless]

*** Bug 236406 has been marked as a duplicate of this bug. ***

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm going to say this is not valid any more given that XPConnect is not threadsafe.