Last Comment Bug 743429 - imdb trailers report missing plugin with CtP enabled
: imdb trailers report missing plugin with CtP enabled
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: 14 Branch
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: David Keeler [:keeler] (use needinfo?)
:
Mentors:
http://www.imdb.com
Depends on: 749257
Blocks: 711552 711618 click-to-play
  Show dependency treegraph
 
Reported: 2012-04-07 01:45 PDT by annaeus
Modified: 2012-04-26 10:42 PDT (History)
11 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot.png (213.48 KB, image/png)
2012-04-07 01:45 PDT, annaeus
no flags Details
proposed fix (4.80 KB, patch)
2012-04-11 14:19 PDT, David Keeler [:keeler] (use needinfo?)
jaws: feedback+
Details | Diff | Review
fix v2 (3.06 KB, patch)
2012-04-11 16:14 PDT, David Keeler [:keeler] (use needinfo?)
jaas: review+
jaws: feedback+
Details | Diff | Review
fix - rebased (3.12 KB, patch)
2012-04-25 12:39 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
Details | Diff | Review

Description annaeus 2012-04-07 01:45:55 PDT
Created attachment 613077 [details]
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 2012-04-07 02:15:12 PDT
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 annaeus 2012-04-11 10:00:59 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-04-11 14:19:54 PDT
Created attachment 614169 [details] [diff] [review]
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-04-11 15:43:20 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-04-11 15:48:07 PDT
(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 David Keeler [:keeler] (use needinfo?) 2012-04-11 16:14:13 PDT
Created attachment 614214 [details] [diff] [review]
fix v2
Comment 7 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-04-11 21:18:15 PDT
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 2012-04-12 06:54:46 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-04-12 11:34:51 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-04-12 14:26:41 PDT
(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 David Keeler [:keeler] (use needinfo?) 2012-04-25 10:54:37 PDT
Here's the try run, btw: https://tbpl.mozilla.org/?tree=Try&rev=65e9042cc3c9
Comment 12 David Keeler [:keeler] (use needinfo?) 2012-04-25 12:39:55 PDT
Created attachment 618395 [details] [diff] [review]
fix - rebased

Carrying over r+ for rebase.
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-04-25 17:14:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/81736e2ab216

Should there be a test for this?
Comment 14 David Keeler [:keeler] (use needinfo?) 2012-04-26 09:42:35 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2012-04-26 09:52:19 PDT
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 [:bsmedberg] 2012-04-26 10:05:42 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-04-26 10:10:09 PDT
(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 David Keeler [:keeler] (use needinfo?) 2012-04-26 10:23:17 PDT
I filed bug 749257 to improve the test plugin.
Comment 19 Ed Morley [:emorley] 2012-04-26 10:42:08 PDT
https://hg.mozilla.org/mozilla-central/rev/81736e2ab216

Note You need to log in before you can comment on or make changes to this bug.