[e10s] Disabled plugin meta information often isn't available to content processes

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jimm, Assigned: blassey)

Tracking

Trunk
mozilla42
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+, firefox40 affected, firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This causes problems for tests at least, and may fail to display proper plugin information to users as well.

STR:

1) Using test helpers in browser/base/content/test/plugins run the following test code using browser_plugin_infolink.js:

setTestPluginEnabledState(Ci.nsIPluginTag.STATE_ENABLED, "Test Plug-in");

yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_test.html");

yield ContentTask.spawn(gTestBrowser, {}, function* () {
  let pluginName = "Test Plug-in";
  let ph = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
  dump("ph:" + ph + "\n");
  let tags = ph.getPluginTags();
  dump("tags:" + tags + "\n");

  // Find the test plugin
  let plugin = null;
  for (let i = 0; i < tags.length; i++) {
    dump("plugin: " + tags[i].name + "\n");
    if (tags[i].name == pluginName) {
      plugin = tags[i];
      break;
    }
  }
  dump(plugin + "\n");
  dump(plugin.enabledState + "\n");
});

In this case the content side query finds the test plugin and dumps the enabledState.

2) Run the same test with the initial state set to STATE_DISABLED.

result: exception thrown as the test plugin is no longer in the plugin tags list.
In content the plugin disabled ui doesn't display either.
tracking-e10s: --- → ?
Duplicate of this bug: 1149552
Assignee: nobody → mconley
Assignee: mconley → jmathies
Posted patch getAllPlugins.patch (obsolete) — Splinter Review
Assignee: jmathies → blassey.bugs
Attachment #8632342 - Flags: review?(jmathies)
Comment on attachment 8632342 [details] [diff] [review]
getAllPlugins.patch

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

::: dom/plugins/base/nsPluginHost.cpp
@@ +2510,5 @@
>    if (aPluginEpoch == ChromeEpoch()) {
>      return;
>    }
>  
> +  //nsTArray<nsRefPtr<nsPluginTag>> plugins;

nit

@@ +2513,5 @@
>  
> +  //nsTArray<nsRefPtr<nsPluginTag>> plugins;
> +  uint32_t count;
> +  nsIPluginTag** plugins;
> +  GetPluginTags(&count, &plugins);

How does switching from GetPlguins to GetPlguinTags fix this bug?

https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp?from=GetPluginTags&case=true#1107

I don't see anything in plugin tags that changes the behavior here. I see that GetPlugins checks (plugin->IsEnabled()) before adding it to the list and the tags call does not, but this seem the reverse of what we want here.
Flags: needinfo?(blassey.bugs)
If a plugin is disabled the content process isn't told about its existence. That's why this test isn't getting information about disabled plugins.
Flags: needinfo?(blassey.bugs)
Comment on attachment 8632342 [details] [diff] [review]
getAllPlugins.patch

<blassey> I'm confused by your question in bug 1160166
<jimm> hey
<jimm> oh I was just curious, the bug is about plugin enabled state being out of date in the child right?
<blassey> no
<jimm> and you swapped one call there for another
<blassey> the actual failure is
<blassey> the test is counting how many plugins on the page are disabled
<blassey> and its not finding any
<blassey> that's because the content process isn't told about the disabled plugins
<blassey> so it treats them as if there isn't a plugin on the system for that mime type
<blassey> make sense?
<jimm> yep
<jimm> and so how does your change fix that?
<blassey> it sends over the info for all the plugins
<jimm> ah, the lack of that disabled check?
<blassey> including the diabled ones
<blassey> yup
<jimm> which allows the page to load up a disabled plugin placeholder?
<jimm> gotcha
<blassey> right
Attachment #8632342 - Flags: review?(jmathies) → review+
I think it would be better to continue using GetPlugins (and nsRefPtr!) and pass it a flag asking for disabled plugins. Right now you're leaking the array returned by GetPlugins. Calling NS_RELEASE manually isn't something we should do unless absolutely necessary.
Posted patch includeDisabledPlugins.patch (obsolete) — Splinter Review
the alternative is to just free() the plugins array with the previous patch
Attachment #8632342 - Attachment is obsolete: true
Attachment #8633460 - Flags: review?(jmathies)
Attachment #8633460 - Flags: review?(jmathies) → review+
Attachment #8633460 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1d9aa5e3ba40
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
I tried to re-enable the test for this, browser_plugin_infolink.js, as it worked locally and in try run. But once landed it caused perma-fail on Win8.  So it was disabled again.
You need to log in before you can comment on or make changes to this bug.