Bug 123821 – -dealloc callbacks from wrapped Objective-C objects can happen at bad times

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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."

[Mark Hahnenberg]

<rdar://problem/15395983>

[Mark Hahnenberg]

Created attachment 216976 [details]
Patch

[EFL EWS Bot]

Comment on attachment 216976 [details]
Patch

Attachment 216976 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/23748057

[Geoffrey Garen]

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".

[Darin Adler]

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.

[Mark Hahnenberg]

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.

[EFL EWS Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 216976 [details]
Patch

Attachment 216976 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/23628066

[Mark Hahnenberg]

Created attachment 216987 [details]
Patch

[EFL EWS Bot]

Comment on attachment 216987 [details]
Patch

Attachment 216987 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/23618068

[Mark Hahnenberg]

Created attachment 216988 [details]
Patch

[EFL EWS Bot]

Comment on attachment 216988 [details]
Patch

Attachment 216988 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/23688068

[Mark Hahnenberg]

Created attachment 216989 [details]
Patch

[Darin Adler]

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.

[Mark Hahnenberg]

Committed r159351: <http://trac.webkit.org/changeset/159351>

[Csaba Osztrogonác]

It caused assertions on !CF platforms, see https://bugs.webkit.org/show_bug.cgi?id=124490 for details and proposed fix.