Closed Bug 776923 Opened 7 years ago Closed 7 years ago

Plugin tag management issues in nsPluginHost

Categories

(Core :: Plug-ins, defect, major)

17 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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
See Also: → 642807
Attached patch Fix plugin tag management (obsolete) — Splinter Review
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).
Attachment #645307 - Flags: review?(joshmoz)
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"?
Attachment #645307 - Flags: review?(joshmoz)
Addressed review comments.
Attachment #645405 - Flags: review?(joshmoz)
(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?
Attachment #645307 - Attachment is obsolete: true
No, I think you can safely ignore that scenario.
Attachment #645405 - Flags: review?(joshmoz) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f8487f92ae1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 839728
You need to log in before you can comment on or make changes to this bug.