Currently with the JSC Obj-C API, JS wrappers for client Obj-C objects retain their associated Obj-C object. When they are swept, they release their Obj-C objects which can trigger a call to that object's -dealloc method. These -dealloc methods can then call back into the same VM, which is not allowed during lazy sweeping or VM shutdown. We can gracefully handle this case by creating our own pool of Obj-C objects to be released when it is safe to do so. The basic idea is: "Scoped object for ObjC dealloc calls. Holds a Vector<RetainPtr>. Destructors use std::move to put things in this vector, instead of calling release directly. Scoped object does the right callback shim thingy. lastChanceToFinalize makes sure to use the scoped object."
<rdar://problem/15395983>
Created attachment 216976 [details] Patch
Comment on attachment 216976 [details] Patch Attachment 216976 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/23748057
Comment on attachment 216976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216976&action=review r=me, but I think you've got some fixup to do before landing. > Source/JavaScriptCore/API/ObjCCallbackFunction.mm:415 > [[m_invocation.get() target] release]; Do we need the same treatment for our invocation's target? What if that -dealloc did something unpleasant? > Source/JavaScriptCore/heap/DelayedReleaseScope.h:38 > + , m_savedScope(markedSpace.m_currentDelayedReleaseScope) I don't think we want to save the previous scope like this. If we did re-entrently create a DelayedReleaseScope (which I believe is impossible), it would not be correct to release those interior objects right away -- we would need to wait until we popped the outermost DelayedReleaseScope, or we would be violating its invariants. So, I guess we should ASSERT that the current scope is NULL, and then set ourselves as the current scope. > Source/JavaScriptCore/heap/DelayedReleaseScope.h:48 > + ~DelayedReleaseScope() > + { > + APICallbackShim callbackShim(*m_markedSpace.m_heap->vm()); > + m_delayedReleaseObjects.clear(); > + m_markedSpace.m_currentDelayedReleaseScope = m_savedScope; > + } Here, we need to unset the current scope right away, and not after we clear the vector. Otherwise, while the -dealloc methods run, we might accumulate new objects into m_delayedReleaseObjects, which we don't really want. > Source/JavaScriptCore/heap/Heap.h:190 > + template <typename T> > + void delayReleaseForObject(RetainPtr<T>&&); Let's call this "releaseSoon" or "releaseAfterGC".
Comment on attachment 216976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216976&action=review > Source/JavaScriptCore/API/APIShims.h:108 > + return 0; nullptr > Source/JavaScriptCore/API/APIShims.h:113 > + return 0; nullptr > Source/JavaScriptCore/API/APIShims.h:119 > + if (!vmForDropAllLocks(exec->vm())) Seems a bit peculiar to do it this way. Why not just a function that returns a boolean? > Source/JavaScriptCore/API/APIShims.h:120 > + return 0; nullptr > Source/JavaScriptCore/API/JSAPIWrapperObject.mm:59 > + RetainPtr<CFTypeRef> retainedObject(wrapperObject->wrappedObject()); > + JSC::Heap::heap(wrapperObject)->delayReleaseForObject(std::move(retainedObject)); > [static_cast<id>(wrapperObject->wrappedObject()) release]; Is the local variable really needed here? Instead of calling release here, I suggest we call adoptNS above. That will be faster. JSC::Heap::heap(wrapperObject)->delayReleaseForObject(adoptNS(wrapperObject->wrappedObject())); The one line would replace all three lines of code above. > Source/JavaScriptCore/API/ObjCCallbackFunction.mm:416 > + RetainPtr<CFTypeRef> target([m_invocation.get() target]); > [[m_invocation.get() target] release]; > + heap.delayReleaseForObject(std::move(target)); Same here: heap.delayReleaseForObject(adoptNS([m_invocation.get() target])); >> Source/JavaScriptCore/heap/DelayedReleaseScope.h:48 >> + ~DelayedReleaseScope() >> + { >> + APICallbackShim callbackShim(*m_markedSpace.m_heap->vm()); >> + m_delayedReleaseObjects.clear(); >> + m_markedSpace.m_currentDelayedReleaseScope = m_savedScope; >> + } > > Here, we need to unset the current scope right away, and not after we clear the vector. Otherwise, while the -dealloc methods run, we might accumulate new objects into m_delayedReleaseObjects, which we don't really want. I’d also suggest including: ASSERT(m_markedSpace.m_currentDelayedReleaseScope == this); > Source/JavaScriptCore/heap/DelayedReleaseScope.h:53 > + m_delayedReleaseObjects.append(reinterpret_cast<RetainPtr<CFTypeRef>&&>(object)); This reinterpret_cast is not a safe approach. We need to instead find a way to make this work with move constructors in the RetainPtr class. >> Source/JavaScriptCore/heap/Heap.h:190 >> + template <typename T> >> + void delayReleaseForObject(RetainPtr<T>&&); > > Let's call this "releaseSoon" or "releaseAfterGC". I think this would read better as a single line. > Source/JavaScriptCore/heap/MarkedSpace.cpp:85 > + , m_currentDelayedReleaseScope(0) nullptr > Source/JavaScriptCore/heap/MarkedSpace.h:119 > + template <typename T> > + void delayReleaseForObject(RetainPtr<T>&&); Ditto.
Comment on attachment 216976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216976&action=review >> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:415 >> [[m_invocation.get() target] release]; > > Do we need the same treatment for our invocation's target? What if that -dealloc did something unpleasant? We are delaying the release for the invocation's target. Do you mean the invocation itself? I think the only objects that the NSInvocation could release would be the target and the arguments. Since we're not using -retainArguments, those won't be dealloc-ed during the invocation's dealloc.
Comment on attachment 216976 [details] Patch Attachment 216976 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/22869444
Comment on attachment 216976 [details] Patch Attachment 216976 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/23698060
Comment on attachment 216976 [details] Patch Attachment 216976 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/23628066
Created attachment 216987 [details] Patch
Comment on attachment 216987 [details] Patch Attachment 216987 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/23618068
Created attachment 216988 [details] Patch
Comment on attachment 216988 [details] Patch Attachment 216988 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/23688068
Created attachment 216989 [details] Patch
Comment on attachment 216989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216989&action=review > Source/JavaScriptCore/API/APIShims.h:86 > + : m_dropAllLocks(shouldDropAllLocks(exec->vm()) ? exec : 0) nullptr > Source/JavaScriptCore/API/APIShims.h:93 > + : m_dropAllLocks(shouldDropAllLocks(vm) ? &vm : 0) nullptr > Source/JavaScriptCore/API/ObjCCallbackFunction.mm:411 > // We need to explicity release the target since we didn't call Missing an "l" in explicitly. Should probably grep for this typo. I think I’ve seen it elsewhere too. > Source/JavaScriptCore/API/ObjCCallbackFunction.mm:524 > + ObjCCallbackFunction* function = jsCast<ObjCCallbackFunction*>(cell); I’d use a reference for the local variable since it’s never null.
Committed r159351: <http://trac.webkit.org/changeset/159351>
It caused assertions on !CF platforms, see https://bugs.webkit.org/show_bug.cgi?id=124490 for details and proposed fix.