Last Comment Bug 776923 - Plugin tag management issues in nsPluginHost
: Plugin tag management issues in nsPluginHost
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: 17 Branch
: All All
: -- major (vote)
: mozilla17
Assigned To: Georg Fritzsche [:gfritzsche]
:
Mentors:
Depends on: 839728
Blocks: 686335
  Show dependency treegraph
 
Reported: 2012-07-24 07:55 PDT by Georg Fritzsche [:gfritzsche]
Modified: 2013-02-09 04:50 PST (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix plugin tag management (6.00 KB, patch)
2012-07-24 08:11 PDT, Georg Fritzsche [:gfritzsche]
no flags Details | Diff | Review
Fix plugin tag management, v2 (6.06 KB, patch)
2012-07-24 11:41 PDT, Georg Fritzsche [:gfritzsche]
jaas: review+
Details | Diff | Review

Description Georg Fritzsche [:gfritzsche] 2012-07-24 07:55:20 PDT
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] 2012-07-24 08:11:03 PDT
Created attachment 645307 [details] [diff] [review]
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 2012-07-24 10:22:04 PDT
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] 2012-07-24 11:41:58 PDT
Created attachment 645405 [details] [diff] [review]
Fix plugin tag management, v2

Addressed review comments.
Comment 4 Georg Fritzsche [:gfritzsche] 2012-07-24 11:43:14 PDT
(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 [:bsmedberg] 2012-07-24 11:45:38 PDT
No, I think you can safely ignore that scenario.
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-07-24 19:08:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8487f92ae1
Comment 7 Ed Morley [:emorley] 2012-07-25 08:10:23 PDT
https://hg.mozilla.org/mozilla-central/rev/4f8487f92ae1

Note You need to log in before you can comment on or make changes to this bug.