Closed Bug 616271 Opened 10 years ago Closed 10 years ago

Plugin-like files add overhead on every startup

Categories

(Core :: Plug-ins, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: taras.mozilla, Assigned: sgreenlay)

References

Details

Attachments

(4 files, 4 obsolete files)

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
One possible solution would be to include the rejected plugins in the pluginreg so we know not to try opening them again.
Attached patch do all dll io in GetPluginInfo (obsolete) — Splinter Review
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.
Now with proper error checking.
Attachment #494834 - Attachment is obsolete: true
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.
(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: --- → ?
Attached patch Add Arch to PluginReg Header (obsolete) — Splinter Review
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?
Attachment #495086 - Attachment is obsolete: true
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.
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.
Attachment #495191 - Attachment is obsolete: true
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+
Attachment #495622 - Flags: review?(joshmoz)
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.
Attachment #495622 - Attachment is obsolete: true
Attachment #496518 - Flags: review?(joshmoz)
Attachment #495622 - Flags: review?(joshmoz)
Attachment #496518 - Flags: review?(joshmoz) → review+
Attachment #496518 - Flags: review?(tglek)
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)
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+
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+
Keywords: checkin-needed
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
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
The patch missed my mq PR_TRUE/PR_FALSE -> true/false changes.
Attachment #496872 - Flags: review?(tglek)
Attachment #496872 - Flags: review?(tglek) → review+
Depends on: 620103
Depends on: 620114
Depends on: 620140
(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
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
Blocks: 614423
Depends on: 620103, 620114, 620140
You need to log in before you can comment on or make changes to this bug.