Summary: | Delay GC triggered NP object destruction to the next runloop cycle | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oliver Hunt <oliver> | ||||||||
Component: | New Bugs | Assignee: | Oliver Hunt <oliver> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aroben, yong.li.webkit | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Oliver Hunt
2011-08-22 14:46:35 PDT
Created attachment 104743 [details]
Patch
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. Created attachment 104753 [details]
Patch
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. Created attachment 104756 [details]
Patch
Committed r93555: <http://trac.webkit.org/changeset/93555> Why do we need to do this? (Either the bug or the ChangeLog or both should say!) Can you write a regression test? |