773761 - Port |Bug 760625 - Use the blocklist to inform click-to-play plugins|

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

From the original bug:
"We want the ability to make vulnerable plugins click-to-play much in the way we have the ability to block plugins via the blocklist right now."

Two new events were introduced which we should handle, PluginVulnerableUpdatable and PluginVulnerableNoUpdate.

Parts to port from http://hg.mozilla.org/mozilla-central/rev/18d69de4ff67:
* changes from mozilla/browser/base/content/{browser-plugins.js,browser.js} to suite/common/bindings/notification.xml (unfortunately no simple copy&paste)
* mozilla/toolkit/themes/winstripe/mozapps/plugins/pluginProblem.css to suite/themes/modern/mozapps/plugins/pluginProblem.css

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: First attempt at patch

This patch catches the events, but does not really work yet. I still need to find out how to "call" the PluginClickToPlay event handler (if this is possible, maybe via dispatchEvent?) or if I need to use another way for this. We probably should backport this patch to -aurora and -beta as the otherwise users might face non-working UI when a plugin has been blocked via the blacklist.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

I'd just create a new method, containing the code of the current PluginClickToPlay event handler, and call that as needed. Simple, eh? :-)

BTW thanks for taking this. I'm a bit short on time lately.

Comment 3

[Philip Chee]

I've just realized that browser_pluginnotification.js is perma-orange because you ported the functionality but you didn't port the changes to the tests.

e.g. Bug 743312 - Implement click-to-play plugins (port FF bug 711552 and bug 744964)

Comment 4

[Frank Wein [:mcsmurf]]

Attachment: Patch

Still needs a bit of tweaking.

Comment 5

[Philip Chee]

Bug 773761 is a followup to Bug 760625.

Comment 6

[Frank Wein [:mcsmurf]]

A few things I noticed (mostly just interesting for me currently):
1. SeaMonkey does not display a info bar when a plugin is out of date. This should actually work, Bug 521159 fixed this already a year ago. I'm not sure why, probably need to open a new bug for this. I tested this by installing Adobe Reader 10.0.1 from ftp://ftp.adobe.com/pub/adobe/reader/win/10.x/ and going to http://plugindoc.mozdev.org/testpages/pdf.html.
2. My code in the PluginVulnerableUpdatable event handler fails to get the update link on in-page plugins (like on the pdf testpage link). Again I'm not sure why, might be a timing problem. I've set breakpoints in the event handler in Venkman to look at this and when I continued execution, the link been set successfully.
"var updateLink = doc.getAnonymousElementByAttribute(plugin, "class",
                  "checkForUpdatesLink");"

Comment 7

[Frank Wein [:mcsmurf]]

(In reply to Frank Wein [:mcsmurf] from comment #6)
Update on 1: Actually Firefox trunk has the problem (it's a problem from my POV at least :). I filed Bug 803491 on this. Maybe a info bar is actually not needed when a plugin is blocked via click-to-play, but at least in Firefox this did not work for me (it did not block the plugin, does this depend on a pref?).

Comment 8

[Frank Wein [:mcsmurf]]

Attachment: Patch

This patch ports Bug 760625 and also the changes from Bug 779662 (without test changes). I'll port the test changes from Bug 779662 (and probably all test changes to the browser_pluginnotification.js file) in a extra patch and/or bug. I'm not sure about my code duplication in the addClickToPlayCallback method, should I maybe modify addLinkClickCallback instead? The code needs a separate code path to check if a link (the "Check for Updates..." link) has been clicked. This is of course the opposite behavior of the addLinkClickCallback function. In that case we don't want to enable the plugin (execute the callback function), but instead open the plugincheck page. Bug 798278 will port/change the wording a bit so that "normal" click-to-play plugins look different from plugins, that have been changed to click-to-play because of vulnerable plugins.
I've tested the patch with an old Adobe Reader and Flash plugin, see Comment 6, first part for where to download an old Adobe Reader plugin and some test page for this.

Comment 9

[Frank Wein [:mcsmurf]]

Comment on attachment 675911 [details] [diff] [review]
Patch

One note on the setupPluginClickToPlay method: This method contains the code from the old PluginClickToPlay method (as the code there is used by the PluginVulnerableNoUpdate and PluginVulnerableUpdatable event handlers, too). Additionally it checks if the plugin area is big enough to display the UI to activate the plugin.

Comment 10

[Frank Wein [:mcsmurf]]

Wanted for SeaMonkey 2.14 as the core bug (Bug 760625) landed in Gecko 16, so SeaMonkey 2.13 already. Without this patch users with outdated plugins will a face a non-working click-to-play plugin GUI. We don't display infobars for outdated plugins (FF also seems to move away from a infobars as those do not seem to be good from a GUI point of view when there is already a better in-content plugin GUI).

Comment 11

[Philip Chee]

Ugh. Yeah. We should really get it into 2.14 ASAP.

Comment 12

[[e-mail]]

Comment on attachment 675911 [details] [diff] [review]
Patch

>+      <method name="addClickToPlayCallback">
>+        <parameter name="linkNode"/>
>+        <parameter name="callback"/>
>+        <body>
>+          <![CDATA[
>+            // XXX just doing (callback)(arg) was giving a same-origin error. bug?
>+            let callbackArgs = Array.slice(arguments, 2);
>+            linkNode.addEventListener("click",
>+                                      function(aEvent) {
>+                                        // Have to check that the target is not the link to update the plugin
>+                                        if (aEvent.originalTarget instanceof HTMLAnchorElement)
>+                                          return;
>+                                        if (!aEvent.isTrusted)
>+                                          return;
>+                                        aEvent.preventDefault();
>+                                        aEvent.stopPropagation();
>+                                        if (callbackArgs.length == 0)
>+                                          callbackArgs = [ aEvent ];
>+                                        callback.apply(this, callbackArgs);
>+                                      }.bind(this),
>+                                      true);
I think we may have shot ourselves in the foot with the use of capturing here. In many cases we capture events on windows and browsers to protect ourselves, but for instance in the case of leaf elements it makes no difference. If we change addLinkClickCallback to use bubbling then you should find that the link click callback on the plugin update link will fire rather than the plugin activation callback, so you don't need to add a special callback here.

>+            let overlay = doc.getAnonymousElementByAttribute(pluginElement, "class", "mainBox");
>+            /* overlay might be null, so only operate on it if it exists */
>+            if (overlay != null && this.isTooSmall(pluginElement, overlay))
When do we have an overlay, and (more to the point) when don't we?
Nit: overlay && suffices.

>+      <handler event="PluginVulnerableUpdatable" phase="capturing">
>+        <![CDATA[
>+          var plugin = event.target;
>+          var doc = plugin.ownerDocument;
>+
>+          var updateLink = doc.getAnonymousElementByAttribute(plugin, "class", "checkForUpdatesLink");
>+          this.addLinkClickCallback(updateLink, this.openURLPref.bind(this, "plugins.update.url"));
What if an updatable plguin is too small? What if click to play plugins have already been activated?
Also, use this.addLinkClickCallback(updateLink, this.openURLPref, "plugins.update.url");

Comment 13

[Frank Wein [:mcsmurf]]

Attachment: Fix and port tests in browser_pluginnotification.js

I've ported/fixed all tests from the Firefox version of  	browser_pluginnotification.js. I still need to look into what to do about the notification.options.centerActions and notification.options.Actions tests as SeaMonkey does not know about centerActions and Actions. Also in test13b() (ignore the dump()s in there for now :) the "ok(overlay.style.visibility == "hidden", "Test 13b, Plugin should not have visible overlay");" test currently fails. I think this may be a bug in the notifications.xml code.

Comment 14

[Frank Wein [:mcsmurf]]

Comment on attachment 677309 [details] [diff] [review]
Fix and port tests in browser_pluginnotification.js

Regarding the test failure in Comment 13 (test 13b): I filed Bug 807664 on this, we need to port the fix for this from Firefox.

Comment 15

[Frank Wein [:mcsmurf]]

Attachment: Updated patch

Event bubbling seems to do the trick, I've tested all the addLinkClickCallback calls, those still work fine. I've also run the plugin click-to-play tests (see my other patch), currently tests 13b and 18a fail. 13b fail is tracked in Bug 807664, I'm not sure what's happening in 18a ("Update link should open up the plugin check"). I've tested opening a website with a vulnerable, updateble plugin (see Comment 6) and the update link was/is working fine.
About the overlay thing: The overlay can be null when the plugin space is too small and the plugin UI gets set to display=hidden. Then no XBL binding will be attached (see Bug 307098). But after all our code runs inside that XBL binding that does the hiding, so this cannot happen to us?
When an (updatable) plugin is too small, there's still the doorhanger UI next to the favicon to enable plugins. But yes, this won't notify you about there is an update available. Already enabled plugins stay enabled, no matter what the website size changes too. That's fine, after all we don't display any UI for that plugin anymore (until we reload that page or whatever)?

Comment 16

[Frank Wein [:mcsmurf]]

Though, one more thought on the XBL binding: The XBL binding for the plugin UI happens in another file, I'm not sure how those two (the XBL binding for the plugin UI and our notification.xml XBL binding) relate to each other :/

Comment 17

[[e-mail]]

OK, so with Unfocused's help I created a dummy blocklist file which blocklisted all versions of Reader and claimed it was updatable, this then shows a click-to-play placeholder with an extra label "Check for updates...", is this expected UI?

Comment 18

[[e-mail]]

Comment on attachment 675911 [details] [diff] [review]
Patch

>+                                        if (!(aEvent.keyCode == aEvent.DOM_VK_RETURN))
>+                                          return;
You can't see it on the new patch because it doesn't have enough context, but this test is wrong :-(

Comment 19

[Frank Wein [:mcsmurf]]

Neil: See Bug 806136 on this, please comment there why this check is wrong.

Comment 20

[[e-mail]]

Oops, forgot to request needinfo on comment #17 because assignee was wrong.

Comment 21

[Frank Wein [:mcsmurf]]

Neil: Yes, this is expected UI, fixing Bug 798278 would change the wording to explicitly point out to the user that the plugin is vulnerable and should be updated.

Comment 22

[[e-mail]]

Comment on attachment 677928 [details] [diff] [review]
Updated patch

Ah, so we should land this on branches too then?

Comment 23

[Frank Wein [:mcsmurf]]

Yes, it needs to land on comm-beta and comm-aurora, too. But I'll need to test first if it works fine on branches before requesting approval. I'll also see that I can get the automated tests working (see the other attached patch).

Comment 24

[Frank Wein [:mcsmurf]]

BTW: I'll remove the bogus whitespace changes before check-in :)

Comment 25

[Frank Wein [:mcsmurf]]

Pushed to comm-central: https://hg.mozilla.org/comm-central/rev/82a61096e746
Leaving bug open to request comm-aurora and comm-beta approval later.

Comment 26

[Frank Wein [:mcsmurf]]

Attachment: Fix and port tests in browser_pluginnotification.js

This patch adds all the tests from browser_pluginnotification.js and renames a few helper functions to get our version of that file closer to the Firefox one.
I'll attach a diff to the Firefox version so it's easier to see what exactly is different.
I've marked a test in test13b as todo as it's dependent on Bug 807664.
I've modified the tabOpenListener test in test18a. Checking for "event.target.label" would only give us "Loading&" in SeaMonkey, that is why the test would fail in its Firefox version SeaMonkey.
I've commented out the tests 21(a-e) as it depends on notification.options.centerActions, SeaMonkey does not know about this property. This is from the multiple plugin click-to-play UI as far as I see this. But I think it's good to have this test in, then it's easier to enable/test it later :)

The tests pass, but there are two leaks:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/br
owser_pluginnotification.js | leaked until shutdown [nsGlobalWindow #191 chrome:
//mochitests/content/browser/suite/browser/test/plugin_hidden_to_visible.html]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/br
owser_pluginnotification.js | leaked until shutdown [nsGlobalWindow #190 chrome:
//mochitests/content/browser/suite/browser/test/plugin_test.html]

I'm not sure what to do about this, does this mean one the tests causes those leaks?

Comment 27

[Frank Wein [:mcsmurf]]

Attachment: Diff between Firefox version and SeaMonkey version of browser_pluginnotification.js

Comment 28

[Frank Wein [:mcsmurf]]

Hrm, forgot to add proper comments where I commented out notification.options.centerActions, I will add that locally for now.

Comment 29

[Frank Wein [:mcsmurf]]

Attachment: Fix and port tests in browser_pluginnotification.js - Aurora version

Bug 797043 only landed in mozilla-central, so the Aurora version of these tests cannot pass tests test22 and test23. I've removed everything after test20 as test21(a-e) depend on the multiple plugins click-to-play UI.

Comment 30

[Frank Wein [:mcsmurf]]

Attachment: Patch for comm-beta/comm-aurora

Neil: This one is basically easy to review :) it's the same as Attachment 
796039 (which already passed review), but it includes the fix/workaround from Bug 796039. I'll attach an modified test for this as well.

Comment 31

[Frank Wein [:mcsmurf]]

Attachment: Fix and port tests in browser_pluginnotification.js - comm-beta/comm-aurora version

The difference between this patch and Attachment 679838 [details] [diff] is that test12a has been modified the catch the different behavior due to Bug 796039. Also Bug 797043 only landed in mozilla-central, so the comm-beta/comm-aurora version of these tests cannot pass tests test22 and test23. I've removed everything after test20 as test21(a-e) depend on the multiple plugins click-to-play UI.

Comment 32

[Frank Wein [:mcsmurf]]

BTW, on how to run these tests: First apply the patch, then run (py)make in "suite/browser/tests/" and then execute this command in the objdir:
TEST_PATH=suite/browser/test/ python -OO ../../comm-central/mozilla/build/pymake/make.py mochitest-browser-chrome
(I use pymake, so I have to reference it that way)

Comment 33

[[e-mail]]

Comment on attachment 679839 [details] [diff] [review]
Diff between Firefox version and SeaMonkey version of browser_pluginnotification.js

>-  ok(gTestBrowser.missingPlugins.has("application/x-unknown"), "Test 1, Should know about application/x-unknown");
>-  ok(!gTestBrowser.missingPlugins.has("application/x-test"), "Test 1, Should not know about application/x-test");
>+  ok("application/x-unknown" in notificationBox.missingPlugins, "Test 1, Should know about application/x-unknown");
>+  ok(!("application/x-test" in notificationBox.missingPlugins), "Test 1, Should not know about application/x-test");
Eek! This means that I need to port bug 799396. (But that will make the test port smaller because you won't need to count() the elements of the Map.)

Comment 34

[Frank Wein [:mcsmurf]]

Neil: The count(...) thing is already included in the SeaMonkey tests :) (of course we can remove it then when that bug is ported). See my other patch for what I changed.

Comment 35

[[e-mail]]

The point is that bug 799396 broke SeaMonkey, not that it broke the tests...

Comment 36

[Frank Wein [:mcsmurf]]

Attachment: Fix and port tests in browser_pluginnotification.js

Bug 810447 was fixed and changed some test code, which the previous patch touched, too. I've updated the patch to apply again and added a few comments explaining why I commented out the tests with .centerActions. Other than that Comment 26 still applies to this patch.

Comment 37

[Frank Wein [:mcsmurf]]

Attachment: Fix and port tests in browser_pluginnotification.js

I forgot to update the other tests to the new code. Updated and checked that the tests still pass (though "!gTestBrowser.missingPlugins" also worked..?). Comment 26 and Comment 36 still apply to this patch.

Comment 38

[[e-mail]]

(In reply to Frank Wein from comment #37)
> "!gTestBrowser.missingPlugins" also worked..?
We don't set gTestBrowser.missingPlugins so it's always undefined which is what we "expected".

Comment 39

[Frank Wein [:mcsmurf]]

Attachment: Diff between Firefox version and SeaMonkey version of browser_pluginnotification.js

Right, forgot about this behavior..
For reference: Here's a diff between the FF version of browser_pluginnotification.js and the SeaMonkey version of that file.

Comment 40

[[e-mail]]

Comment on attachment 680439 [details] [diff] [review]
Fix and port tests in browser_pluginnotification.js

OK, the diffs seem reasonable.

Comment 41

[[e-mail]]

Comment on attachment 680078 [details] [diff] [review]
Patch for comm-beta/comm-aurora

[Approval Request Comment]
Regression caused by (bug #): 760625 (core change broke us)
User impact if declined: Problems with blocklisted plugins
Testing completed (on m-c, etc.): Trunk patch already landed on c-c
Risk to taking this patch (and alternatives if risky): 
String changes made by this patch: None

Comment 42

[Frank Wein [:mcsmurf]]

Comment on attachment 680439 [details] [diff] [review]
Fix and port tests in browser_pluginnotification.js

Pushed to comm-central: https://hg.mozilla.org/comm-central/rev/6e69635f3db8

Comment 43

[Philip Chee]

Frank, any ideas about these errors?

TEST_PATH=suite/browser/test/browser_pluginnotification.js  pymake -C ../objdir-sm/ mochitest-browser-chrome

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 161
        Failed: 3
        Todo: 1
....
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_pluginnotification.js | leaked until shutdown [nsGlobalWindow #56 chrome://mochitests/content/browser/suite/browser/test/plugin_hidden_to_visible.html]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_pluginnotification.js | leaked until shutdown [nsGlobalWindow #55 chrome://mochitests/content/browser/suite/browser/test/plugin_test.html]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_pluginnotification.js | leaked until shutdown [nsGlobalWindow #52 http://127.0.0.1:8888/browser/suite/browser/test/plugin_test.html]

Comment 44

[Justin Wood (:Callek)]

Comment on attachment 680078 [details] [diff] [review]
Patch for comm-beta/comm-aurora

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

a+ based on IRC convo that plugins forced to be click-to-play by blocklist would entirely break.

Comment 45

[Frank Wein [:mcsmurf]]

Pushed Attachment 680078 [details] [diff] to comm-beta: http://hg.mozilla.org/releases/comm-beta/rev/588b5b98f12c
Pushed Attachment 680079 [details] [diff] to comm-beta (I fixed two comments in the tests to actually say something useful): http://hg.mozilla.org/releases/comm-beta/rev/fa9b8a2819c0

Pushed Attachment 680078 [details] [diff] to comm-aurora: http://hg.mozilla.org/releases/comm-aurora/rev/82bfb8f75a28
Pushed Attachment 680079 [details] [diff] to comm-aurora (I fixed two comments in the tests to actually say something useful): http://hg.mozilla.org/releases/comm-aurora/rev/62f41d117099

Need to look into the potential memleak issue, so leaving open.

Comment 46

[Philip Chee]

https://hg.mozilla.org/comm-central/rev/6e69635f3db8#l2.295

>    2.295 +  ok(!notificationBox._missingPlugins, "Test 9c, Should not be a missing plugin list");
Frankm, do you remember why you used the field notificationBox._missingPlugins instead of the property notificationBox.missingPlugins?
See: http://hg.mozilla.org/comm-central/annotate/6996e05c74ee/suite/common/bindings/notification.xml#l441

Comment 47

[Frank Wein [:mcsmurf]]

Because the property always returns a map (see definition of onget in the getter), so the value would always be non-null.

Comment 48

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Frank Wein [:mcsmurf] from comment #45)
> Need to look into the potential memleak issue, so leaving open.

Such investigations tend to take some time so I propose to file a new follow-up for for that (which is cheap) and close this one.

Comment 49

[Frank Wein [:mcsmurf]]

I filed Bug 773761 on the memleak issue, so closing this bug.