Bug 123821 - -dealloc callbacks from wrapped Objective-C objects can happen at bad times
Summary: -dealloc callbacks from wrapped Objective-C objects can happen at bad times
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 124490
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-05 13:17 PST by Mark Hahnenberg
Modified: 2013-11-18 03:17 PST (History)
7 users (show)

See Also:


Attachments
Patch (23.39 KB, patch)
2013-11-14 13:50 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (25.27 KB, patch)
2013-11-14 15:07 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (25.48 KB, patch)
2013-11-14 15:24 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (25.48 KB, patch)
2013-11-14 15:33 PST, Mark Hahnenberg
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2013-11-05 13:17:53 PST
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."
Comment 1 Mark Hahnenberg 2013-11-05 13:18:46 PST
<rdar://problem/15395983>
Comment 2 Mark Hahnenberg 2013-11-14 13:50:32 PST
Created attachment 216976 [details]
Patch
Comment 3 EFL EWS Bot 2013-11-14 14:00:59 PST
Comment on attachment 216976 [details]
Patch

Attachment 216976 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/23748057
Comment 4 Geoffrey Garen 2013-11-14 14:06:08 PST
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 5 Darin Adler 2013-11-14 14:14:21 PST
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 6 Mark Hahnenberg 2013-11-14 14:15:44 PST
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 7 EFL EWS Bot 2013-11-14 14:17:23 PST
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 8 Build Bot 2013-11-14 14:23:34 PST
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 9 Build Bot 2013-11-14 14:54:06 PST
Comment on attachment 216976 [details]
Patch

Attachment 216976 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/23628066
Comment 10 Mark Hahnenberg 2013-11-14 15:07:04 PST
Created attachment 216987 [details]
Patch
Comment 11 EFL EWS Bot 2013-11-14 15:14:37 PST
Comment on attachment 216987 [details]
Patch

Attachment 216987 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/23618068
Comment 12 Mark Hahnenberg 2013-11-14 15:24:39 PST
Created attachment 216988 [details]
Patch
Comment 13 EFL EWS Bot 2013-11-14 15:29:56 PST
Comment on attachment 216988 [details]
Patch

Attachment 216988 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/23688068
Comment 14 Mark Hahnenberg 2013-11-14 15:33:27 PST
Created attachment 216989 [details]
Patch
Comment 15 Darin Adler 2013-11-14 17:44:01 PST
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.
Comment 16 Mark Hahnenberg 2013-11-15 11:51:19 PST
Committed r159351: <http://trac.webkit.org/changeset/159351>
Comment 17 Csaba Osztrogonác 2013-11-18 03:17:11 PST
It caused assertions on !CF platforms, see https://bugs.webkit.org/show_bug.cgi?id=124490 for details and proposed fix.