[webkit-reviews] review granted: [Bug 123821] -dealloc callbacks from wrapped Objective-C objects can happen at bad times : [Attachment 216976] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 123821: -dealloc callbacks from wrapped Objective-C objects can happen at
bad times
https://bugs.webkit.org/show_bug.cgi?id=123821

Attachment 216976: Patch
https://bugs.webkit.org/attachment.cgi?id=216976&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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".