73071 - nsIPlugin::Shutdown is not called if there is no nsIPluginInstance object created

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[av (gone)]

It is possible that after general plugin initialization (nsIPlugin 
stuff) nsIPluginInstance object is never created. We should still call 
nsIPlugin::Shutdown method when we clean up. MRJ is an example of such a plugin.

Comment 1

[av (gone)]

--> 0.9

Comment 2

[av (gone)]

Patric, would you please take a look and if this patch will address what we 
talked about the other day?

NS_IMETHODIMP nsPluginHostImpl::Destroy(void)
{
  if (mIsDestroyed)
    return NS_OK;

  mIsDestroyed = PR_TRUE;

-  // at this point nsIPlugin::Shutdown calls will be performed if needed
+  // at this point nsIPlugin::Shutdown calls will be performed on last inst  
  mActivePluginList.shut();

+  //go through the list of the plugins to see if we have anything to shutdown
+  for(nsPluginTag * p = mPlugins; p != nsnull; p = p->mNext)
+  {
+    if(p->mEntryPoint != nsnull)
+    {
+      p->mEntryPoint->Shutdown();
+      p->mEntryPoint = nsnull;
+    }
+
+    if ((nsnull != p->mLibrary) && p->mCanUnloadLibrary)
+    {
+      PR_UnloadLibrary(p->mLibrary);
+      p->mLibrary = nsnull;
+    }
+  }

  return NS_OK;
}

Comment 3

[Peter Lubczynski]

I think I saw another bug like this. Is this patch going in?

Comment 4

[av (gone)]

I filed this bug after Patric pointed out this problem to me. I still want to 
see his response if the solution above will help.

Comment 5

[Patrick C. Beard]

Patch appears to work. r=beard

Comment 6

[Patrick C. Beard]

One point to consider, do we know that all existing plugin instances have been 
deleted at this point? If so, then this is what we've been needing all along.

Comment 7

[av (gone)]

Yes, mActivePluginList.shut(); destroys all the instances. But as far as I 
understand from what we talked about theoretically a plugin (nsIPlugin) may not 
even create a single instance (nsIPluginInstance) but still needs to be 
shutdown. This patch addresses this specific situation.

Comment 8

[Chris Waterson]

sr=waterson

Comment 9

[av (gone)]

There is a loop now in nsPluginHostImpl::Destroy to delete members of the 
plugin list (bug 73289) so we can move this shutdown stuff in there. No reason 
to have a separate loop for this.

Comment 10

[av (gone)]

Attachment: patch with slightly reorganized things

Comment 11

[av (gone)]

cheked in

Comment 12

[shrirang khanzode]

verified patch is in the tree.