[webkit-reviews] review denied: [Bug 66717] Delay GC triggered NP object destruction to the next runloop cycle : [Attachment 104743] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Oliver Hunt
<oliver at apple.com>'s request for review:
Bug 66717: Delay GC triggered NP object destruction to the next runloop cycle
https://bugs.webkit.org/show_bug.cgi?id=66717

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

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104743&action=review


> Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp:80
> +NPObject* JSNPObject::releaseObject()

Maybe this should be called leakNPObject() that would match the WTF idiom.
Bonus points for adding WARN_UNUSED_RETURN to the declaration!

> Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp:83
> +    ASSERT_GC_OBJECT_INHERITS(this, &s_info);

I like to add a newline after asserts.

> Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp:47
> +    : RunLoop::Timer<NPRuntimeObjectMap>(RunLoop::current(), this,
&NPRuntimeObjectMap::invalidateQueuedObjects)

I think this should use WebProcess::shared().runLoop().

> Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp:289
> +    // queue

No need to put "queue" on a new line.

> Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.h:54
> +class NPRuntimeObjectMap : private JSC::WeakHandleOwner,
RunLoop::Timer<NPRuntimeObjectMap> {

NPRuntimeObject shouldn't inherit from RunLoop::Timer, the timer should be a
member variable of NPRuntimeObjectMap.

> Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeUtilities.cpp:109
> +    ASSERT(npObject->referenceCount >= 1);
> +    npObject->referenceCount--;
> +    if (npObject->referenceCount)
> +	   return true;
> +    if (npObject->_class->deallocate)
> +	   return false;
> +    deallocateNPObject(npObject);
> +    return true;

I'd like to see some more comments here.

> Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeUtilities.h:57
> +bool trySafeReleaseNPObject(NPObject*);

Please add a comment specifying what this function does.