1335448 - Fix a couple things in the hidden plugin test

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: hidden_plugin_test.patch

Two things I noticed while investigating something else. setTestPluginEnabledState looks up the plugin by name using getTestPlugin. Unfortunately it then checks the enabled state based on the plugin name, not the looked up plugin, so if there is a mismatch you can get an infinitely spinning nested event loop (comments in the function already express the appropriate levels of disgust).

Second setTestPluginEnabledState does its own clean up, so the registerCleanupFunction call is not needed (and could be causing problems for subsequent tests).

Comment 1

[Mike Conley (:mconley) (:⚙️) (PTO June 8 - 12)]

Comment on attachment 8832085 [details] [diff] [review]
hidden_plugin_test.patch

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

Thanks for the patch, blassey.

I must say I'm a little confused by what we're trying to do here - see below.

::: dom/plugins/test/mochitest/test_hidden_plugin.html
@@ +18,3 @@
>      let prefName = "plugins.navigator.hidden_ctp_plugin";
>      try {
>        oldPrefVal = SpecialPowers.getCharPref(prefName);

While you're cleaning up in here, it doesn't look like oldPrefVal is used for anything, and can be removed.

@@ +22,5 @@
>      let promise = SpecialPowers.pushPrefEnv({ set: [[prefName, testPluginName]]});
>      promise.then(function() {
>        for (let i = 0; i < plugins.length; i++) {
>          let plugin = plugins[i];
> +	if (plugin.name != getTestPlugin(plugins.name).name) {

plugins is the Array of PluginTags, so plugins.name is going to be undefined.

But even if this was made to be getTestPlugin(plugin.name)...I don't think I fully understand what we're trying to do here.

My understanding of this test was that we were intentionally disabling all except one plugin (the first one in navigator.plugins), and setting that one to click-to-play (and hidden). Is that not what we want to be doing here?

I ask, because here's what I'm seeing:

1) We get at the nsIPluginHost, and ask for all plugin tags
2) The very first tag that we get, we stash its name
3) After setting a few things up, we go through each tag, and call getTestPlugin, passing plugin.name
4) getTestPlugin is here: http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/dom/plugins/test/mochitest/plugin-utils.js#10-23

It gets at the nsIPluginHost, gets the tags Array, and returns the nsIPluginTag that has a name matching the one we just passed in.

But isn't "plugin" in this context the nsIPluginTag we were looking for? What are we solving for by searching through the tags list again via getTestPlugin?

Or do I misunderstand what we're supposed to be testing here?

@@ -27,5 @@
>                          SpecialPowers.Ci.nsIPluginTag.STATE_DISABLED);
>          if (plugin.enabledState != newState) {
>            let oldState = plugin.enabledState;
>            setTestPluginEnabledState(newState, plugin.name);
> -          SimpleTest.registerCleanupFunction(function() {

Good catch - I forgot that setTestPluginEnabledState took care of that for us.

Comment 2

[BMO Automation]

Resolving as wont fix, plugin support deprecated in Firefox 85.