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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

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.

Comment 1

[Jim Mathies [:jimm]]

In content the plugin disabled ui doesn't display either.

Comment 3

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

Attachment: getAllPlugins.patch

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

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

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.

Comment 6

[Jim Mathies [:jimm]]

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0e127c7f69c4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for e10s LSAN leaks.
https://treeherder.mozilla.org/logviewer.html#?job_id=3738107&repo=fx-team
https://treeherder.mozilla.org/logviewer.html#?job_id=3738112&repo=fx-team
https://treeherder.mozilla.org/logviewer.html#?job_id=3738108&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/61df86ca9b50

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 10

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

Attachment: includeDisabledPlugins.patch

the alternative is to just free() the plugins array with the previous patch

Comment 11

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

pushed the previous patch with free(plugins) to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a3a3d8d0897

new patch here https://treeherder.mozilla.org/#/jobs?repo=try&revision=a10f217d678d

Comment 12

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

Attachment: patch for checkin

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d9aa5e3ba40

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d9aa5e3ba40

Comment 15

[Tracy Walker [:tracy]]

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.