Bug 18208 – Acid3 test 65 takes >33ms due to plugin refreshing on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Adam Roben (:aroben)]

Acid3 test 65 takes 172ms on my MacBook Pro on Windows XP. On Mac the same test takes less than 33ms. The test creates an <object data="svg.xml">, and we seem to spend the majority of the time refreshing the plugin database when we fail to find a plugin to handle text/xml.

[Adam Roben (:aroben)]

Created attachment 20192 [details]
Preliminary cleanup: Make PluginPackage work like our other RefCounted classes

[Darin Adler]

Comment on attachment 20192 [details]
Preliminary cleanup: Make PluginPackage work like our other RefCounted classes

r=me

[Adam Roben (:aroben)]

Created attachment 20193 [details]
Cleanup 2: Change PlatformFileTime to time_t on Windows

[Adam Roben (:aroben)]

Created attachment 20194 [details]
Cleanup 3: Remove PlatformFileTime

[Adam Roben (:aroben)]

Created attachment 20196 [details]
Reduced testcase

Here's a reduced testcase that just creates the object element and puts it into the document.

Comment on attachment 20193 [details]
Cleanup 2: Change PlatformFileTime to time_t on Windows

r=me

Comment on attachment 20194 [details]
Cleanup 3: Remove PlatformFileTime

r=me

[Adam Roben (:aroben)]

Created attachment 20197 [details]
Cleanup 4: Rename PluginPaths -> PluginDirectories

[Adam Roben (:aroben)]

Created attachment 20198 [details]
Reduced testcase that works in Firefox as well

This version of the testcase works in Firefox as well. Firefox doesn't seem to respect the <base> tag when resolving the <object> element's data attribute.

Comment on attachment 20197 [details]
Cleanup 4: Rename PluginPaths -> PluginDirectories

+        void setPluginDirectories(const Vector<String>& paths) { m_pluginDirectories = paths; }

You can change the name of the argument here too.

-bool PluginDatabase::isPreferredPluginPath(const String& path)
+bool PluginDatabase::isPreferredPluginDirectory(const String& path)

And here.

-static inline void addMozillaPluginPaths(Vector<String>& paths)
+static inline void addMozillaPluginDirectories(Vector<String>& paths)

And here.

-static inline void addWindowsMediaPlayerPluginPath(Vector<String>& paths)
+static inline void addWindowsMediaPlayerPluginDirectory(Vector<String>& paths)

And in several other places.

rs=me either way

[Adam Roben (:aroben)]

(In reply to comment #10)
> (From update of attachment 20197 [details] [edit])
> +        void setPluginDirectories(const Vector<String>& paths) {
> m_pluginDirectories = paths; }
> 
> You can change the name of the argument here too.
> 
> -bool PluginDatabase::isPreferredPluginPath(const String& path)
> +bool PluginDatabase::isPreferredPluginDirectory(const String& path)
> 
> And here.
> 
> -static inline void addMozillaPluginPaths(Vector<String>& paths)
> +static inline void addMozillaPluginDirectories(Vector<String>& paths)
> 
> And here.
> 
> -static inline void addWindowsMediaPlayerPluginPath(Vector<String>& paths)
> +static inline void addWindowsMediaPlayerPluginDirectory(Vector<String>& paths)
> 
> And in several other places.
> 
> rs=me either way

I've changed these and more locally. Thanks!

[Adam Roben (:aroben)]

Created attachment 20199 [details]
Cleanup 5: Return const String& from PluginPackage methods

Comment on attachment 20199 [details]
Cleanup 5: Return const String& from PluginPackage methods

r=me

[Adam Roben (:aroben)]

Created attachment 20200 [details]
Cleanup 6: Change getPluginsInDirectories to use an out parameter

Comment on attachment 20200 [details]
Cleanup 6: Change getPluginsInDirectories to use an out parameter

r=me

[Adam Roben (:aroben)]

Created attachment 20209 [details]
Step 1: Separate filesystem crawling from PluginPackage instantiation

[Adam Roben (:aroben)]

Created attachment 20210 [details]
Step 2: Don't do any work if the plugin paths haven't changed between calls to refresh()

I can easily turn attachment 20198 [details] into a regression test, but I worry that it might fail on slower machines. Any suggestions?

Comment on attachment 20209 [details]
Step 1: Separate filesystem crawling from PluginPackage instantiation

r=me

[Darin Adler]

Comment on attachment 20210 [details]
Step 2: Don't do any work if the plugin paths haven't changed between calls to refresh()

+        HashMap<String, time_t>::iterator found = m_pluginPathsWithTimes.find(*it);

Can this just use get instead?

r=me

[Adam Roben (:aroben)]

Created attachment 20236 [details]
Step 2 (revised)

Here's a new version of Step 2. There are two changes from attachment 20210 [details]:

1) I now use HashMap::get instead of HashMap::find, as suggested by Darin in comment 19.
2) We will now handle the case where one or more plugins have been uninstalled but no new plugins have been installed correctly.

[Anders Carlsson]

Comment on attachment 20236 [details]
Step 2 (revised)

r=me

[Adam Roben (:aroben)]

Committed in r31459, r31460, r31461, r31462, r31463, r31644, r31465, and r31466.

[Adam Roben (:aroben)]

*** Bug 18159 has been marked as a duplicate of this bug. ***