537998 - Add basic plugin blocking feature

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: wip 1

Add a plugin blocker that acts like flashblock and other add-ons, which replace the plugin (<object> and <embed>) with a place holder. If the user clicks the place holder, the plugin is activated and displayed.

Basic WIP patch

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

One notable problem with this patch clicking to activate the plugin does not always force the canvas to redraw. Don't know what the problem with that is yet.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: wip 2

This patch moves most of the hardcoded style code into a CSS file

Comment 3

[Madhava Enros [:madhava]]

Attachment: mockup of what a blocked plugin would look like

I'll attach the "play" icon in then next comment.

The plugin icon in the lower right is this one:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/mozapps/plugins/pluginGeneric.png

The background grey is the one we use for the background color on the "See all bookmarks" row in the awesomebar (which should be the same as our section dividers in lists).

The outer border line is black.

Comment 4

[Madhava Enros [:madhava]]

Attachment: the plugin "play" icon

try this one out

Comment 5

[Justin Dolske [:Dolske]]

(In reply to comment #0)
> Created an attachment (id=420151) [details]
> wip 1

So, one downside of this approach is that people will inevitably treat this as a security mechanism, which means it needs to worry about the possibility of a malicious page activating its own plugins. (eg, <plugin allowplugin="true">, via reaching into the anonymous content, dispatching fake click events...).

Over in bug 538910 I mentioned it might be interesting to look at how <video> has it's own native anonymous content for the video controls, which largely prevents content from touching them. [Although that was done more to prevent web-compat headaches than as a security mechanism.] Doing something similar for "plugin controls" might allow us to more securely deal with this.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch adds events and will block and unblock on Maemo. It does cause a crash though. I am investigating.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

with this patch, I went to espn.com and got the following crash stack:

Program received signal SIGSEGV, Segmentation fault.
0x412e3a50 in g_object_get_qdata () from /usr/lib/libgobject-2.0.so.0
0x412e3a50 <g_object_get_qdata+28>:     ldr     r3, [r4]
(gdb) bt
#0  0x412e3a50 in g_object_get_qdata () from /usr/lib/libgobject-2.0.so.0
#1  0x41de2660 in gtk_widget_get_colormap () from /usr/lib/libgtk-x11-2.0.so.0
#2  0x41de2718 in gtk_widget_get_visual () from /usr/lib/libgtk-x11-2.0.so.0
#3  0x4b13000c in ?? () from /usr/lib/browser/plugins/libflashplayer.so
Cannot access memory at address 0x8
(gdb)

is the sort of stack you're seeing?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

This patch adds a background image and styling to the blocked state div

The patch still holds onto the blocked flash object, but that doesn't seem to cause any crashes (that I saw).

Also, we still use the simple "allowplugin" attribute as well. Ideas to make this better are welcome.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Another thing I noticed about this patch was when we disable plugins, the blocker still shows up for some objects. Clicking the object makes it all disappear, since the plugin is disabled.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 3

This patch changes to SVG for the 'play' indicator so we don't get scaling artifacts. This patch also checks the "PluginBlocked" event to see if we should block the plugin or not. This allows the frontend to disable plugin blocking.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 3 (real)

forgot to hg qref

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 4

Added code to disable the blocker if plugins are disabled.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 5

Same as before but I use a more robust way of finding the width and height of the plugin. 100% was breaking me. Also, I add !important to some of the style rules.

Both were found on youtube

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

>diff --git a/chrome/content/bindings/plugin.xml b/chrome/content/bindings/plugin.xml
>new file mode 100644
>--- /dev/null
>+++ b/chrome/content/bindings/plugin.xml
>@@ -0,0 +1,97 @@
>+<?xml version="1.0"?>
>+
>+<bindings
>+    xmlns="http://www.mozilla.org/xbl"
>+    xmlns:html="http://www.w3.org/1999/xhtml"
>+    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+
>+  <binding id="plugin-blocker">
>+    <implementation>
>+      <constructor>
>+      <![CDATA[
>+        let plugin = this;
>+        if (plugin.hasAttribute("allowplugin"))
>+          return;
>+

So, i'm not a fan but as you said, blassey's patch should resolve this.
By the way why are we not creating real anonymous content like that:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsVideoFrame.cpp#120?

After a quick look at the plugin code i'm not really sure where to put that,though.

>+        let event = document.createEvent("UIEvents");
>+        event.initUIEvent("PluginBlocked", true, true, window, 1);
>+        plugin.dispatchEvent(event);
>+        if (event.getPreventDefault() == true)
>+          return;

why not just 
let canceled = plugin.dispatchEvent(event);
if (canceled)
  return;


>+
>+        let parent = this.parentNode;
>+        let placeholder = document.createElementNS("http://www.w3.org/1999/xhtml", "div");
>+
>+        // Build the 'Play' image as SVG
>+        let svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");
>+        let circle = document.createElementNS("http://www.w3.org/2000/svg", "circle");
>+        circle.setAttribute("cx", "24");
>+        circle.setAttribute("cy", "24");
>+        circle.setAttribute("r", "22");
>+        circle.setAttribute("fill", "none");
>+        circle.setAttribute("stroke", "#32343A");
>+        circle.setAttribute("stroke-width", "3.725");
>+        let triangle = document.createElementNS("http://www.w3.org/2000/svg", "polygon");
>+        triangle.setAttribute("points", "34,24 18,14 18,34");
>+        triangle.setAttribute("fill", "#32343A");
>+        triangle.setAttribute("stroke", "#32343A");
>+        triangle.setAttribute("stroke-width", "0.5");
>+        svg.appendChild(circle);
>+        svg.appendChild(triangle);
>+        placeholder.appendChild(svg);

can't we save this image once and displayed it here?
btw, I'm not sure to understand why we need a svg image here?

>+
>+        // Initialize the placeholder to match the plugin
>+        placeholder.setAttribute("class", "plugin-blocker " + plugin.getAttribute("class"));
>+
>+        let style = window.getComputedStyle(plugin, "");
>+        let width = 32;
>+        if (!plugin.width || plugin.width.match("%$"))
>+          width = parseInt(style.getPropertyValue("width"));
>+        else
>+          width = parseInt(plugin.width);
>+        placeholder.style.width = width + "px";
>+

if we're going to be allow rotation (portrait mode) (and so to change the page width/height), or if there is some dynamically loaded content what's happened here with fixed size instead of "%"?

>+        let height = 32;
>+        if (!plugin.height || plugin.height.match("%$"))
>+          height = parseInt(style.getPropertyValue("height"));
>+        else
>+          height = parseInt(plugin.height);
>+        placeholder.style.height = height + "px";

idem



>+        function _unblock() {

Maybe we should test aEvent.isTrusted here?


> 
>+// Handle disabling the plugin blocker feature
>+var PluginBlocker = {
>+  handleEvent: function handleEvent(aEvent) {
>+    if (aEvent.type == "PluginBlocked") {
>+      try {
>+        let value = gPrefService.getBoolPref("plugins.enabled");
>+        if (!value)
>+          aEvent.preventDefault();

replace "value" by "enabled"


>+      // Make sure the plugin has been unblocked
>+      if (!objects[i].hasAttribute("allowplugin"))
>+        continue;
>+

Should we not test for the value here?

>diff --git a/chrome/content/content.css b/chrome/content/content.css
>--- a/chrome/content/content.css
>+++ b/chrome/content/content.css

>+/* Block plugins */
>+object,
>+embed
>+{
>+  -moz-binding: url("chrome://browser/content/bindings/plugin.xml#plugin-blocker") !important;
>+}

do we want that for "applet" too?

>+
>+/* Placeholder for blocked plugins */
>+div.plugin-blocker {
>+  cursor: pointer;

do we need "cursor" here?

>+  display: inline-block;
>+  visibility: visible !important;
>+  overflow: hidden; /* makes baseline vertical-alignment act like plugins */
>+  background: rgba(230,230,230,.8) !important;
>+  border: 1px solid #32343A !important;
>+  -moz-border-radius: 8px !important;
>+  -moz-box-sizing: border-box;
>+}



Also I'm not sure if that's me or not but sometimes I need to click twice on the arrow to see the plugin-blocker div dissapears.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #14)

> >+        let event = document.createEvent("UIEvents");
> >+        event.initUIEvent("PluginBlocked", true, true, window, 1);
> >+        plugin.dispatchEvent(event);
> >+        if (event.getPreventDefault() == true)
> >+          return;
> 
> why not just 
> let canceled = plugin.dispatchEvent(event);
> if (canceled)
>   return;

Perfect.

> 
> can't we save this image once and displayed it here?
> btw, I'm not sure to understand why we need a svg image here?

If we use a PNG the image is scaled badly. Remember, the image here is injected into web content, which is then zoomed (scaled) on to the canvas tiles.

Here is an example of what the PNG looked like:
http://www.flickr.com/photos/finkle/4293308509/in/photostream/

And now the SVG:
http://www.flickr.com/photos/finkle/4295538011/in/photostream/

> >+        let style = window.getComputedStyle(plugin, "");
> >+        let width = 32;
> >+        if (!plugin.width || plugin.width.match("%$"))
> >+          width = parseInt(style.getPropertyValue("width"));
> >+        else
> >+          width = parseInt(plugin.width);
> >+        placeholder.style.width = width + "px";
> >+
> 
> if we're going to be allow rotation (portrait mode) (and so to change the page
> width/height), or if there is some dynamically loaded content what's happened
> here with fixed size instead of "%"?

Remember, the image and the <div> are plaved in the web content. The size of the web content does not change when landscape / portrait change. Only when the width of the entire <browser> is changed.

That said, we could look for a better way to handle "%" width and height, also making sure the SVG image is centered.


> >+        function _unblock() {
> 
> Maybe we should test aEvent.isTrusted here?

Hmm, maybe?

> >+        let value = gPrefService.getBoolPref("plugins.enabled");
> >+        if (!value)
> >+          aEvent.preventDefault();
> 
> replace "value" by "enabled"

OK

> >+      // Make sure the plugin has been unblocked
> >+      if (!objects[i].hasAttribute("allowplugin"))
> >+        continue;
> >+
> 
> Should we not test for the value here?

No need. We don't care about allowplugin="false". We are following more HMTL-like rules where the presence of allowplugin means "true".

> >+/* Block plugins */
> >+object,
> >+embed
> >+{
> >+  -moz-binding: url("chrome://browser/content/bindings/plugin.xml#plugin-blocker") !important;
> >+}
> 
> do we want that for "applet" too?

I guess we could. No objection from me.

> >+div.plugin-blocker {
> >+  cursor: pointer;
> 
> do we need "cursor" here?

Nope. I'll remove

> Also I'm not sure if that's me or not but sometimes I need to click twice on
> the arrow to see the plugin-blocker div dissapears.

I have seen that too. I'm not sure why.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 423081 [details] [diff] [review]
patch 5

>diff --git a/chrome/content/bindings/plugin.xml b/chrome/content/bindings/plugin.xml

>+  <binding id="plugin-blocker">

>+        let style = window.getComputedStyle(plugin, "");
>+        let width = 32;
>+        if (!plugin.width || plugin.width.match("%$"))
>+          width = parseInt(style.getPropertyValue("width"));
>+        else
>+          width = parseInt(plugin.width);
>+        placeholder.style.width = width + "px";
>+
>+        let height = 32;
>+        if (!plugin.height || plugin.height.match("%$"))
>+          height = parseInt(style.getPropertyValue("height"));
>+        else
>+          height = parseInt(plugin.height);
>+        placeholder.style.height = height + "px";

This seems quite hacky, but I don't have any better ideas offhand :/

>+        // Make some style adjustments since the placeholder is a <div>

Can you avoiding needing this by using a <span> rather than a <div>?

>+        parent.insertBefore(placeholder, plugin);
>+        parent.removeChild(plugin);

This makes me nervous. Ask bz how bad this is? (binding removing its node from the document from within its constructor)

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

> > Also I'm not sure if that's me or not but sometimes I need to click twice on
> > the arrow to see the plugin-blocker div dissapears.
> 
> I have seen that too. I'm not sure why.


I think i've found why, on a lot of sites the structure is <object><embed/></object> so we create 2 pluginblocker bindings for them.
As an example:
 http://www.gazbming.com/home.php


Other little nit, when doing 
+        placeholder.setAttribute("class", "plugin-blocker " + plugin.getAttribute("class"));


if the class is null, we'll add that at the end of the class.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 6

This patch adds vivien and gavin comments to the code:
* changed to <span> and removed <div> adjustment code
* value -> enabled
* uses return value from event.dispatchEvent
* removes cursor from CSS
* adds a event.isTrusted check
* changes from "click" to "mousedown" to fix the "needs two clicks" issue

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 7

This patch:
* makes the SVG image centered on 0, 0 - so it's easier to center it in the span
* uses the <param> list to look for width / height fallbacks (nytimes video player)
* checks the plugin's class name before appeanding it to the span's class

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 8

This patch:
* changes the CSS to not bind the xbl if the "allowplugin" attribute is present. this makes the "don't block plugins" state cleaner and faster.
* adds a delay hack for plugins that are document.write added and not quite ready when the xbl constructor fires (device is too slow I guess - works ok on desktop)
* adds a cache for the plugin source URL so we can blank it out when blocked and reset it when unblocked
* changes browser.js to support a tri-sate plugin mode (disabled=0, blocked=1, enabled=2) even though we only support 1 & 2 for now
* removed old temporary plugin preference cruft code
* made "plugins.enabled" deprecated and used as a flag for old profiles
* we now force all plugins to not be disabled in old profiles

Tested on nytimes, espn, fandango, cnn, youtube and http://www.gazbming.com/home.php (for nested <object><embed>)
Tested blocked -> unblocked and enabled (never blocked) modes

All the sites worked. I noticed some flicker on sites that had other content changes, but we can't stop that flicker. If a page does something to fire a MozAfterPaint, any active plugins will flicker. espn.com and fandango are good examples.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: interdiff between patch 6 and patch 7

might be helpful when reviewing

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

test build with patch 7 can be found here:
https://people.mozilla.com/~mfinkle/fennec/fennec-1.0pre.en-US.linux-gnueabi-arm.pluginblock.tar

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 423491 [details] [diff] [review]
patch 8

>+<bindings
>+    xmlns="http://www.mozilla.org/xbl"
>+    xmlns:html="http://www.w3.org/1999/xhtml"
>+    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+
>+  <binding id="plugin-blocker">
>+    <implementation>
>+      <constructor>
>+      <![CDATA[
>+        let plugin = this;
>+        let parent = this.parentNode;
>+
>+        // Don't block a nested <embed>. Blocking the outer <object> is enough
>+        if (plugin.localName == "embed" && parent.localName == "object")
>+          return;

Can't we add this rule in css instead of checking it here?

/* Block plugins */
object:not([allowplugin]),
:not(object) > embed:not([allowplugin]),
applet:not([allowplugin]) {
  -moz-binding: url("chrome://browser/content/bindings/plugin.xml#plugin-blocker") !important;
}

This should work

>+
>+        // Hold onto a nested inner <embed>
>+        let embeds = plugin.getElementsByTagName("embed");
>+        if (plugin.localName == "object" && embeds.length)
>+          plugin.embed = embeds[0];

urgh. Maybe that's the reason why you're not doing it in css?
Is there really some cases where an <object> tag can have multiple <embed> nested?


>+
>+        let event = document.createEvent("UIEvents");
>+        event.initUIEvent("PluginBlocked", true, true, window, 1);
>+        let canceled = plugin.dispatchEvent(event);
>+        if (!canceled) {
>+          // Blocking is not needed, so unblock and get out
>+          plugin.setAttribute("allowplugin", "true");
>+          if (plugin.embed)
>+            plugin.embed.setAttribute("allowplugin", "true");
>+          return;
>+        }
>+
>+        let placeholder = document.createElementNS("http://www.w3.org/1999/xhtml", "span");
>+
>+        // Cache the source URL and set the plugin URL to a blank resource
>+        let uri = plugin.getAttribute("src") || plugin.getAttribute("movie") || plugin.getAttribute("data");
>+	if (uri) {

a Tab! :)


r+ with nits.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 423491 [details] [diff] [review]
patch 8

>diff --git a/app/mobile.js b/app/mobile.js

>-pref("plugins.enabled", true);
>+/* 0=disabled, 1=blocked, 2=enabled */
>+pref("browser.plugins.mode", 1);

Seems like we should just stick to the pref values we need rather than try to predict the pref name/values we'll want once this is all in the core. We're likely going to need to migrate anyways, so might as well just keep it simple.

>diff --git a/chrome/content/bindings/plugin.xml b/chrome/content/bindings/plugin.xml

>+<bindings
>+    xmlns="http://www.mozilla.org/xbl"

>+    xmlns:html="http://www.w3.org/1999/xhtml"
>+    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

nit: don't need these

>+  <binding id="plugin-blocker">

>+        // Don't block a nested <embed>. Blocking the outer <object> is enough
>+        if (plugin.localName == "embed" && parent.localName == "object")
>+          return;

Use instanceof HTMLEmbedElement/HTMLObjectElement ?

>+        // Cache the source URL and set the plugin URL to a blank resource
>+        let uri = plugin.getAttribute("src") || plugin.getAttribute("movie") || plugin.getAttribute("data");

Where do these come from? I've never heard of "movie", is that just some flash convention?

>+	if (uri) {
>+          let type = null;
>+          if (plugin.hasAttribute("src"))
>+            type = "src";
>+          else if (plugin.hasAttribute("data"))
>+            type = "data";
>+          else if (plugin.hasAttribute("movie"))
>+            type = "movie";

If there are empty attributes this won't do what you want. Might as well put the getAttribute() calls here too.

>+        // Build the 'Play' image as SVG

>+        // Use <param> list as a fallback for figuring out the width and height
>+        let paramW = 0;
>+        let paramH = 0;
>+        let params = plugin.getElementsByTagName("param");
>+        for (var i = 0; i < params.length; i++) {
>+          if (params[i].getAttribute("name") == "width" && params[i].hasAttribute("value"))
>+            paramW = params[i].value
>+          if (params[i].getAttribute("name") == "height" && params[i].hasAttribute("value"))
>+            paramH = params[i].value

Can use param[i].name

>+        // Some plugins are document.write into the page and might not be completely
>+        // loaded at this time. A telltale sign is the need to fallback to <param>
>+        // for width and height. We need to give these plugins a delay when swapping
>+        // or they won't display correctly when clicked.
>+        let needsDelay = false;
>+
>+        // Initialize the placeholder to match the plugin

Both of these blocks of code are pretty gross :( 

>+        if (className)
>+          placeholder.setAttribute("class", "plugin-blocker " + className);
>+        else
>+          placeholder.setAttribute("class", "plugin-blocker");

placeholder.setAttribute("class", "plugin-blocker " + (className || ""));

>+        function _unblock(aEvent) {
>+          if (!aEvent.isTrusted)
>+            return;

This is running in content, right? I think this means this check isn't very useful.

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+    // Force all plugins enabled. Remove this when tri-state plugin state is supported
>+    if (gPrefService.prefHasUserValue("plugins.enabled")) {
>+      // "plugins.enabled" is deprecated and should only exist in pre-1.0 releases
>+      gPrefService.clearUserPref("plugins.enabled");
>       this.setPluginState(true);
>     }

I kinda wonder whether we should call setPluginState(true) unconditionally just to avoid issues.

>+var PluginBlocker = {
>+  handleEvent: function handleEvent(aEvent) {
>+    if (aEvent.type == "PluginBlocked") {
>+      try {
>+        let mode = gPrefService.getIntPref("browser.plugins.mode");
>+        if (mode != 1)
>+          aEvent.preventDefault();
>+      } catch(e) {}

why try/catch?

> PluginObserver.prototype = {

>   /** Observe listens for plugin change events and maintains an embed cache. */
>   observe: function observe(subject, topic, data) {
>+    if (topic == "nsPref:changed" && data == "browser.plugins.mode") {

I don't see where this observer is set up... Comment is wrong now too. I can't see where anyone notifies for plugin-reflow-event to begin with, so this looks like currently dead code.

This patch makes me really nervous, mostly because it's a rather hacky approach. I think I would be more comfortable landing a patch for bug 542293 than I would be landing this one, even at this stage.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 423491 [details] [diff] [review]
patch 8

I think we are dropping this

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

This is an add-on now.

Comment 27

[Justin Dolske [:Dolske]]

I think this would be good to add to the platform, though, so I've filed bug 549697. :-)