739048 - "Tap here to activate plugin" placeholder shown for unsupported plugin content

Status: VERIFIED FIXED

Description

[Martijn Wargers (dead)]

Created attachment 609104 [details]
testcase

You need to have Plugins set to "Tap to play" in the settings for this bug.
Tested on the Galaxy Nexus, Android 4.0.2, using latest Fennec Native build.

Steps to reproduce:
- Visit testcase

Expected result:
- "A plugin is needed to display this content" placholder on the page

Actual result:
- "Tap here to activate plugin" placeholder on the page, which doesn't do anything when being tapped upon

Comment 1

[Aaron Train [:aaronmt]]

This sounds like a general core plugin-container click-to-play issue. Does this happen on Firefox desktop too?

Comment 2

[Martijn Wargers (dead)]

I don't know, I don't have a click-to-play desktop build. Since when is click-to-play working on desktop?

Comment 3

[:Margaret Leibovic]

When I tested this myself, after tapping the "Tap here to activate plugin" overlay, the overlay message switched to the "A plugin is needed to display this content" message. I was discussing this with Jared, and it seems this is happening because we need to try to activate the plugin object before we can know if the appropriate plugin type is installed.

We're using the :-moz-state-unsupported pseduo-class to decide whether or not to show the unsupported plugin overlay, and that's determined by the NS_EVENT_STATE_TYPE_UNSUPPORTED state. Following the code paths through MXR, it looks like we're returning NS_ERROR_PLUGIN_CLICKTOPLAY before we check to see if the plugin is actually missing:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#490

So we always end up in the click to play state instead of the unsupported state.

Josh, I was wondering if you knew the right way to go about fixing this issue. Can we just switch around nsObjectLoadingContent::IsPluginEnabledForType to check pluginHost->IsPluginEnabledForType before jumping to conclusions about click to play?

Moving this over to Core->Plugins, since it will affect desktop as well.

Comment 4

[Josh Aas]

(In reply to Margaret Leibovic [:margaret] from comment #3)

> Josh, I was wondering if you knew the right way to go about fixing this
> issue. Can we just switch around
> nsObjectLoadingContent::IsPluginEnabledForType to check
> pluginHost->IsPluginEnabledForType before jumping to conclusions about click
> to play?

If that works and doesn't break anything on try it seems fine to me.

Comment 5

[:Margaret Leibovic]

Created attachment 609756 [details] [diff] [review]
patch

I'll push to try, but this patch was working locally for me. Following the idea in my last comment, it checks to see if the plugin is enabled before returning NS_ERROR_PLUGIN_CLICKTOPLAY.

I also changed a |return false| in there to |return NS_ERROR_FAILURE|, since false isn't a nsresult.

Comment 6

[:Margaret Leibovic]

https://tbpl.mozilla.org/?tree=Try&rev=b1bc9153cd4c

Comment 7

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

Comment on attachment 609756 [details] [diff] [review]
patch

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +489,5 @@
> +  nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID));
> +  nsPluginHost *pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());
> +  if (!pluginHost) {
> +    return NS_ERROR_FAILURE;
> +  }

NS_ENSURE_TRUE(pluginHost, NS_ERROR_FAILURE);

Comment 8

[Josh Aas]

Comment on attachment 609756 [details] [diff] [review]
patch

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

This is perfectly functional as far as I can tell but I would like to see the revision before it goes in.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +489,5 @@
> +  nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID));
> +  nsPluginHost *pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());
> +  if (!pluginHost) {
> +    return NS_ERROR_FAILURE;
> +  }

No, leave it the way you have it in this patch. That's how we do the checks in plugin code. I don't like NS_ENSURE_TRUE here, it seems too much like we're intending to check for an actual boolean TRUE value as opposed to non-NULL.

@@ +492,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsresult rv = pluginHost->IsPluginEnabledForType(aMIMEType.get());
> +  if (NS_SUCCEEDED(rv) && !mShouldPlay) {

I don't like these in the same if statement, it seems overly clever. Please check for NS_FAILED here and return rv if it failed, then check mShouldPlay if it succeeded. The logic is more clear that way.

Also, please add a comment indicating that the order of operations matters here so that we don't mess it up again in the future.

@@ -492,5 @@
>  
> -  nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID));
> -  nsPluginHost *pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());
> -  if (!pluginHost) {
> -    return false;

Nice catch!

Comment 9

[:Margaret Leibovic]

Created attachment 609926 [details] [diff] [review]
patch v2

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15a4628f412e

Comment 11

[Marco Bonardo [::mak]]

https://hg.mozilla.org/mozilla-central/rev/15a4628f412e

Comment 12

[Martijn Wargers (dead)]

Verified fixed in today's trunk build on the Samsung Galaxy SII.

Comment 13

[:Margaret Leibovic]

Comment on attachment 609926 [details] [diff] [review]
patch v2

[Approval Request Comment]
Mobile only. Beta blocker.

Comment 14

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #13)
> Comment on attachment 609926 [details] [diff] [review]
> patch v2
> 
> [Approval Request Comment]
> Mobile only. Beta blocker.

Er, mass approval request fail. This is platform code, but it should only affect applications where click to play plugins are enabled, which for now is just mobile.

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 609926 [details] [diff] [review]
patch v2

[Triage Comment]
No risk to desktop Aurora because click-to-play is mobile only for now. Approved for Aurora 13.