616271 - Plugin-like files add overhead on every startup

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(dormant account)]

The check for bad plugins http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/src/nsPluginHost.cpp#2070 doesn't record that result anywhere resulting in us opening the same rejected plugin-like files every startup. For example silverlight's npctrlui.dll

Comment 1

[Scott Greenlay [:sgreenlay]]

One possible solution would be to include the rejected plugins in the pluginreg so we know not to try opening them again.

Comment 2

[(dormant account)]

Attachment: do all dll io in GetPluginInfo

This combined with pluginreg tracking will keep us from pointlessly opening dll files.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 494834 [details] [diff] [review]
do all dll io in GetPluginInfo

>+  if (NS_FAILED(rv = CanLoadPlugin(fullPathUTF8.get())))

CanLoadPlugin doesn't return an nsresult.

Comment 4

[(dormant account)]

Attachment: do all dll io in GetPluginInfo

Now with proper error checking.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

>+  if (NS_FAILED(rv = mPlugin->GetNativePath(fullPathUTF8)))
>+    return rv;

Is this prevailing style here? (I can't see due to lack of 8 lines of context in the patch.) Otherwise, 

  rv = mPlugin->GetNativePath(fullPathUTF8);
  NS_ENSURE_SUCCESS(rv, rv);

would be much preferred. (Actually, it would be preferred even if it isn't the prevailing style here.)

Comment 6

[Benjamin Smedberg]

Do we blow away pluginreg when the arch changes? I know people who switch between 32-bit and 64-bit builds on Linux would have problems if we permanently stored that a plugin-like file didn't load (because it was the wrong architecture). I suspect we'll have similar problems for Windows and Mac soon.

Comment 7

[(dormant account)]

(In reply to comment #6)
> Do we blow away pluginreg when the arch changes? I know people who switch
> between 32-bit and 64-bit builds on Linux would have problems if we permanently
> stored that a plugin-like file didn't load (because it was the wrong
> architecture). I suspect we'll have similar problems for Windows and Mac soon.

That's a good point. Should add arch to the [header] section. 

(In reply to comment #5)
> >+  if (NS_FAILED(rv = mPlugin->GetNativePath(fullPathUTF8)))
> >+    return rv;
> 
> Is this prevailing style here? (I can't see due to lack of 8 lines of context
> in the patch.) Otherwise, 
> 
>   rv = mPlugin->GetNativePath(fullPathUTF8);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
> would be much preferred. (Actually, it would be preferred even if it isn't the
> prevailing style here.)

It is the prevailing style, you can see an example below in the context


btw this blocks the plugin enumeration blocker.

Comment 8

[Scott Greenlay [:sgreenlay]]

Attachment: Add Arch to PluginReg Header

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 495086 [details] [diff] [review]
Add Arch to PluginReg Header

>+  nsCOMPtr<nsIXULRuntime> runtime = do_GetService("@mozilla.org/xre/runtime;1");
>+  if (!runtime)
>+    return rv;

rv will be NS_OK here and below... You sure that's what you want to return?

Comment 10

[Scott Greenlay [:sgreenlay]]

Attachment: Add Arch/Invalid to PluginReg (v1.0)

Comment 11

[(dormant account)]

Comment on attachment 495191 [details] [diff] [review]
Add Arch/Invalid to PluginReg (v1.0)

>+    
>+    nsRefPtr<nsInvalidPluginTag> invalidPlugins = mInvalidPlugins;
>+    while (invalidPlugins) {
>+      // If already marked as invalid, ignore it
>+      if (invalidPlugins->mFullPath.Equals(NS_ConvertUTF16toUTF8(pfd.mFilePath).get()) &&
>+          LL_EQ(invalidPlugins->mLastModifiedTime, fileModTime))
>+      {
>+        continue;
>+      }
>+      invalidPlugins = invalidPlugins->mNext;
>+    }

a) this infinite loops because invalidPlugins = invalidPlugins->mNext; keeps getting skipped
b) I prefer a for loop ie for( nsRefPtr<nsInvalidPluginTag> invalidPlugins = mInvalidPlugins;invalidPlugins;invalidPlugins = invalidPlugins->mNext)
c) Don't use LL_EQ use normal == and other operators. LL_* junk is obsolete.
d) Don't ConvertUTF16toUTF8 the same thing multiple times. That includes use below

>+        nsRefPtr<nsInvalidPluginTag> invalidTag = new nsInvalidPluginTag(NS_ConvertUTF16toUTF8(pfd.mFilePath).get(),
>+                                                                         fileModTime);
>         pluginFile.FreePluginInfo(info);
>+        if (!invalidTag)
>+          return NS_ERROR_OUT_OF_MEMORY;
>+        
>+        invalidTag->mNext = mInvalidPlugins;
>+        mInvalidPlugins = invalidTag;

So is there some flag this should trigger to tell the code to write a new pluginreg file? I got one invalid plugin to be skipped properly. The other one isn't working as well. Not sure if the bug is here.

Comment 12

[(dormant account)]

This should also throw away invalid plugins that are no longer present.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 495191 [details] [diff] [review]
Add Arch/Invalid to PluginReg (v1.0)

>       // if we don't have mime type don't proceed, this is not a plugin
>       if (NS_FAILED(res) || !info.fMimeTypeArray) {
>+        nsRefPtr<nsInvalidPluginTag> invalidTag = new nsInvalidPluginTag(NS_ConvertUTF16toUTF8(pfd.mFilePath).get(),
>+                                                                         fileModTime);
>         pluginFile.FreePluginInfo(info);
>+        if (!invalidTag)
>+          return NS_ERROR_OUT_OF_MEMORY;

Please drop the null check.

>+      const char *lastModifiedTimeStamp = reader.LinePtr();
>+      PRInt64 lastmod = (vdiff == 0) ? nsCRT::atoll(lastModifiedTimeStamp) : -1;
>+      
>+      nsRefPtr<nsInvalidPluginTag> invalidTag = new nsInvalidPluginTag(fullpath, lastmod);
>+      if (!invalidTag)
>+        continue;

Here too.

>+      

Trailing whitespace.

>+      invalidTag->mNext = mInvalidPlugins;
>+      mInvalidPlugins = invalidTag;
>+    }
>+  }
>+  

And here. Please check the entire patch.

Comment 14

[Scott Greenlay [:sgreenlay]]

Attachment: Add Arch/Invalid to PluginReg (v1.1)

Comment 15

[Johnny Stenback (:jst)]

We should get this in asap to get feedback on it. Aiming for beta9, but if we can do it for beta8, even better.

Comment 16

[Josh Aas]

Comment on attachment 495622 [details] [diff] [review]
Add Arch/Invalid to PluginReg (v1.1)

>+nsInvalidPluginTag::nsInvalidPluginTag(const char* aFullPath, PRInt64 aLastModifiedTime)
>+: mFullPath(aFullPath),
>+mLastModifiedTime(aLastModifiedTime),
>+mSeen(PR_FALSE)

Indent these last two lines to match 'mFullPath'.

>+    PRBool ignorePlugin = PR_FALSE;

Change the name of this variable to something like "knownInvalid". "ignorePlugin" sounds a little too much like it refers to ignoring an actual plugin.

>+    for (nsRefPtr<nsInvalidPluginTag> invalidPlugins = mInvalidPlugins;
>+         invalidPlugins; invalidPlugins = invalidPlugins->mNext) 
>+    {

Put the brace at the end of the preceding line.

>+      // If already marked as invalid, ignore it
>+      if (invalidPlugins->mFullPath.Equals(filePath.get()) &&
>+          invalidPlugins->mLastModifiedTime == fileModTime)
>+      {

Same here. Happens in more places.

>+        if (aCreatePluginList)
>+          invalidPlugins->mSeen = PR_TRUE;

I'd prefer that all new code have braces for "if" statements.

More comments soon.

Comment 17

[Scott Greenlay [:sgreenlay]]

Attachment: Add Arch/Invalid to PluginReg (v1.2)

Comment 18

[(dormant account)]

Comment on attachment 494840 [details] [diff] [review]
do all dll io in GetPluginInfo

This patch makes it so we only check plugin validity before we add plugins to pluginreg.dat instead of on every startup. This avoid completely unnecessary io.

Comment 19

[(dormant account)]

Comment on attachment 496518 [details] [diff] [review]
Add Arch/Invalid to PluginReg (v1.2)


>+  
>+  // Remove unseen invalid plugins
>+  nsRefPtr<nsInvalidPluginTag> invalidPlugins = mInvalidPlugins;
>+  while (invalidPlugins) {
>+    if (!invalidPlugins->mSeen) {
>+      nsRefPtr<nsInvalidPluginTag> invalidPlugin = invalidPlugins;
>+      
>+      if (!invalidPlugin->mPrev) {
>+        mInvalidPlugins = invalidPlugin->mNext;
>+      }
>+      else {
>+        invalidPlugin->mPrev->mNext = invalidPlugin->mNext;
>+      }
Please invert this if

>+      if (invalidPlugin->mNext) {
>+        invalidPlugin->mNext->mPrev = invalidPlugin->mPrev; 
>+      }
>+      
>+      invalidPlugins = invalidPlugin->mNext;
>+      
>+      invalidPlugin->mPrev = NULL;
>+      invalidPlugin->mNext = NULL;
>+      
>+      // Pluginreg rewritten to remove unseen plugins
>+      *aPluginsChanged = PR_TRUE;

No real need to rewrite the pluginreg explicitly for this. Ie it can be rewritten next time a plugin is added or removed. 

>+    }
>+    else {
>+      invalidPlugins->mSeen = PR_FALSE;

Why is mSeen being reset?

>+      invalidPlugins = invalidPlugins->mNext;

Optional: I prefer explicit for loops, but due to item skipping, while is a matter of taste.

Please replace the handful of PRBools in the patch with bool. r+ with these minor changes.

I tested the latest patch on my machine, it works as expected. Thanks for the quick turnaround on this.

Comment 20

[Scott Greenlay [:sgreenlay]]

Attachment: Add Arch/Invalid to PluginReg - commit message

Comment 21

[Josh Aas]

Comment on attachment 494840 [details] [diff] [review]
do all dll io in GetPluginInfo

Looks good. Can we do this for Mac OS X as well?

Comment 22

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/8a0d2af384bc
http://hg.mozilla.org/mozilla-central/rev/b48addc64c2f

Landed this to get some exposure asap. I'll look at what happens on mac/linux in followup bugs.

Comment 23

[Scott Greenlay [:sgreenlay]]

Attachment: Add Arch/Invalid to PluginReg fix

The patch missed my mq PR_TRUE/PR_FALSE -> true/false changes.

Comment 24

[(dormant account)]

(In reply to comment #23)
> 
> The patch missed my mq PR_TRUE/PR_FALSE -> true/false changes.

http://hg.mozilla.org/mozilla-central/rev/238f53866895

Comment 25

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)