[webkit-reviews] review granted: [Bug 131691] JSVirtualMachine could disappear and take its external object graph with it : [Attachment 229847] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 131691: JSVirtualMachine could disappear and take its external object graph
with it
https://bugs.webkit.org/show_bug.cgi?id=131691

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229847&action=review


> Source/JavaScriptCore/API/APIData.h:36
> +class APIData {

Maybe this can just be a struct rather than having getters and setters.

> Source/JavaScriptCore/API/APIData.h:40
> +    APIData()
> +    {
> +    }

Should just omit this. Generated one should be fine.

> Source/JavaScriptCore/API/APIData.h:62
> +    RetainPtr<CFTypeRef> m_externalObjectGraph;
> +    RetainPtr<CFTypeRef> m_externalRememberedSet;

Why so coy about the fact that the type is NSMapTable? Is there a problem
compiling that when it’s non-Objective-C++?

> Source/JavaScriptCore/API/JSVirtualMachine.mm:118
> +	   vm.apiData.setExternalObjectGraph(RetainPtr<CFTypeRef>(
> +	       [[NSMapTable alloc] initWithKeyOptions:weakIDOptions
valueOptions:strongIDOptions capacity:0]));

This leaks. It needs to be adoptNS, not just a conversion to RetainPtr.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:125
> +	   vm.apiData.setExternalRememberedSet(RetainPtr<CFTypeRef>(
> +	       [[NSMapTable alloc] initWithKeyOptions:weakIDOptions
valueOptions:integerOptions capacity:0]));

Ditto.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:187
> +    NSMapTable *externalObjectGraph =
static_cast<id>(vm.apiData.externalObjectGraph());

Why is this one casting to <id>, but all the others casting to NSMapTable?

> Source/JavaScriptCore/runtime/VM.h:213
> +	   APIData apiData;

Why so high in the class? Is this in some sort of order?