Changeset 18786 in webkit


Ignore:
Timestamp:
Jan 11, 2007, 4:46:36 PM (19 years ago)
Author:
ggaren
Message:

Reviewed by Anders Carlsson.

Even more cleanup in preparation for fixing <rdar://problem/4608404>
WebScriptObject's _executionContext lack of ownership policy causes
crashes (e.g., in Dashcode)


Layout tests pass.


Renames:

ReferencesSet | ProtectCounts => ProtectCountSet (because it's a typename for a set of GC protect counts)
ReferencesByRootMap => RootObjectMap (because RootObjectToProtectCountSetMap would have been confusing)
pv => protectedValues
rootObjectForImp => getRootObject (overloaded for JSObject* and Interpreter*)
rootObjectForInterpreter => getRootObject (ditto)
findReferenceSet => getProtectCountSet
imp => jsObject


(KJS::Bindings::getRootObjectMap): Changed to take advantage of built-in
facility for initializing static variables.

(KJS::Bindings::getProtectCountSet):
(KJS::Bindings::destroyProtectCountSet): Added. Helps encapsulate the fact
that getting a ProtectCountSet entails adding a RootObject to a hash table,
and destroying one entails the reverse.

(KJS::Bindings::getRootObject): Removed spurious NULL check.


(KJS::Bindings::findReferenceSet): Renamed. Changed to use getRootObject()
instead of iterating on its own.

(KJS::Bindings::addNativeReference): Changed to use an early return instead
of indenting the whole function.
(KJS::Bindings::removeNativeReference): Ditto.

Location:
trunk/JavaScriptCore
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/JavaScriptCore/ChangeLog

    r18782 r18786  
     12007-01-11  Geoffrey Garen  <[email protected]>
     2
     3        Reviewed by Anders Carlsson.
     4
     5        Even more cleanup in preparation for fixing <rdar://problem/4608404>
     6        WebScriptObject's _executionContext lack of ownership policy causes
     7        crashes (e.g., in Dashcode)
     8       
     9        Layout tests pass.
     10       
     11        Renames:
     12            ReferencesSet | ProtectCounts => ProtectCountSet (because it's a typename for a set of GC protect counts)
     13            ReferencesByRootMap => RootObjectMap (because RootObjectToProtectCountSetMap would have been confusing)
     14            pv => protectedValues
     15            rootObjectForImp => getRootObject (overloaded for JSObject* and Interpreter*)
     16            rootObjectForInterpreter => getRootObject (ditto)
     17            findReferenceSet => getProtectCountSet
     18            imp => jsObject
     19       
     20        (KJS::Bindings::getRootObjectMap): Changed to take advantage of built-in
     21        facility for initializing static variables.
     22
     23        (KJS::Bindings::getProtectCountSet):
     24        (KJS::Bindings::destroyProtectCountSet): Added. Helps encapsulate the fact
     25        that getting a ProtectCountSet entails adding a RootObject to a hash table,
     26        and destroying one entails the reverse.
     27
     28        (KJS::Bindings::getRootObject): Removed spurious NULL check.
     29       
     30        (KJS::Bindings::findReferenceSet): Renamed. Changed to use getRootObject()
     31        instead of iterating on its own.
     32
     33        (KJS::Bindings::addNativeReference): Changed to use an early return instead
     34        of indenting the whole function.
     35        (KJS::Bindings::removeNativeReference): Ditto.
     36
    1372007-01-11  Geoffrey Garen  <[email protected]>
    238

  • trunk/JavaScriptCore/bindings/c/c_utility.cpp

    r18481 r18786  
    127127        } else {
    128128            Interpreter *originInterpreter = exec->dynamicInterpreter();
    129             const Bindings::RootObject* originRootObject = rootObjectForInterpreter(originInterpreter);
     129            const Bindings::RootObject* originRootObject = getRootObject(originInterpreter);
    130130
    131131            Interpreter *interpreter = 0;
    … …  
    137137                interpreter = originInterpreter;
    138138
    139             const RootObject* rootObject = rootObjectForInterpreter(interpreter);
     139            const RootObject* rootObject = getRootObject(interpreter);
    140140            if (!rootObject) {
    141141                RootObject* newRootObject = new RootObject(0, interpreter);

  • trunk/JavaScriptCore/bindings/jni/jni_jsobject.cpp

    r18782 r18786  
    8080        else {
    8181            JSObject *imp = jlong_to_impptr(nativeHandle);
    82             if (!rootObjectForImp(imp)) {
     82            if (!getRootObject(imp)) {
    8383                fprintf (stderr, "%s:%d:  Attempt to access JavaScript from destroyed applet, type %d.\n", __FILE__, __LINE__, context->type);
    8484                return result;
    … …  
    128128                case Finalize: {
    129129                    JSObject *imp = jlong_to_impptr(nativeHandle);
    130                     if (findReferenceSet(imp) == 0) {
     130                    if (!getProtectCountSet(imp)) {
    131131                        // We may have received a finalize method call from the VM
    132132                        // AFTER removing our last reference to the Java instance.
    … …  
    159159    assert (_imp != 0);
    160160   
    161     _rootObject = rootObjectForImp(_imp);
     161    _rootObject = getRootObject(_imp);
    162162   
    163163    // If we can't find the root for the object something is terribly wrong.
    … …  
    301301        return nativeHandle;
    302302
    303     if (rootObjectForImp(jlong_to_impptr(nativeHandle)))
     303    if (getRootObject(jlong_to_impptr(nativeHandle)))
    304304        return nativeHandle;
    305305

  • trunk/JavaScriptCore/bindings/objc/objc_runtime.mm

    r18481 r18786  
    142142static id convertValueToObjcObject(ExecState* exec, JSValue* value)
    143143{
    144     const RootObject* rootObject = rootObjectForInterpreter(exec->dynamicInterpreter());
     144    const RootObject* rootObject = getRootObject(exec->dynamicInterpreter());
    145145    if (!rootObject) {
    146146        RootObject* newRootObject = new RootObject(0, exec->dynamicInterpreter());

  • trunk/JavaScriptCore/bindings/objc/objc_utility.mm

    r18481 r18786  
    137137        case ObjcObjectType: {
    138138            Interpreter *originInterpreter = exec->dynamicInterpreter();
    139             const RootObject* originRootObject = rootObjectForInterpreter(originInterpreter);
     139            const RootObject* originRootObject = getRootObject(originInterpreter);
    140140
    141141            Interpreter *interpreter = 0;
    … …  
    146146                interpreter = originInterpreter;
    147147               
    148             const RootObject* rootObject = rootObjectForInterpreter(interpreter);
     148            const RootObject* rootObject = getRootObject(interpreter);
    149149            if (!rootObject) {
    150150                RootObject* newRootObject = new RootObject(0, interpreter);

  • trunk/JavaScriptCore/bindings/runtime_root.cpp

    r18782 r18786  
    4545// are represented by a jlong.
    4646
    47 typedef HashMap<const RootObject*, ReferencesSet*> ReferencesByRootMap;
    48 
    49 static ReferencesByRootMap* getReferencesByRootMap()
    50 {
    51     static ReferencesByRootMap* referencesByRootMap = 0;
    52    
    53     if (!referencesByRootMap)
    54         referencesByRootMap = new ReferencesByRootMap;
    55    
    56     return referencesByRootMap;
    57 }
    58 
    59 static ReferencesSet* getReferencesSet(const RootObject* rootObject)
    60 {
    61     ReferencesByRootMap* refsByRoot = getReferencesByRootMap();
    62     ReferencesSet* referencesSet = 0;
    63    
    64     referencesSet = refsByRoot->get(rootObject);
    65     if (!referencesSet) {
    66         referencesSet  = new ReferencesSet;
    67         refsByRoot->add(rootObject, referencesSet);
    68     }
    69     return referencesSet;
    70 }
    71 
    72 // Scan all the dictionary for all the roots to see if any have a
    73 // reference to the imp, and if so, return it's reference count
    74 // dictionary.
    75 // FIXME:  This is a potential performance bottleneck with many applets.  We could fix be adding a
    76 // imp to root dictionary.
    77 ReferencesSet* findReferenceSet(JSObject* imp)
    78 {
    79     ReferencesByRootMap* refsByRoot = getReferencesByRootMap ();
    80     if (refsByRoot) {
    81         ReferencesByRootMap::const_iterator end = refsByRoot->end();
    82         for (ReferencesByRootMap::const_iterator it = refsByRoot->begin(); it != end; ++it) {
    83             ReferencesSet* set = it->second;
    84            
    85             if (set->contains(imp))
    86                 return set;
    87         }
     47typedef HashMap<const RootObject*, ProtectCountSet*> RootObjectMap;
     48
     49static RootObjectMap* getRootObjectMap()
     50{
     51    static RootObjectMap staticRootObjectMap;
     52    return &staticRootObjectMap;
     53}
     54
     55static ProtectCountSet* getProtectCountSet(const RootObject* rootObject)
     56{
     57    RootObjectMap* rootObjectMap = getRootObjectMap();
     58    ProtectCountSet* protectCountSet = rootObjectMap->get(rootObject);
     59
     60    if (!protectCountSet) {
     61        protectCountSet = new ProtectCountSet;
     62        rootObjectMap->add(rootObject, protectCountSet);
     63    }
     64    return protectCountSet;
     65}
     66
     67static void destroyProtectCountSet(const RootObject* rootObject, ProtectCountSet* protectCountSet)
     68{
     69    RootObjectMap* rootObjectMap = getRootObjectMap();
     70    rootObjectMap->remove(rootObject);
     71    delete protectCountSet;
     72}
     73
     74// FIXME:  These two functions are a potential performance problem.  We could
     75// fix them by adding a JSObject to RootObject dictionary.
     76
     77ProtectCountSet* getProtectCountSet(JSObject* jsObject)
     78{
     79    const RootObject* rootObject = getRootObject(jsObject);
     80    return getProtectCountSet(rootObject);
     81}
     82
     83const RootObject* getRootObject(JSObject* jsObject)
     84{
     85    RootObjectMap* rooObjectMap = getRootObjectMap();
     86   
     87    RootObjectMap::const_iterator end = rooObjectMap->end();
     88    for (RootObjectMap::const_iterator it = rooObjectMap->begin(); it != end; ++it) {
     89        ProtectCountSet* set = it->second;
     90        if (set->contains(jsObject))
     91            return it->first;
    8892    }
    8993   
    … …  
    9195}
    9296
    93 // FIXME:  This is a potential performance bottleneck with many applets.  We could fix be adding a
    94 // imp to root dictionary.
    95 const RootObject* rootObjectForImp (JSObject* imp)
    96 {
    97     ReferencesByRootMap* refsByRoot = getReferencesByRootMap ();
    98     const RootObject* rootObject = 0;
    99    
    100     if (refsByRoot) {
    101         ReferencesByRootMap::const_iterator end = refsByRoot->end();
    102         for (ReferencesByRootMap::const_iterator it = refsByRoot->begin(); it != end; ++it) {
    103             ReferencesSet* set = it->second;
    104             if (set->contains(imp)) {
    105                 rootObject = it->first;
    106                 break;
    107             }
    108         }
    109     }
    110    
    111     return rootObject;
    112 }
    113 
    114 const RootObject* rootObjectForInterpreter(Interpreter* interpreter)
    115 {
    116     ReferencesByRootMap* refsByRoot = getReferencesByRootMap ();
    117    
    118     if (refsByRoot) {
    119         ReferencesByRootMap::const_iterator end = refsByRoot->end();
    120         for (ReferencesByRootMap::const_iterator it = refsByRoot->begin(); it != end; ++it) {
     97const RootObject* getRootObject(Interpreter* interpreter)
     98{
     99    RootObjectMap* rooObjectMap = getRootObjectMap();
     100   
     101    if (rooObjectMap) {
     102        RootObjectMap::const_iterator end = rooObjectMap->end();
     103        for (RootObjectMap::const_iterator it = rooObjectMap->begin(); it != end; ++it) {
    121104            const RootObject* aRootObject = it->first;
    122105           
    … …  
    129112}
    130113
    131 void addNativeReference(const RootObject* rootObject, JSObject* imp)
    132 {
    133     if (rootObject) {
    134         ReferencesSet* referenceMap = getReferencesSet(rootObject);
    135        
    136         unsigned numReferences = referenceMap->count(imp);
    137         if (numReferences == 0) {
    138             JSLock lock;
    139             gcProtect(imp);
    140         }
    141         referenceMap->add(imp);
    142     }
    143 }
    144 
    145 void removeNativeReference(JSObject* imp)
    146 {
    147     if (!imp)
     114void addNativeReference(const RootObject* rootObject, JSObject* jsObject)
     115{
     116    if (!rootObject)
    148117        return;
    149 
    150     ReferencesSet *referencesSet = findReferenceSet(imp);
    151     if (referencesSet) {
    152         unsigned numReferences = referencesSet->count(imp);
    153        
    154         if (numReferences == 1) {
    155             JSLock lock;
    156             gcUnprotect(imp);
    157         }
    158         referencesSet->remove(imp);
    159     }
     118   
     119    ProtectCountSet* protectCountSet = getProtectCountSet(rootObject);
     120    if (!protectCountSet->contains(jsObject)) {
     121        JSLock lock;
     122        gcProtect(jsObject);
     123    }
     124    protectCountSet->add(jsObject);
     125}
     126
     127void removeNativeReference(JSObject* jsObject)
     128{
     129    if (!jsObject)
     130        return;
     131
     132    ProtectCountSet* protectCountSet = getProtectCountSet(jsObject);
     133    if (protectCountSet->count(jsObject) == 1) {
     134        JSLock lock;
     135        gcUnprotect(jsObject);
     136    }
     137    protectCountSet->remove(jsObject);
    160138}
    161139
    … …  
    287265void RootObject::destroy()
    288266{
    289     ReferencesSet* referencesSet = getReferencesSet(this);
    290    
    291     if (referencesSet) {
    292         ReferencesSet::iterator end = referencesSet->end();
    293         for (ReferencesSet::iterator it = referencesSet->begin(); it != end; ++it) {
     267    ProtectCountSet* protectCountSet = getProtectCountSet(this);
     268   
     269    if (protectCountSet) {
     270        ProtectCountSet::iterator end = protectCountSet->end();
     271        for (ProtectCountSet::iterator it = protectCountSet->begin(); it != end; ++it) {
    294272            JSLock lock;
    295273            gcUnprotect(it->first);           
    296274        }
    297         referencesSet->clear();
    298         ReferencesByRootMap* refsByRoot = getReferencesByRootMap();
    299         refsByRoot->remove(this);
    300         delete referencesSet;
     275
     276        destroyProtectCountSet(this, protectCountSet);
    301277    }
    302278

  • trunk/JavaScriptCore/bindings/runtime_root.h

    r18782 r18786  
    4040
    4141typedef RootObject* (*CreateRootObjectFunction)(void* nativeHandle);
    42 typedef HashCountedSet<JSObject*> ReferencesSet;
     42typedef HashCountedSet<JSObject*> ProtectCountSet;
    4343
    44 extern ReferencesSet* findReferenceSet(JSObject *imp);
    45 extern const RootObject* rootObjectForImp(JSObject*);
    46 extern const RootObject* rootObjectForInterpreter(Interpreter*);
    47 extern void addNativeReference (const RootObject *root, JSObject *imp);
    48 extern void removeNativeReference (JSObject *imp);
     44extern ProtectCountSet* getProtectCountSet(JSObject*);
     45extern const RootObject* getRootObject(JSObject*);
     46extern const RootObject* getRootObject(Interpreter*);
     47extern void addNativeReference(const RootObject*, JSObject*);
     48extern void removeNativeReference(JSObject*);
    4949
    5050class RootObject

  • trunk/JavaScriptCore/kjs/collector.cpp

    r17566 r18786  
    418418}
    419419
    420 typedef HashCountedSet<JSCell *> ProtectCounts;
    421 
    422 static ProtectCounts& protectedValues()
    423 {
    424     static ProtectCounts pv;
    425     return pv;
     420typedef HashCountedSet<JSCell*> ProtectCountSet;
     421
     422static ProtectCountSet& protectedValues()
     423{
     424    static ProtectCountSet staticProtectCountSet;
     425    return staticProtectCountSet;
    426426}
    427427
    … …  
    450450void Collector::markProtectedObjects()
    451451{
    452   ProtectCounts& pv = protectedValues();
    453   ProtectCounts::iterator end = pv.end();
    454   for (ProtectCounts::iterator it = pv.begin(); it != end; ++it) {
     452  ProtectCountSet& protectedValues = KJS::protectedValues();
     453  ProtectCountSet::iterator end = protectedValues.end();
     454  for (ProtectCountSet::iterator it = protectedValues.begin(); it != end; ++it) {
    455455    JSCell *val = it->first;
    456456    if (!val->marked())
    … …  
    670670    HashCountedSet<const char*>* counts = new HashCountedSet<const char*>;
    671671
    672     ProtectCounts& pv = protectedValues();
    673     ProtectCounts::iterator end = pv.end();
    674     for (ProtectCounts::iterator it = pv.begin(); it != end; ++it)
     672    ProtectCountSet& protectedValues = KJS::protectedValues();
     673    ProtectCountSet::iterator end = protectedValues.end();
     674    for (ProtectCountSet::iterator it = protectedValues.begin(); it != end; ++it)
    675675        counts->add(typeName(it->first));
    676676
Note: See TracChangeset for help on using the changeset viewer.