imdb trailers report missing plugin with CtP enabled

RESOLVED FIXED in mozilla15

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: annaeus, Assigned: keeler)

Tracking

14 Branch
mozilla15
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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/
(Reporter)

Updated

5 years ago
Blocks: 711552
(Reporter)

Updated

5 years ago
Blocks: 711618, 738698, 743312
No longer blocks: 711552
(Reporter)

Updated

5 years ago
(Reporter)

Updated

5 years ago
Blocks: 711552
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

5 years ago
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
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.
(Assignee)

Updated

5 years ago
Attachment #614169 - Flags: feedback?(jwein)
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?
Attachment #614169 - Flags: feedback?(jwein) → feedback+
(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.
Created attachment 614214 [details] [diff] [review]
fix v2
Attachment #614169 - Attachment is obsolete: true
Attachment #614214 - Flags: feedback?(jwein)
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 :)
Attachment #614214 - Flags: feedback?(jwein) → feedback+
(Assignee)

Updated

5 years ago
Attachment #614214 - Flags: review?(joshmoz)

Updated

5 years ago
Assignee: nobody → dkeeler

Comment 8

5 years ago
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.
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.
(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).
No longer blocks: 743312

Updated

5 years ago
Attachment #614214 - Flags: review?(joshmoz) → review+
Component: Untriaged → Plug-ins
Product: Firefox → Core
QA Contact: untriaged → plugins
Here's the try run, btw: https://tbpl.mozilla.org/?tree=Try&rev=65e9042cc3c9
Created attachment 618395 [details] [diff] [review]
fix - rebased

Carrying over r+ for rebase.
Attachment #614214 - Attachment is obsolete: true
Attachment #618395 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/81736e2ab216

Should there be a test for this?
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla15
(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?
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

5 years ago
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?
(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).
I filed bug 749257 to improve the test plugin.
Depends on: 749257
https://hg.mozilla.org/mozilla-central/rev/81736e2ab216
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.