Closed Bug 1138154 Opened 9 years ago Closed 9 years ago

Plugins default to "always activate" in Thunderbird

Categories

(Thunderbird :: Security, defect)

x86
All
defect
Not set
normal

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed

People

(Reporter: aleth, Assigned: aleth)

References

Details

Crash Data

Attachments

(2 files, 1 obsolete file)

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.
(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
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.
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?
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 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-
Depends on: 814168
Should we switch to default-disable for 38 then (since ask-to-activate/click-to-play doesn't work per comment 5)?
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.
Attached patch Change default plugin state to disabled (obsolete) — — Splinter Review
Changes the default state to "never activate".
Attachment #8587586 - Flags: review?(mkmelin+mozilla)
Attachment #8572694 - Attachment description: plugindefaults.diff → Change default state to "ask to activate"
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?
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.
BTW, did you check if mozmill tests would need updating due to the change?
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.
Not for mail, for feeds.
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)
(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.
Attachment #8587586 - Attachment is obsolete: true
Attachment #8587586 - Flags: review?(mkmelin+mozilla)
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+
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
Heh, kinda useless error messages. Return code of 0 usually means success. And 0 != 0 ? :)
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.
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?
Maybe at least with #ifdefs ?
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.
See Also: → 1154894
OK, I'll look into the other failing test then to see what's causing that.
(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.
Standard8, do you have an opinion about this bug, since you likely know the history of TB plugin settings?
Flags: needinfo?(standard8)
My try run did not work. I suspect that hgtool overrides the apply-patches. Perhaps I can try it without hgtool?
(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)
(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)
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)
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)
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.
(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?
I'd just land it.
"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.
Depends on: 1154894
https://hg.mozilla.org/comm-central/rev/20e68aef39a9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
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+
Depends on: 1165152
You need to log in before you can comment on or make changes to this bug.