265936 - Memory leak in nsPluginHostImpl::ScanPluginsDirectory()

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Boying Lu]

// Look for it in our cache
    nsPluginTag *pluginTag =
RemoveCachedPluginsInfo(NS_ConvertUCS2toUTF8(pfd->mFilename).get());

    delete pfd;
    if (pluginTag) {
      // If plugin changed, delete cachedPluginTag and dont use cache
      if (LL_NE(fileModTime, pluginTag->mLastModifiedTime)) {
        // Plugins has changed. Dont use cached plugin info.
        delete pluginTag;
        pluginTag = nsnull;

        // plugin file changed, flag this fact
        *aPluginsChanged = PR_TRUE;
      }
      else {
        // if it is unwanted plugin we are checking for, get it back to the
cache info list
        // if this is a duplicate plugin, too place it back in the cache info
list marking unwantedness
        if((checkForUnwantedPlugins && isUnwantedPlugin(pluginTag)) ||
IsDuplicatePlugin(pluginTag) || isUnwantedJavaPlugin(pluginTag)) {         
pluginTag->Mark(NS_PLUGIN_FLAG_UNWANTED);
          pluginTag->mNext = mCachedPlugins;
          mCachedPlugins = pluginTag;
        } // else
          // delete pluginTag; //brian
      }
    }
    else {
      // plugin file was added, flag this fact
      *aPluginsChanged = PR_TRUE;
    }

    // if we are not creating the list, just continue the loop
    // no need to proceed if changes are detected
    if (!aCreatePluginList) {
      if (*aPluginsChanged)    // leak here
        return NS_OK;
      else
        continue;    
}

pluginTag is not deleted when if(!aCreatePluginList){...} is executed.

pluginTag is removed from mCachedPlugins. it should be deleted before:
      if (*aPluginsChanged)    
        return NS_OK;
      else
        continue;
is executed.

Comment 1

[Boying Lu]

Attachment: delete pluginTag before return

Comment 2

[Boying Lu]

Comment on attachment 163289 [details] [diff] [review]
delete pluginTag before return

Can you give r? Thanks

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 163289 [details] [diff] [review]
delete pluginTag before return

+      if (pluginTag)
+	 delete pluginTag;

delete is null safe, no need for the if check.

r=jst with that change.

Comment 4

[Boying Lu]

Attachment: remove if(...) - bogus

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 163522 [details] [diff] [review]
remove if(...) - bogus

r+sr=jst for this change. Let me know if you need help checking this in.

Comment 6

[Boying Lu]

yes. Please help me to checkin the patch. Thanks

Comment 7

[Johnny Stenback (:jst)]

Fix checked in. Thanks for the diff!

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

This might've caused bug 269472.

Comment 9

[timeless]

The fix is wrong. Check the codepaths, there's a member variable which has the
plugintag you're deleting. I've backed this out:
mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v  <--  nsPluginHostImpl.cpp
new revision: 1.508; previous revision: 1.507

Comment 10

[Bill Mason]

(In reply to comment #8)
> This might've caused bug 269472.

And bug 269824.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 163522 [details] [diff] [review]
remove if(...) - bogus

Besides the problem timeless points out (is the right solution not to delete,
or to null the member that dangled with Brian's patch?), there's an else after
return visible here.  Please fix that too.

/be

Comment 12

[Boying Lu]

Attachment: a new proposal

Comment 13

[Boying Lu]

Comment on attachment 166991 [details] [diff] [review]
a new proposal

Can you give r/sr? Thanks

Comment 14

[timeless]

Comment on attachment 166991 [details] [diff] [review]
a new proposal

where to begin.

1. please use considerably more context, i'd suggest -up105, as that's what it
took for me to see the whole function.
2. you didn't address brendan's comment about silly code
3. mCachedPlugins is a linked list comparing a plugin tag against the head of
the list is asking for us to crash again.

Comment 15

[Boying Lu]

Timeless, thanks for your feedback. I can't fully understand Brendan's comment.
The following is what I got after reading the codes in
nsPluginHostImpl::ScanPluginsDierctory()
1.  nsPluginTag *pluginTag = RemoveCachedPluginsInfo(...)
this statement shows that pluginTag points to an object which is deleted from
mCachedPlugins list.

2. pluginTag is put back to the list mCachePlugins only if a unwanted plugin and
in this case it is put at the head of the list. I.e. mCachedPlugins points to it

So I think we can use  mCachedPlugins != pluginTag to detect the case 2.

Obviously, pluginTag will be leaked in:
   if (!aCreatedPlugins) {
      if (*aPluginsChanged)
         return NS_OK;    //leaked here
      else
         continue;    // leaked here also
   }

If "continue" is executed, pluginTag will point to next object in the next iterate.

Comment 16

[Stuart Morgan]

It looks like leaked nsPluginTags are a significant source of leaks in Camino, so I'll try to take a look at this when I have a chance.

Comment 17

[Stuart Morgan]

Well, this is ugly.

On the face of it, the last patch here seems like it should be right: if the item is going to be thrown away from all the lists, delete it rather than leaking it. However, I was able to get into infinite loops of calls to ScanPluginsDirectory for reasons I don't understand but presume are related to the fact that a side effect of calling the destructor for an nsPluginTag is to unload the plugin.

Sticking it back on the queue seemed to work in simple tests, but I'm not sure whether or not that's the correct fix, since it means that something in the cache list is there (and changed) that would have been gone otherwise.

Comment 18

[Stuart Morgan]

Unfortunately, that doesn't work either: I traced the early return code back to bug 119621, and that change fails the recursive load testcase attached there. So it appears that the system developed to handle those two issues (recursive reload and alternating reload failure) only ever worked because of the leak it introduced.

Comment 19

[Boying Lu]

(In reply to comment #17)
> Well, this is ugly.
> 
> On the face of it, the last patch here seems like it should be right: if the
> item is going to be thrown away from all the lists, delete it rather than
> leaking it. However, I was able to get into infinite loops of calls to
> ScanPluginsDirectory for reasons I don't understand but presume are related to
> the fact that a side effect of calling the destructor for an nsPluginTag is to
> unload the plugin.
> 

Can you give me a testcase for the "infinite loops"?  Can it be re-produced on Linux?

Comment 20

[Stuart Morgan]

It's been a while since I looked at this, but as I recall the loop was triggered most of the time by going back and forth between about:plugins and a web page (I don't think it had to be one with plugin content). The test case I mentioned in comment 18 was also useful. I have no idea if it's reproducible on Linux.

Comment 21

[Boying Lu]

I added the last patch to the trunk code (2007-05-25) and made both debug  and release builds for Window XP.  But I can't re-produce the bugs 269472 and 269824.

Can anyone help me to re-produce those two bugs? I want to make a new patch for this bug.  Thanks

Comment 22

[Michael Wu [:mwu]]

Due to the refcounting of nsPlugInTags introduced by bug 382367, this should not be a problem anymore. If someone can confirm/deny this, it will be helpful. Otherwise, I'll do some testing later this week and probably resolve this bug.

Comment 23

[Michael Wu [:mwu]]

Fixed by bug 382367's refcounted nsPluginTags, as far as I can tell.