Closed Bug 1160166 Opened 9 years ago Closed 9 years ago

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


(Core Graveyard :: Plug-ins, defect)

Not set


(e10sm8+, firefox40 affected, firefox42 fixed)

Tracking Status
e10s m8+ ---
firefox40 --- affected
firefox42 --- fixed


(Reporter: jimm, Assigned: blassey)




(1 file, 2 obsolete files)

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


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[";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];
  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: --- → ?
Assignee: nobody → mconley
Assignee: mconley → jmathies
Attached patch getAllPlugins.patch (obsolete) — Splinter Review
Assignee: jmathies → blassey.bugs
Attachment #8632342 - Flags: review?(jmathies)
Comment on attachment 8632342 [details] [diff] [review]

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

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


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

How does switching from GetPlguins to GetPlguinTags fix this bug?

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]

<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.
Attached 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
Closed: 9 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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.