Summary: | -dealloc callbacks from wrapped Objective-C objects can happen at bad times | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | buildbot, eflews.bot, gyuyoung.kim, ossy, rego+ews, rniwa, xan.lopez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 124490 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2013-11-05 13:17:53 PST
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. |