776923 - Plugin tag management issues in nsPluginHost

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Bug 642807 showed problems with the tag management changes in 686335:
 * different versions of in-process plugins being loaded side-by-side
 * duplicate plugin tags being created for unmodified plugins when there are changes in the plugin directory

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix plugin tag management

This patch:
 * avoids creating duplicate tags when reloading plugins
 * avoids loading different version of the same plugin if it runs in-process
 * prefers the most recently modified version of a plugin (matters for in-process plugins)

Tested Flash pave-over (on Windows) and in- and out-of-process duplicates of the test-plugin (Windows and OS X).

Comment 2

[Josh Aas]

Comment on attachment 645307 [details] [diff] [review]
Fix plugin tag management

Review of attachment 645307 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. I don't see any major problems but I'd like to see a final patch.

::: dom/plugins/base/nsPluginHost.cpp
@@ +2198,5 @@
>        *aPluginsChanged = true;
>      }
> +    
> +    // Don't add duplicate plugins if they are running in-process,
> +    // otherwise we risk undefined behaviour.

Maybe note in this comment what exactly "duplicate" means.

@@ +2199,5 @@
>      }
> +    
> +    // Don't add duplicate plugins if they are running in-process,
> +    // otherwise we risk undefined behaviour.
> +    if (!nsNPAPIPlugin::RunPluginOOP(pluginTag)) {

This value could theoretically change by the time we instantiate, but I don't think that will be a significant issue.

::: dom/plugins/base/nsPluginTags.cpp
@@ +379,5 @@
>    return HasFlag(NS_PLUGIN_FLAG_ENABLED) && !HasFlag(NS_PLUGIN_FLAG_BLOCKLISTED);
>  }
>  
> +bool
> +nsPluginTag::Equals(const nsPluginTag *aPluginTag) const

Equals is a pretty ambiguous name here. I wouldn't know what it meant without reading the code. Can we rename this to the admittedly more tedious "HasSameNameAndMimes"?

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix plugin tag management, v2

Addressed review comments.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Josh Aas (Mozilla Corporation) from comment #2)
> @@ +2199,5 @@
> >      }
> > +    
> > +    // Don't add duplicate plugins if they are running in-process,
> > +    // otherwise we risk undefined behaviour.
> > +    if (!nsNPAPIPlugin::RunPluginOOP(pluginTag)) {
> 
> This value could theoretically change by the time we instantiate, but I
> don't think that will be a significant issue.

Hm, is this a supported scenario that i should look into?

Comment 5

[Benjamin Smedberg]

No, I think you can safely ignore that scenario.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8487f92ae1

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4f8487f92ae1