[webkit-reviews] review granted: [Bug 119044] [Win] Crash after plugin is unloaded. : [Attachment 235229] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted peavo at outlook.com's request for
review:
Bug 119044: [Win] Crash after plugin is unloaded.
https://bugs.webkit.org/show_bug.cgi?id=119044

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

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


r=me, but please consider landing my version instead

> Source/WebCore/bridge/runtime_root.cpp:118
>	   HashMap<RuntimeObject*, JSC::Weak<RuntimeObject>>::iterator end =
m_runtimeObjects.end();
>	   for (HashMap<RuntimeObject*, JSC::Weak<RuntimeObject>>::iterator it
= m_runtimeObjects.begin(); it != end; ++it) {
> -	       RuntimeObject* runtimeObject = it->value.get();
> -	       if (!runtimeObject) // Skip zombies.
> +	       // Use the hash key to get the runtime object, since we want to
invalidate all runtime objects.
> +	       // If we use the weak pointer from the hash value, it might be
null, and it will not be invalidated.
> +	       // This should be safe since finalized runtime objects are
removed from the hash table in the RootObject::finalize() method.
> +	       RuntimeObject* runtimeObject = it->key;
> +	       if (!runtimeObject)
>		   continue;
>	       runtimeObject->invalidate();
>	   }

The null check isn’t needed. A HashMap with pointers for keys can’t hold a
null, so there is no need to check for it. Here’s the best way to write this:

    // Get the objects from the keys; the values might be nulled.
    // Safe because finalized runtime objects are removed from m_runtimeObjects
by RootObject::finalize.
    for (RuntimeObject* runtimeObject : m_runtimeObjects.keys())
	runtimeObject->invalidate();