Ignore:
Timestamp:
Jan 26, 2007, 8:50:17 PM (18 years ago)
Author:
ggaren
Message:

JavaScriptCore:

Reviewed by Maciej Stachowiak.


Fixed <rdar://problem/4608404> WebScriptObject's _rootObject lack
of ownership policy causes crashes (e.g., in Dashcode)


The old model for RootObject ownership was either to (1) leak them or (2) assign
them to a single owner -- the WebCore::Frame -- which would destroy them
when it believed that all of its plug-ins had unloaded.


This model was broken because of (1) and also because plug-ins are not the only
RootObject clients. All Bindings clients are RootObjects clients, including
applications, which outlive any particular WebCore::Frame.


The new model for RootObject ownership is to reference-count them, with a
throw-back to the old model: The WebCore::Frame tracks the RootObjects
it creates, and invalidates them when it believes that all of its plug-ins
have unloaded.


We maintain this throw-back to avoid plug-in leaks, particularly from Java.
Java is completely broken when it comes to releasing JavaScript objects.
Comments in our code allege that Java does not always call finalize when
collecting objects. Moreoever, my own testing reveals that, when Java does
notify JavaScript of a finalize, the data it provides is totally bogus.


This setup is far from ideal, but I don't think we can do better without
completely rewriting the bindings code, and possibly part of the Java
plug-in / VM.


Layout tests pass. No additional leaks reported. WebCore/manual-tests/*liveconnect*
and a few LiveConnect demos on the web also run without a hitch.


const RootObject* => RootObject*, since we need to ref/deref


  • bindings/NP_jsobject.cpp: (jsDeallocate): deref our RootObjects. Also unprotect or JSObject, instead of just relying on the RootObject to do it for us when it's invalidated. (_isSafeScript): Check RootObject validity. (_NPN_CreateScriptObject): ditto (_NPN_Invoke): ditto (_NPN_Evaluate): ditto (_NPN_GetProperty): ditto (_NPN_SetProperty): ditto (_NPN_RemoveProperty): ditto (_NPN_HasProperty): ditto (_NPN_HasMethod): ditto (_NPN_SetException): ditto
  • bindings/runtime_root.cpp: Revived bit-rotted LIAR LIAR LIAR comment.


LOOK: Added support for invalidating RootObjects without deleting them,
which is the main goal of this patch.

Moved protect counting into the RootObject class, to emphasize that
the RootObject protects the JSObject, and unprotects it upon being invalidated.

addNativeReference => RootObject::gcProtect
removeNativeReference => RootObject::gcUnprotect
ProtectCountSet::contains => RootObject::gcIsProtected


I know we'll all be sad to see the word "native" go.


  • bindings/runtime_root.h: Added ref-counting support to RootObject, with all the standard accoutrements.
  • bindings/c/c_utility.cpp: (KJS::Bindings::convertValueToNPVariant): If we can't find a valid RootObject, return void instead of just leaking.
  • bindings/jni/jni_instance.cpp: (JavaInstance::JavaInstance): Don't take a RootObject in our constructor; be like other Instances and require the caller to call setRootObject. This reduces the number of ownership code paths. (JavaInstance::invokeMethod): Check RootObject for validity.
  • bindings/jni/jni_instance.h: Removed private no-arg constructor. Having an arg constructor accomplishes the same thing.
  • bindings/jni/jni_jsobject.cpp: (JavaJSObject::invoke): No need to call findProtectCountSet, because finalize() checks for RootObject validity. (JavaJSObject::JavaJSObject): check RootObject for validity (JavaJSObject::call): ditto (JavaJSObject::eval): ditto (JavaJSObject::getMember): ditto (JavaJSObject::setMember): ditto (JavaJSObject::removeMember): ditto (JavaJSObject::getSlot): ditto (JavaJSObject::setSlot): ditto (JavaJSObject::toString): ditto (JavaJSObject::finalize): ditto (JavaJSObject::createNative): No need to tell the RootObject to protect the global object, since the RootObject already owns the interpreter.
  • bindings/jni/jni_runtime.cpp: (JavaArray::JavaArray): Removed copy construcutor becaue it was unused. Dead code is dangerous code.
  • bindings/objc/objc_runtime.mm: Added WebUndefined protocol. Previous use of WebScriptObject was bogus, because WebUndefined is not a subclass of WebScriptObject. (convertValueToObjcObject): If we can't find a valid RootObject, return nil instead of just leaking.
  • bindings/objc/objc_utility.mm: (KJS::Bindings::convertValueToObjcValue): If we can't find a valid RootObject, return nil instead of just leaking.

LayoutTests:

Reviewed by Maciej Stachowiak.


Added test for <rdar://problem/4608404> WebScriptObject's _rootObject lack
of ownership policy causes crashes (e.g., in Dashcode)


No test for Java or NPP versions of this bug because there's no reliable way to
make Java and NPP objects outlive their RootObjects (although Java objects
sometimes do).

  • plugins/root-object-premature-delete-crash-expected.txt: Added.
  • plugins/root-object-premature-delete-crash.html: Added.

WebCore:

Reviewed by Maciej Stachowiak.


Fixed <rdar://problem/4608404> WebScriptObject's _executionContext lack
of ownership policy causes crashes (e.g., in Dashcode)

Added RootObject ref-counting goodness.

  • page/mac/FrameMac.h:
  • page/mac/FrameMac.mm: (WebCore::FrameMac::cleanupPluginObjects): Invalidate our RootObjects instead of detroying them. Track _bindingRootObject separately from the rest of our RootObjects, since it has its own variable.
  • page/mac/WebCoreFrameBridge.mm: (createRootObject): Use the Frame's new, more encapsulated function to create a RootObject.
  • bindings/objc/WebScriptObject.mm: Nixed rootObject setters, since they were unused and they complicated reference-counting.

WebKitTools:

Reviewed by Maciej Stachowiak.


Added support for test for <rdar://problem/4608404> WebScriptObject's
_rootObject lack of ownership policy causes crashes (e.g., in Dashcode)


  • DumpRenderTree/DumpRenderTree.m: (+[LayoutTestController isSelectorExcludedFromWebScript:]): (+[LayoutTestController webScriptNameForSelector:]): (-[LayoutTestController storeWebScriptObject:]): (-[LayoutTestController accessStoredWebScriptObject]): (-[LayoutTestController dealloc]):
File:
1 edited

Legend:

Unmodified
Added
Removed

  • trunk/JavaScriptCore/bindings/NP_jsobject.cpp

    r18461 r19183  
    4949}
    5050
    51 static void jsDeallocate(NPObject* obj)
    52 {
     51static void jsDeallocate(NPObject* npObj)
     52{
     53    JavaScriptObject* obj = (JavaScriptObject*)npObj;
     54
     55    if (obj->rootObject && obj->rootObject->isValid())
     56        obj->rootObject->gcUnprotect(obj->imp);
     57
     58    if (obj->rootObject)
     59        obj->rootObject->deref();
     60
     61    if (obj->originRootObject)
     62        obj->originRootObject->deref();
     63
    5364    free(obj);
    5465}
    … …  
    6273static bool _isSafeScript(JavaScriptObject* obj)
    6374{
    64     if (obj->originRootObject) {
    65         Interpreter* originInterpreter = obj->originRootObject->interpreter();
    66         if (originInterpreter)
    67             return originInterpreter->isSafeScript(obj->rootObject->interpreter());
    68     }
    69     return true;
    70 }
    71 
    72 NPObject* _NPN_CreateScriptObject (NPP npp, JSObject* imp, const RootObject* originRootObject, const RootObject* rootObject)
     75    if (!obj->originRootObject || !obj->rootObject)
     76        return true;
     77   
     78    if (!obj->originRootObject->isValid() || !obj->rootObject->isValid())
     79        return false;
     80       
     81    return obj->originRootObject->interpreter()->isSafeScript(obj->rootObject->interpreter());
     82}
     83
     84NPObject* _NPN_CreateScriptObject(NPP npp, JSObject* imp, PassRefPtr<RootObject> originRootObject, PassRefPtr<RootObject> rootObject)
    7385{
    7486    JavaScriptObject* obj = (JavaScriptObject*)_NPN_CreateObject(npp, NPScriptObjectClass);
    7587
     88    obj->originRootObject = originRootObject.releaseRef();
     89    obj->rootObject = rootObject.releaseRef();
     90
     91    if (obj->rootObject)
     92        obj->rootObject->gcProtect(imp);
    7693    obj->imp = imp;
    77     obj->originRootObject = originRootObject;   
    78     obj->rootObject = rootObject;   
    79 
    80     addNativeReference(rootObject, imp);
    81 
    82     return (NPObject *)obj;
     94
     95    return (NPObject*)obj;
    8396}
    8497
    … …  
    120133
    121134        // Lookup the function object.
    122         ExecState* exec = obj->rootObject->interpreter()->globalExec();
     135        RootObject* rootObject = obj->rootObject;
     136        if (!rootObject || !rootObject->isValid())
     137            return false;
     138
     139        ExecState* exec = rootObject->interpreter()->globalExec();
    123140        JSLock lock;
    124141        JSValue* func = obj->imp->get(exec, identifierFromNPIdentifier(i->value.string));
    … …  
    157174            return false;
    158175
    159         ExecState* exec = obj->rootObject->interpreter()->globalExec();
     176        RootObject* rootObject = obj->rootObject;
     177        if (!rootObject || !rootObject->isValid())
     178            return false;
     179
     180        ExecState* exec = rootObject->interpreter()->globalExec();
    160181       
    161182        JSLock lock;
    … …  
    163184        unsigned int UTF16Length;
    164185        convertNPStringToUTF16(s, &scriptString, &UTF16Length); // requires free() of returned memory
    165         Completion completion = obj->rootObject->interpreter()->evaluate(UString(), 0, UString((const UChar*)scriptString,UTF16Length));
     186        Completion completion = rootObject->interpreter()->evaluate(UString(), 0, UString((const UChar*)scriptString,UTF16Length));
    166187        ComplType type = completion.complType();
    167188       
    … …  
    192213            return false;
    193214
    194         ExecState* exec = obj->rootObject->interpreter()->globalExec();
     215        RootObject* rootObject = obj->rootObject;
     216        if (!rootObject || !rootObject->isValid())
     217            return false;
     218
     219        ExecState* exec = rootObject->interpreter()->globalExec();
    195220        PrivateIdentifier* i = (PrivateIdentifier*)propertyName;
    196221       
    … …  
    231256            return false;
    232257
    233         ExecState* exec = obj->rootObject->interpreter()->globalExec();
     258        RootObject* rootObject = obj->rootObject;
     259        if (!rootObject || !rootObject->isValid())
     260            return false;
     261
     262        ExecState* exec = rootObject->interpreter()->globalExec();
    234263        JSLock lock;
    235264        PrivateIdentifier* i = (PrivateIdentifier*)propertyName;
    … …  
    254283            return false;
    255284
    256         ExecState* exec = obj->rootObject->interpreter()->globalExec();
     285        RootObject* rootObject = obj->rootObject;
     286        if (!rootObject || !rootObject->isValid())
     287            return false;
     288
     289        ExecState* exec = rootObject->interpreter()->globalExec();
    257290        PrivateIdentifier* i = (PrivateIdentifier*)propertyName;
    258291        if (i->isString) {
    … …  
    282315            return false;
    283316
    284         ExecState* exec = obj->rootObject->interpreter()->globalExec();
     317        RootObject* rootObject = obj->rootObject;
     318        if (!rootObject || !rootObject->isValid())
     319            return false;
     320
     321        ExecState* exec = rootObject->interpreter()->globalExec();
    285322        PrivateIdentifier* i = (PrivateIdentifier*)propertyName;
    286323        JSLock lock;
    … …  
    307344            return false;
    308345
    309         ExecState* exec = obj->rootObject->interpreter()->globalExec();
     346        RootObject* rootObject = obj->rootObject;
     347        if (!rootObject || !rootObject->isValid())
     348            return false;
     349
     350        ExecState* exec = rootObject->interpreter()->globalExec();
    310351        JSLock lock;
    311352        JSValue* func = obj->imp->get(exec, identifierFromNPIdentifier(i->value.string));
    … …  
    323364    if (o->_class == NPScriptObjectClass) {
    324365        JavaScriptObject* obj = (JavaScriptObject*)o;
    325         ExecState* exec = obj->rootObject->interpreter()->globalExec();
     366        RootObject* rootObject = obj->rootObject;
     367        if (!rootObject || !rootObject->isValid())
     368            return;
     369
     370        ExecState* exec = rootObject->interpreter()->globalExec();
    326371        JSLock lock;
    327372        throwError(exec, GeneralError, message);
Note: See TracChangeset for help on using the changeset viewer.