626477 - Block known problematic plugins

Status: RESOLVED FIXED

(Camino Graveyard :: Plug-ins, defect)

Description

[Stuart Morgan]

Bug 387285 gives us the ability to block plugins that are known to be bad, so we should hit some of the biggest offenders for 2.1.

Smokey's comment from that bug:
> So, I'd definitely block Flash 9 and the old RealPlayer version (if we can
> figure out the version number associated with the plug-in was).  And I agree
> about the F4M versions; we should block everything older than 2.3.6.5, as long
> as 2.3.6.5 appears to work fine on 10.4 (one of the two reasons we switched
> back to 2.2.3.7 was that it worked better on 10.4 than the newer ones, which
> introduced the hanging bug); if not, I'd block everything between 2.3.6.5 and
> 2.2.3.7, and then everything older than 2.2.3.7, so that we leave 2.2.3.7
> available for 10.4.
> 
> We still see startup crashes (they spiked again recently, though not nearly as
> high as when I filed the bug) due to the Facebook plug-in (bug 548197), so it
> might be worth blocking it (or it might not).

Comment 1

[Stuart Morgan]

Flip4Mac-wise, the versions we should block are:
- Everything older than 2.2.1 (the first version with the origin-moving bug fixed--technically we could start the block at 2.0.2, but that gets into versions so old I don't care).
- 2.3.0-2.3.5 (the range where all loading was synchronous)
That's a simple enough set that I'm going to just block those; we should generally aim for minimal sets for blocks when it's not too much trouble, since the user can't do anything about them (well, not without hand-editing plugin-reg.dat).

Comment 2

[Stuart Morgan]

Flash-wise, what's the thinking for what we block? If we are blocking based on security vulnerabilities, it should be everything before the current version, right? If it's not security, what's the issues with 9 that makes us block it?

Comment 3

[Stuart Morgan]

When we do this, we should change the dtd to add a link to learn more. Right now there's no way to find out what this means or how to fix it, so we should add a page to cb.o explaining each plugin we block and how to fix it (which will generally just be "get the latest version")

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> what's the issues with 9 that makes us block it?

Leak the world, and we removed world-leaking protection.

(In reply to comment #3)
> When we do this, we should change the dtd to add a link to learn more.

For sure.

Comment 5

[Stuart Morgan]

Attachment: Block bad Flash and Flip4Mac

This blocks the plugins discussed above. It also tweaks the inline UI:
- Changes "Click here to learn" to just "Learn"
- Adds a link for blocked plugins (we'll need another section on that cb.o page)
- Changes the explanation of blocking to being about "stability" rather than "protection" since that's how we're using it.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 504634 [details] [diff] [review]
Block bad Flash and Flip4Mac

>diff --git a/geckochrome/locale/en-US/mozapps/plugins/plugins.dtd b/geckochrome/locale/en-US/mozapps/plugins/plugins.dtd
>index bf025de..6011e5b 100644
>--- a/geckochrome/locale/en-US/mozapps/plugins/plugins.dtd
>+++ b/geckochrome/locale/en-US/mozapps/plugins/plugins.dtd
>@@ -20,12 +20,12 @@
> <!ENTITY pluginWizard.finalPage.moreInfo.label               "Find out more about Plugins or manually find missing plugins.">
> <!ENTITY pluginWizard.finalPage.restart.label                "&brandShortName; needs to be restarted for the plugin(s) to work.">
>   
>-<!ENTITY pluginLink.label                                    "here">
>-<!ENTITY missingPlugin1.label                                "No plug-in is availble for this content. Click ">
>-<!ENTITY missingPlugin2.label                                "to learn about available plug-ins.">
>-<!ENTITY disabledPlugin1.label                               "The plug-in for this content has been disabled. Click ">
>-<!ENTITY disabledPlugin2.label                               "to learn about enabling plug-ins.">
>-<!ENTITY blockedPlugin.label                                 "This plug-in has been blocked for your protection.">
>+<!ENTITY missingPlugin.label                                "No plug-in is availble for this content.">
>+<!ENTITY missingPluginLink.label                                "Learn about available plug-ins.">
>+<!ENTITY disabledPlugin.label                               "The plug-in for this content has been disabled.">
>+<!ENTITY disabledPluginLink.label                               "Learn about enabling plug-ins.">
>+<!ENTITY blockedPlugin.label                                 "This plug-in has been blocked for stability.">
>+<!ENTITY blockedPluginLink.label                               "Learn about blocked plug-ins.">

Nit: please fix the spacing on these; the Plugin.labels (except blocked) need an extra space, and the PluginLink.labels have three, three and two extra spaces that need to be removed.

* "This plug-in has been blocked for stability." sounds strange to me; "stability reasons" sounds less weird, though still not great.

* Was there a specific reason you went with "Learn about foo" instead of just "Learn more" (I ask wrt the to fact many plug-in regions are small.  It won't help on *really* small ones like http://www.adobe.com/software/flash/about/ or "Java using object and applet tag" on http://browserspy.dk/java.php but it will make a difference elsewhere.

* Also, I think the period looks better outside of the link rather than as part of it.  It's a little crappy l10n-wise to put the period in the XML instead of the .dtd, but we're also hard-coding links in there already (and, at the moment, .dtd files are still not localizable)…

I glanced at the code (and basically could follow the Obj-C part) but did not review it; I trust pink will examine it thoroughly ;)  I did, however, test against Flash 9.0r124, and it was successfully blocked.

So r=ardissone on the dtd/xml parts with the nit fixed and either changes to address or rebuttals of my other comments.

Comment 7

[Stuart Morgan]

"Learn more" works for me; I was just thinking in incremental change mode. I'll re-spin.

Comment 8

[Stuart Morgan]

> * "This plug-in has been blocked for stability." sounds strange to me

Me too, but everything I could come up with that didn't sound strange was substantially longer. I didn't think that "stability reasons" and various other permutations made it enough better to warrant the length. How about "due to instability"?

Comment 9

[Stuart Morgan]

Attachment: Block bad Flash and Flip4Mac, v2

Updated per above.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Not to keep bikeshedding the strings (but hey, they came from Gecko, so they need work, and we mostly punted on them in bug 626142 to get something better landed!), but tonight while looking at philippe's mockups (which all reused the "missing" plug-in string), I started to wonder if that string should use "installed" instead of "available"?

That is, it can sound like there is no plug-in available--anywhere on the Internet--for the content in question, rather than just there being no plug-in installed on the user's system.

We would maybe have to tweak the entire string slightly; "No plug-in is installed for this content." sounds just a little odd, which is maybe why they went with "available" there…

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

One more thing :P

Since we're only using that one block of strings in plugin.dtd, we might want to add our own l10n note that directs localizers to it explicitly; they can safely leave untranslated the entire rest of that file.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #10)
> "missing" plug-in string), I started to wonder if that string should use
> "installed" instead of "available"?
> 
> That is, it can sound like there is no plug-in available--anywhere on the
> Internet--for the content in question, rather than just there being no plug-in
> installed on the user's system.

philippe mentioned to me on irc about the time I made this comment that he had thought the same thing.

I don't yet have a better re-word solution than "No plug-in is installed for this content." though.

Comment 13

[Stuart Morgan]

Installed is fine with me; I'll change it on checkin.

Comment 14

[Stuart Morgan]

Landed as http://hg.mozilla.org/camino/rev/426ecd831f3a (without sr, per discussion with pink).