npruntime bug, plugin object deleted twice on CPlugin::init failure

[Reed Bement]

While hacking about in the debugger with the error paths on my
npruntime based plugin, I discovered what looks like a bug in
NPP_Gate.cpp, NPP_SetWindow:

NPError NPP_SetWindow (NPP instance, NPWindow* pNPWindow)
...
CPlugin * pPlugin = (CPlugin *)instance->pdata;

if(pPlugin == NULL) return NPERR_GENERIC_ERROR;

// window just created
if(!pPlugin->isInitialized() && (pNPWindow->window != NULL)) {
if(!pPlugin->init(pNPWindow)) {
delete pPlugin; // BUGBUG remove, let NPP_Destroy take
care of it.
pPlugin = NULL; // BUGBUG remove, pPlugin is a local
variable they were probably intending to set instance->pdata
return NPERR_MODULE_LOAD_FAILED_ERROR;
}
...

In my code I simply removed the above indicated lines and let
PP_Destroy do the cleanup.

[taxilian]

The indicated lines should only be executed if init returns false,
which would indicate a serious failure with the plugin. Thus, it
should set the plugin to NULL and NP_Destroy will just call free on
the NULL value, which is perfectly valid. Under certain circumstances
you may want to continue using the plugin even if (for whatever
reason) your init fails, but it depends on the plugin, since this is
very plugin-specific code and not anything that has to be "exactly
so". I wouldn't consider this a bug, but rather an implementation
detail that is left to the preference of the user.

[Reed Bement]

True enough if they had instance->pdata; to NULL, but they did not.
They set the local pPlugin variable to NULL, which doesn't make any
sense in this context. The result is two calls to NP_Destroy (and an
associated fault). It's a bug. If people put all of the object create
code which can fail in init (as the sample authors did), the failure
path should be correct.