Plugin-like files add overhead on every startup

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: (dormant account), Assigned: sgreenlay)

Tracking

(Blocks: 1 bug)

unspecified
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Assignee)

Comment 1

7 years ago
One possible solution would be to include the rejected plugins in the pluginreg so we know not to try opening them again.
(Reporter)

Comment 2

7 years ago
Created attachment 494834 [details] [diff] [review]
do all dll io in GetPluginInfo

This combined with pluginreg tracking will keep us from pointlessly opening dll files.
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.
(Reporter)

Comment 4

7 years ago
Created attachment 494840 [details] [diff] [review]
do all dll io in GetPluginInfo

Now with proper error checking.
Attachment #494834 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
Assignee: nobody → sgreenlay
>+  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.)
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.
(Reporter)

Comment 7

7 years ago
(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.
blocking2.0: --- → ?
(Assignee)

Comment 8

7 years ago
Created attachment 495086 [details] [diff] [review]
Add Arch to PluginReg Header
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?
(Assignee)

Comment 10

7 years ago
Created attachment 495191 [details] [diff] [review]
Add Arch/Invalid to PluginReg (v1.0)
Attachment #495086 - Attachment is obsolete: true
(Reporter)

Comment 11

7 years ago
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.
(Reporter)

Comment 12

7 years ago
This should also throw away invalid plugins that are no longer present.
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.
(Assignee)

Comment 14

7 years ago
Created attachment 495622 [details] [diff] [review]
Add Arch/Invalid to PluginReg (v1.1)
Attachment #495191 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
We should get this in asap to get feedback on it. Aiming for beta9, but if we can do it for beta8, even better.
blocking2.0: ? → beta9+
(Assignee)

Updated

7 years ago
Attachment #495622 - Flags: review?(joshmoz)

Comment 16

7 years ago
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.
(Assignee)

Comment 17

7 years ago
Created attachment 496518 [details] [diff] [review]
Add Arch/Invalid to PluginReg (v1.2)
Attachment #495622 - Attachment is obsolete: true
Attachment #496518 - Flags: review?(joshmoz)
Attachment #495622 - Flags: review?(joshmoz)

Updated

7 years ago
Attachment #496518 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

7 years ago
Attachment #496518 - Flags: review?(tglek)
(Reporter)

Comment 18

7 years ago
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.
Attachment #494840 - Flags: review?(joshmoz)
(Reporter)

Comment 19

7 years ago
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.
Attachment #496518 - Flags: review?(tglek) → review+
(Assignee)

Comment 20

7 years ago
Created attachment 496628 [details] [diff] [review]
Add Arch/Invalid to PluginReg - commit message

Comment 21

7 years ago
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?
Attachment #494840 - Flags: review?(joshmoz) → review+
(Reporter)

Updated

7 years ago
Keywords: checkin-needed
(Reporter)

Comment 22

7 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 23

7 years ago
Created attachment 496872 [details] [diff] [review]
Add Arch/Invalid to PluginReg fix

The patch missed my mq PR_TRUE/PR_FALSE -> true/false changes.
Attachment #496872 - Flags: review?(tglek)
(Reporter)

Updated

7 years ago
Attachment #496872 - Flags: review?(tglek) → review+

Updated

7 years ago
Depends on: 620103

Updated

7 years ago
Depends on: 620114

Updated

7 years ago
Depends on: 620140
(Reporter)

Comment 24

7 years ago
(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

7 years ago
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)
No longer blocks: 614423
blocking2.0: beta9+ → betaN+
No longer depends on: 620103, 620114, 620140

Updated

7 years ago
Blocks: 614423
Depends on: 620103, 620114, 620140
Depends on: 698324
You need to log in before you can comment on or make changes to this bug.