743646 - Plugin doorhanger appears for unsupported plugin embeds (and hidden embeds that are enabled by default)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

You need to have the Setting for Plugins set to "Tap to Play" to see this bug.

See testcase, the invisible embeds have type=unknown/unknown, so Fennec can't support that plugin.
When visiting that page however, I get to see the plugin doorhanger that asks me if I would like to play that plugin. That doesn't make sense for plugin content that isn't supported.

Comment 1

[:Margaret Leibovic]

Bug 739048 should prevent us from firing PluginClickToPlay events for unsupported content, but it looks like this is happening because we're not actually checking if a PluginClickToPlay event fired before deciding to show the doorhanger, only checking if there are plugins:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1826

This also means that we're probably showing the doorhanger for hidden plugins even when plugins are enabled. Martijn, do you happen to know if a bug was filed for that?

Comment 2

[Martijn Wargers (dead)]

(In reply to Margaret Leibovic [:margaret] from comment #1)
> This also means that we're probably showing the doorhanger for hidden
> plugins even when plugins are enabled. Martijn, do you happen to know if a
> bug was filed for that?

I think this is bug 741134.

Comment 3

[:Margaret Leibovic]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #2)
> (In reply to Margaret Leibovic [:margaret] from comment #1)
> > This also means that we're probably showing the doorhanger for hidden
> > plugins even when plugins are enabled. Martijn, do you happen to know if a
> > bug was filed for that?
> 
> I think this is bug 741134.

That's about when plugins have been enabled for a specific site - I'm talking about the global setting here. I was able to reproduce this problem by changing plugins to enabled in the settings menu, then visiting http://people.mozilla.com/~mwargers/tests/plugins/flash/flashembed_invisible.html.

Comment 4

[Johnathan Nightingale [:johnath]]

Wouldn't hold the release for it, but would take a safe fix

Comment 5

[:Margaret Leibovic]

I think we really need to fix this, since this means we're showing this doorhanger even when plugins are enabled by default. What we need to do is make sure a PluginClickToPlay event has fired before deciding to show the doorhanger in the load event.

In bug 730318, we decided to stop keeping track of plugins that we saw through the PluginClickToPlay event, and it would be nice to avoid having to rely on that again. Jared, have you found a way to deal with this problem in your desktop patches?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, on desktop we no longer use the load event, and do all of the calculations when we receive a PluginClickToPlay event.

This has a negative side effect that the first PluginClickToPlay event would determine if the doorhanger should show or not show. Given that, we decided to always show the doorhanger but in a dismissed state.

Comment 7

[:Margaret Leibovic]

(In reply to Jared Wein [:jaws] from comment #6)
> Yeah, on desktop we no longer use the load event, and do all of the
> calculations when we receive a PluginClickToPlay event.
> 
> This has a negative side effect that the first PluginClickToPlay event would
> determine if the doorhanger should show or not show. Given that, we decided
> to always show the doorhanger but in a dismissed state.

Right now we don't have UI for dismissed doorhangers on mobile, but I think that potentially showing a notification even when there's a placeholder is okay, since the user can now choose to set permissions to never see a doorhanger on a site again.

Also, I'm updating the summary to indicate the full scope of this problem.

Comment 8

[:Margaret Leibovic]

Attachment: patch

This patch gets rid of the load listener like Jared suggested, making the PluginClickToPlay event handler the only place that we decide to show the doorhanger. This means that we'll never accidentally show the doorhanger for non-click-to-play plugins.

The downside of this approach is that we will show a notification whenever there is hidden plugin content on the page, even if there are visible "tap to play" overlays. However, like I mentioned before, I don't think this is a big problem, since users can choose to set permissions to never see the doorhanger again for a site.

Comment 9

[Martijn Wargers (dead)]

(In reply to Margaret Leibovic [:margaret] from comment #3)
> > I think this is bug 741134.
> 
> That's about when plugins have been enabled for a specific site - I'm
> talking about the global setting here. I was able to reproduce this problem

Sorry, you're right. I don't think I filed a bug for that, but I guess you've included that case in this bug now.

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a47a875dc0

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/d9a47a875dc0

Comment 12

[Martijn Wargers (dead)]

Verified fixe on a 2012-04-14 trunk build on the Samsung Galaxy Nexus.

Comment 13

[Martijn Wargers (dead)]

Also verified that the str in comment 3 are working now.