Closed
Bug 1138154
Opened 9 years ago
Closed 9 years ago
Plugins default to "always activate" in Thunderbird
Categories
(Thunderbird :: Security, defect)
Tracking
(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)
RESOLVED
FIXED
Thunderbird 40.0
People
(Reporter: aleth, Assigned: aleth)
References
Details
Crash Data
Attachments
(2 files, 1 obsolete file)
1.44 KB,
patch
|
mkmelin
:
review-
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
mkmelin
:
review+
rkent
:
approval-comm-aurora+
rkent
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
Plugins the user likely installed with the intention of using them in the browser also automatically appear in Thunderbird. This isn't bad, but the fact that the plugins default to "always activate" seems wrong. Defaulting to "Ask to activate" would be better. It seems plugins used to be disabled by default for performance reasons (bug 605563), and possibly "ask to activate" wasn't available at the time that was changed.
Comment 1•9 years ago
|
||
(In reply to aleth [:aleth] from comment #0) > Plugins the user likely installed with the intention of using them in the > browser also automatically appear in Thunderbird. This isn't bad, but the > fact that the plugins default to "always activate" seems wrong. Defaulting > to "Ask to activate" would be better. > > It seems plugins used to be disabled by default for performance reasons (bug > 605563), and possibly "ask to activate" wasn't available at the time that > was changed. The performance issue was the mere loading of plugins, not their actual usage. bug 599119 comment 21 "plugin.disable completely stops loading plugins (e.g. even finding they are present). mailnews.message_display.allow.plugins only prevents add-ons from being activated in content." Does plugin.disable still exist? See also bug 615675
Crash Signature: https://bugzilla.mozilla.org/show_bug.cgi?id=599119
OS: Mac OS X → All
Comment 2•9 years ago
|
||
I've seen complaints about this publicly in discussions about Thunderbird, so I would like to see us do this. I don't believe we have a good reason to enable a bunch of plugins by default that have an unknown purpose within Thunderbird.
Assignee | ||
Comment 3•9 years ago
|
||
Pref defaults comparison: TB (https://dxr.mozilla.org/comm-central/source/mail/app/profile/all-thunderbird.js#786): // let all plugins except Flash default to click-to-play pref("plugin.default.state", 1); pref("plugin.state.flash", 2); Firefox (https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#659): pref("plugins.click_to_play", true); pref("plugin.default.state", 1); // Plugins bundled in XPIs are enabled by default. pref("plugin.defaultXpi.state", 2); // Flash is enabled by default, and Java is click-to-activate by default on // all channels. pref("plugin.state.flash", 2); pref("plugin.state.java", 1); plugin.default.state == 1 is nsIPluginTag.enabledState == STATE_CLICKTOPLAY, but in TB plugins.click_to_play defaults to false. Copying across the missing pref defaults seems to fix this bug. Do we *really* want to default-enable flash for TB?
Assignee | ||
Comment 4•9 years ago
|
||
Note this patch changes the default behaviour for flash (see my previous comment).
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attachment #8572694 -
Flags: review?(mkmelin+mozilla)
Comment 5•9 years ago
|
||
Comment on attachment 8572694 [details] [diff] [review] Change default state to "ask to activate" Review of attachment 8572694 [details] [diff] [review]: ----------------------------------------------------------------- We can't do this yet as we need to actually implment click to play in thunderbird first - bug 814168.
Attachment #8572694 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 6•9 years ago
|
||
Should we switch to default-disable for 38 then (since ask-to-activate/click-to-play doesn't work per comment 5)?
Comment 7•9 years ago
|
||
When I look at the list of plugins I have activated (10 in all) I can't see a single one that I think is important. I think we should try a disable by default in the next beta, and see if there are any reported issues. I'm going to manually disable all of them now. Adding tracking.
tracking-thunderbird38:
--- → +
Assignee | ||
Comment 8•9 years ago
|
||
Changes the default state to "never activate".
Attachment #8587586 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8572694 -
Attachment description: plugindefaults.diff → Change default state to "ask to activate"
Assignee | ||
Comment 9•9 years ago
|
||
Some previous history to help make this decision: bug 605563, bug 607031, bug 615675 In particular, there is a separate pref("mailnews.message_display.allow_plugins", false); in mailnews.js. Someone who knows that code better could check it still works as intended - it's not clear to me what the reasoning was for enabling plugins generally. Addons?
Comment 10•9 years ago
|
||
I doubt there's any good reason to keep plugins in general enabled, and the whole conecpt of mailnews.message_display.allow_plugins should probably die also. However, I think flash's still important enough that we want it enabled by default.
Comment 11•9 years ago
|
||
BTW, did you check if mozmill tests would need updating due to the change?
Comment 12•9 years ago
|
||
I have never seen flash be used in email. Are there any stats on this? Why should flash be enabled? I agree with disabling (or "ask to enable" when available) all plugins.
Comment 13•9 years ago
|
||
Not for mail, for feeds.
Assignee | ||
Comment 14•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8588741 -
Attachment description: Change default plugin state to disabled → Change default plugin state to disabled, but keep Flash enabled
Attachment #8588741 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Magnus Melin from comment #11) > BTW, did you check if mozmill tests would need updating due to the change? I'd push to try but c-c mozmill is currently busted.
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d0c809afcd1
Updated•9 years ago
|
Attachment #8587586 -
Attachment is obsolete: true
Attachment #8587586 -
Flags: review?(mkmelin+mozilla)
Comment 18•9 years ago
|
||
Comment on attachment 8588741 [details] [diff] [review] Change default plugin state to disabled, but keep Flash enabled Review of attachment 8588741 [details] [diff] [review]: ----------------------------------------------------------------- The try link won't open for me, but assuming it works, r=mkmelin
Attachment #8588741 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 19•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a016ada027b
Comment 20•9 years ago
|
||
The try run shows these new failures which I guess is from this patch: TEST-UNEXPECTED-FAIL | dom/plugins/test/unit/test_persist_in_prefs.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | dom/plugins/test/unit/test_plugin_default_state.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | dom/plugins/test/unit/test_plugin_default_state.js | run_test - [run_test : 6] 0 != 0
Comment 21•9 years ago
|
||
Heh, kinda useless error messages. Return code of 0 usually means success. And 0 != 0 ? :)
Comment 22•9 years ago
|
||
I would really like to try this patch in the next beta. It would be good if someone could look over the mozmill failures and determine if this is a real issue or just a testing issue, so we can make the decision.
Assignee | ||
Comment 23•9 years ago
|
||
test_plugin_default_state.js actually contains do_check_neq(pluginDefaultState, Ci.nsIPluginTag.STATE_DISABLED); so we would need a way to disable this test for TB if we want to take this patch. I don't know if that is possible?
Comment 24•9 years ago
|
||
Maybe at least with #ifdefs ?
Comment 25•9 years ago
|
||
The mechanism exists to disable the test in xpcshell.ini like: 7 # Bug 907732: thunderbird still uses legacy downloads manager. 8 skip-if = (os == "android" || buildapp == '../mail') The mechanism also exists in client.py to land specific patches in the mozilla directory if needed for a build. So we could use that mechanism to disable the test in comm-* without getting it landed first in mozilla-* There has been a lot of opposition to this in the past. I'm in favor though. I'll do a bug that demos this.
Assignee | ||
Comment 26•9 years ago
|
||
OK, I'll look into the other failing test then to see what's causing that.
Comment 27•9 years ago
|
||
Let's see if this works: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5c608ff82496
Assignee | ||
Comment 28•9 years ago
|
||
(In reply to aleth [:aleth] from comment #26) > OK, I'll look into the other failing test then to see what's causing that. It appears that when plugin.default.state == disabled, the plugin.state.* for the test plugin is not imported into prefs.
Assignee | ||
Comment 29•9 years ago
|
||
Standard8, do you have an opinion about this bug, since you likely know the history of TB plugin settings?
Flags: needinfo?(standard8)
Comment 30•9 years ago
|
||
My try run did not work. I suspect that hgtool overrides the apply-patches. Perhaps I can try it without hgtool?
Comment 31•9 years ago
|
||
(In reply to aleth [:aleth] from comment #29) > Standard8, do you have an opinion about this bug, since you likely know the > history of TB plugin settings? I know we did some of the plugin support for web pages quite a while back. It was before FF introduced things like click-to-play and disabled by default - so I think its fine to update the prefs and follow what FF are doing. (In reply to Kent James (:rkent) from comment #30) > My try run did not work. I suspect that hgtool overrides the apply-patches. > Perhaps I can try it without hgtool? https://wiki.mozilla.org/Thunderbird/Infrastructure/TryServer#Pushing_mozilla-central_patches
Flags: needinfo?(standard8)
Assignee | ||
Comment 32•9 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #31) > (In reply to aleth [:aleth] from comment #29) > > Standard8, do you have an opinion about this bug, since you likely know the > > history of TB plugin settings? > > I know we did some of the plugin support for web pages quite a while back. > It was before FF introduced things like click-to-play and disabled by > default - so I think its fine to update the prefs and follow what FF are > doing. There may be a misunderstanding here: this patch wouldn't introduce click to play ("ask to activate"), as that hasn't been ported (comment 5). Unlike Firefox, it would disable the plugins by default. I'm trying to find out if it is risky to make this change in the ESR beta cycle. (It's certainly too late to land a port of click-to-play for 38.)
Flags: needinfo?(standard8)
Comment 33•9 years ago
|
||
I don't have an opinion here. WRT risk you probably want to talk to the new drivers, so I'll redirect to Kent.
Flags: needinfo?(standard8) → needinfo?(rkent)
Comment 34•9 years ago
|
||
Mark, I think the question here is "from your experience, are there any common uses of plugins that would be broken if we simply disable them by default"? Most of us don't have much experience with plugins in Thunderbird, which is to be expected actually if they are used and needed rarely. But I am wary of what I don't know that I don't know. At the moment though the default plan is to try to disable them in the next beta. Let me work on the mozilla-*.patch issue to see if I can resolve that. I would hate to dirty my 100% green beta tree!
Flags: needinfo?(rkent)
Comment 35•9 years ago
|
||
I have the patch in bug 1154894 available that makes this work without adding new failures. But it is an m-c patch. I would like to try landing this current bug soon, so we can get some beta experience, but without bug 1154894 that would result in new test failures. What should we do? It also seems odd to land bug 1154894 when we are not confident that we are going to disable plugins by default.
Assignee | ||
Comment 36•9 years ago
|
||
(In reply to Kent James (:rkent) from comment #35) > I have the patch in bug 1154894 available that makes this work without > adding new failures. But it is an m-c patch. I would like to try landing > this current bug soon, so we can get some beta experience, but without bug > 1154894 that would result in new test failures. What should we do? It also > seems odd to land bug 1154894 when we are not confident that we are going to > disable plugins by default. You are suggesting we take this patch in the next beta, live with the two expected test failures on beta for now, and see what feedback comes back before taking the final decision to ship it in 38?
Comment 37•9 years ago
|
||
I'd just land it.
Comment 38•9 years ago
|
||
"You are suggesting we take this patch in the next beta, live with the two expected test failures on beta for now, and see what feedback comes back before taking the final decision to ship it in 38?" Essentially, yes. Normally we would also land on c-c and c-a as well so that they are in sync.
Comment 39•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/20e68aef39a9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Comment 40•9 years ago
|
||
Comment on attachment 8588741 [details] [diff] [review] Change default plugin state to disabled, but keep Flash enabled http://hg.mozilla.org/releases/comm-aurora/rev/03b2f3f3f02a https://hg.mozilla.org/releases/comm-beta/rev/ed7df48ef501
Attachment #8588741 -
Flags: approval-comm-beta+
Attachment #8588741 -
Flags: approval-comm-aurora+
Updated•9 years ago
|
status-thunderbird38:
--- → fixed
status-thunderbird39:
--- → fixed
status-thunderbird40:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•