Delay GC triggered NP object destruction to the next runloop cycle

[Oliver Hunt]

Delay GC triggered NP object destruction to the next runloop cycle

[Oliver Hunt]

Created attachment 104743 [details]
Patch

[Anders Carlsson]

Comment on attachment 104743 [details]
Patch

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.

[Oliver Hunt]

Created attachment 104753 [details]
Patch

[Darin Adler]

Comment on attachment 104753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104753&action=review

> Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.h:53
> +    // Used to invalidate an NPObject asynchronously

Periods at ends of comments is the usual WebKit style.

> Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp:223
> +    Vector<Strong<JSNPObject> > jsNPObjects;

This could just be called “objects” or “objectsToInvalidate” instead of having a clump of characters at the beginning.

> Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp:229
> +        jsNPObjects[i].clear();

Is it important to clear each of these explicitly rather than just destroying them? We could just wrap braces around this code so that the local vector is destroyed before the rest of the code runs and maybe leave out that explicit clear.

> Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp:240
> +    Vector<NPObject*> localNPObjects;
> +    localNPObjects.swap(m_npObjectsToFinalize);
> +    for (size_t i = 0; i < localNPObjects.size(); ++i)
> +        deallocateNPObject(localNPObjects[i]);

Why is this implemented differently from invalidateQueuedObjects? Can these share code? If they can’t for a good reason, please add a “why” comment.

[Oliver Hunt]

Created attachment 104756 [details]
Patch

[Oliver Hunt]

Committed r93555: <http://trac.webkit.org/changeset/93555>

[Adam Roben (:aroben)]

Why do we need to do this? (Either the bug or the ChangeLog or both should say!) Can you write a regression test?