Bug 66717

Summary: Delay GC triggered NP object destruction to the next runloop cycle
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: New BugsAssignee: Oliver Hunt <oliver>
Severity: Normal CC: aroben, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch andersca: review+

Description Oliver Hunt 2011-08-22 14:46:35 PDT
Delay GC triggered NP object destruction to the next runloop cycle
Comment 1 Oliver Hunt 2011-08-22 14:48:46 PDT
Created attachment 104743 [details]
Comment 2 Anders Carlsson 2011-08-22 14:55:13 PDT
Comment on attachment 104743 [details]

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.
Comment 3 Oliver Hunt 2011-08-22 15:28:18 PDT
Created attachment 104753 [details]
Comment 4 Darin Adler 2011-08-22 15:37:04 PDT
Comment on attachment 104753 [details]

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.
Comment 5 Oliver Hunt 2011-08-22 15:50:45 PDT
Created attachment 104756 [details]
Comment 6 Oliver Hunt 2011-08-22 16:08:55 PDT
Committed r93555: <http://trac.webkit.org/changeset/93555>
Comment 7 Adam Roben (:aroben) 2011-08-22 18:28:23 PDT
Why do we need to do this? (Either the bug or the ChangeLog or both should say!) Can you write a regression test?