743429 - imdb trailers report missing plugin with CtP enabled

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[John Doe]

Attachment: screenshot.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120406 Firefox/14.0a1
Build ID: 20120406031222

Steps to reproduce:

Go to imdb and try to play a trailer, either by clicking by clicking the "watch trailer" button or by clicking on a thumbnail.


Actual results:

The "missing add-on" notification appears.


Expected results:

The "click to play" notification should appear.

Clicking "activate all plugins on the page" does work and activates the plugin correctly.

Example : http://www.imdb.com/title/tt0425210/

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

Confirmed, Setting to NEW for now.  Not sure if this is a dupe of bug 743102.  

May be that the title of 743102 should be changed and just lump all the 'need to install plugin' fails under one bug, if there is a subtle difference between each and every plugin that would need separate bugs.  I'll leave it to the triagers to figure that one out.

Comment 2

[John Doe]

I don't know if this has been reported yet but on vimeo the the "click to activate plugins" message which should replace the video doesn't appear. Clicking where the video should be does activate the plugins however, but only after the page is completely loaded. Testcase : http://vimeo.com/39945517

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: proposed fix

When the mime type is "binary/octet-stream" (equivalently, "application/octet-stream"), the code tries to get a more useful mime type based on the file extension and what plugins are installed and enabled. If a plugin is marked click-to-play, no mime type will be returned, and it will appear that there is no plugin for this object. This patch adds functionality to look up the mime type regardless of enabled/disabled/click-to-play status. This way, we get the right mime type and make the right decision regarding click-to-play/disabled/missing etc.

Comment 4

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

Comment on attachment 614169 [details] [diff] [review]
proposed fix

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +800,5 @@
> +      mContentType.EqualsASCII(BINARY_OCTET_STREAM)) {
> +    nsCAutoString ext;
> +    const char* typeFromExt;
> +    GetExtensionFromURI(uri, ext);
> +    nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID));

MOZ_ASSERT(pluginHostCOM);

::: dom/plugins/base/nsPluginHost.cpp
@@ +1577,5 @@
>    return nsnull;
>  }
>  
> +nsresult
> +nsPluginHost::GetPluginTypeFromExtension(const char* aExtension, const char* &aMimeType) {

This is very similar to FindPluginEnabledForExtension, with the only difference being that FindPluginEnabledForExtension checks that the plugin is enabled.

Is it possible to remove the duplication here or make FindPluginEnabledForExtension use this function?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Jared Wein [:jaws] from comment #4)
> 
> This is very similar to FindPluginEnabledForExtension, with the only
> difference being that FindPluginEnabledForExtension checks that the plugin
> is enabled.
> 
> Is it possible to remove the duplication here or make
> FindPluginEnabledForExtension use this function?

As it turns out, there's another section of the code that I think does the correct thing. The problem was it only looked for application/octet-stream. Adding binary/octet-stream should fix it. I'll put a better patch up.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: fix v2

Comment 7

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

Comment on attachment 614214 [details] [diff] [review]
fix v2

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

Please make sure to request review from Josh Aas as he has much better background knowledge on plugins than I do :)

Comment 8

[Josh Aas]

Comment on attachment 614214 [details] [diff] [review]
fix v2

Why does this bug only happen when click-to-play is enabled? Your patch doesn't seem to have anything to do with click-to-play.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Empirically, it seems the code only runs nsObjectLoadingContent::OnStartRequest when click-to-play is enabled. I'm having a hard time figuring out exactly why, because it's on an input stream callback, and I can't find where it gets set up.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to David Keeler from comment #9)
> Empirically, it seems the code only runs
> nsObjectLoadingContent::OnStartRequest when click-to-play is enabled. I'm
> having a hard time figuring out exactly why, because it's on an input stream
> callback, and I can't find where it gets set up.

Ok - I think I figured it out. If the plugin is enabled, AsyncStartPluginInstance() is called. Otherwise (if the plugin is click-to-play or there are other errors), a new nsIChannel is created that loads the url and calls back to OnStartRequest (around nsObjectLoadingContent.cpp:1570-1620).

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Here's the try run, btw: https://tbpl.mozilla.org/?tree=Try&rev=65e9042cc3c9

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: fix - rebased

Carrying over r+ for rebase.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81736e2ab216

Should there be a test for this?

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Ryan VanderMeulen from comment #13)
> Should there be a test for this?

Probably. Do we have a way of testing using plugins other than the built-in test one?

Comment 15

[(not currently active) Ted Mielczarek]

Not really, but you could add a slimmed-down copy of the existing test plugin for use in click-to-play testing, and have it marked as click-to-play in the Mochitest profile. Alternately you could do some funky things like:
* Build two copies of the test plugin with different names, but with different preprocessor defines so they handle different mime types.
* Make the test plugin inspect its own filename to determine what mime types to handle, and make a copy of it with a different name after building it

Comment 16

[Benjamin Smedberg]

How hard is it to mark a plugin click-to-play within the mochitest framework itself? Can we just temporarily mark the testplugin as click-to-play for one test and then reset it?

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #16)
> How hard is it to mark a plugin click-to-play within the mochitest framework
> itself? Can we just temporarily mark the testplugin as click-to-play for one
> test and then reset it?

We actually do that now. The problem with this bug is it involves loading content with a mime type of "binary/octet-stream", and if there isn't a plugin present that can handle it (e.g. flash), it will get marked as unsupported/missing.

For this and other bugs, it would be nice to have (a) more versatile test plugin(s), so I'll file a bug to make that happen (unless there is one already).

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I filed bug 749257 to improve the test plugin.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/81736e2ab216