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

RESOLVED FIXED in seamonkey2.16

Status

SeaMonkey
UI Design
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: InvisibleSmiley, Assigned: mcsmurf)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.16
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.14+ fixed, seamonkey2.15 fixed, seamonkey2.16 fixed)

Details

Attachments

(5 attachments, 8 obsolete attachments)

6.65 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
12.59 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
46.77 KB, patch
Details | Diff | Splinter Review
53.34 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
13.17 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

5 years ago
Created attachment 668908 [details] [diff] [review]
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.
(Reporter)

Comment 2

5 years ago
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

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

Comment 4

5 years ago
Created attachment 669750 [details] [diff] [review]
Patch

Still needs a bit of tweaking.
Attachment #668908 - Attachment is obsolete: true

Comment 5

5 years ago
Bug 773761 is a followup to Bug 760625.
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

5 years ago
Created attachment 675911 [details] [diff] [review]
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.
Attachment #669750 - Attachment is obsolete: true
Attachment #675911 - Flags: review?(neil)
(Assignee)

Comment 9

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

Comment 10

5 years ago
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).
tracking-seamonkey2.14: --- → ?

Comment 11

5 years ago
Ugh. Yeah. We should really get it into 2.14 ASAP.
tracking-seamonkey2.14: ? → +
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");
(Assignee)

Comment 13

5 years ago
Created attachment 677309 [details] [diff] [review]
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.
(Assignee)

Comment 14

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

Comment 15

5 years ago
Created attachment 677928 [details] [diff] [review]
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)?
Attachment #675911 - Attachment is obsolete: true
Attachment #675911 - Flags: review?(neil)
Attachment #677928 - Flags: review?(neil)
(Assignee)

Comment 16

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

Comment 19

5 years ago
Neil: See Bug 806136 on this, please comment there why this check is wrong.
Oops, forgot to request needinfo on comment #17 because assignee was wrong.
Assignee: nobody → bugzilla
Flags: needinfo?(bugzilla)
(Assignee)

Comment 21

5 years ago
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.
Flags: needinfo?(bugzilla) needinfo?(bugzilla) → needinfo+
Comment on attachment 677928 [details] [diff] [review]
Updated patch

Ah, so we should land this on branches too then?
Attachment #677928 - Flags: review?(neil) → review+
(Assignee)

Comment 23

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

Comment 24

5 years ago
BTW: I'll remove the bogus whitespace changes before check-in :)
(Assignee)

Comment 25

5 years ago
Pushed to comm-central: https://hg.mozilla.org/comm-central/rev/82a61096e746
Leaving bug open to request comm-aurora and comm-beta approval later.
Target Milestone: --- → seamonkey2.16
(Assignee)

Comment 26

5 years ago
Created attachment 679838 [details] [diff] [review]
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?
Attachment #677309 - Attachment is obsolete: true
Attachment #679838 - Flags: review?(neil)
(Assignee)

Comment 27

5 years ago
Created attachment 679839 [details] [diff] [review]
Diff between Firefox version and SeaMonkey version of browser_pluginnotification.js
(Assignee)

Comment 28

5 years ago
Hrm, forgot to add proper comments where I commented out notification.options.centerActions, I will add that locally for now.
(Assignee)

Comment 29

5 years ago
Created attachment 679852 [details] [diff] [review]
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.
(Assignee)

Comment 30

5 years ago
Created attachment 680078 [details] [diff] [review]
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.
Attachment #680078 - Flags: review?(neil)
(Assignee)

Comment 31

5 years ago
Created attachment 680079 [details] [diff] [review]
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.
Attachment #679852 - Attachment is obsolete: true
(Assignee)

Comment 32

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

Comment 34

5 years ago
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.
The point is that bug 799396 broke SeaMonkey, not that it broke the tests...
(Assignee)

Comment 36

5 years ago
Created attachment 680437 [details] [diff] [review]
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.
Attachment #679838 - Attachment is obsolete: true
Attachment #679838 - Flags: review?(neil)
Attachment #680437 - Flags: review?(neil)
(Assignee)

Comment 37

5 years ago
Created attachment 680439 [details] [diff] [review]
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.
Attachment #680437 - Attachment is obsolete: true
Attachment #680437 - Flags: review?(neil)
Attachment #680439 - Flags: review?(neil)
(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".
(Assignee)

Comment 39

5 years ago
Created attachment 680441 [details] [diff] [review]
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.
Attachment #679839 - Attachment is obsolete: true
Comment on attachment 680439 [details] [diff] [review]
Fix and port tests in browser_pluginnotification.js

OK, the diffs seem reasonable.
Attachment #680439 - Flags: review?(neil) → review+
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
Attachment #680078 - Flags: review?(neil)
Attachment #680078 - Flags: review+
Attachment #680078 - Flags: approval-comm-beta?
Attachment #680078 - Flags: approval-comm-aurora?
(Assignee)

Comment 42

5 years ago
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

5 years ago
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 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.
Attachment #680078 - Flags: approval-comm-beta?
Attachment #680078 - Flags: approval-comm-beta+
Attachment #680078 - Flags: approval-comm-aurora?
Attachment #680078 - Flags: approval-comm-aurora+
(Assignee)

Comment 45

5 years ago
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.
status-seamonkey2.14: --- → fixed
status-seamonkey2.15: --- → fixed
status-seamonkey2.16: --- → fixed

Comment 46

5 years ago
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
Flags: needinfo?(bugzilla)
(Assignee)

Comment 47

5 years ago
Because the property always returns a map (see definition of onget in the getter), so the value would always be non-null.
Flags: needinfo?(bugzilla)
(Reporter)

Comment 48

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

Updated

4 years ago
Blocks: 819642
(Assignee)

Comment 49

4 years ago
I filed Bug 773761 on the memleak issue, so closing this bug.
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 837397
You need to log in before you can comment on or make changes to this bug.