615675 - Allow plugins to work by default in non-email situations

Status: RESOLVED FIXED

(MailNews Core :: Security, defect)

Description

[Mark Banner (:standard8)]

Currently we have a preference mailnews.message_display.allow.plugins that globally disables or enables plugins within mailnews displays, e.g. 3-pane window, compose window etc.

Various users want to be able to use plugins within RSS feeds, but probably not necessarily within emails, and likewise, with the use of content tabs, there may be a case for extension wanting to use plugins.

I have a WIP patch that I think will allow us to enable plugins in RSS feeds and content tabs, but have the pref above only disable plugins in messages. If users then want to completely disable plugins, they they can still do so via the add-on manager or the plugin.disable preference.

One of the side-effects of this is that we should be able to get reftests passing on Thunderbird (they assume plugins are enabled), I've not worked out how useful that is yet, but it may be useful information around release times.

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP

This patch implements the changes required. I've got a couple more tweaks to do on the mozmill tests, but it should be pretty much there now.

I'll detail more about it when I request review, more posting just to keep it safe.

Comment 2

[Mark Banner (:standard8)]

Attachment: The fix

This is all now working and I've included some tests. Here's a few notes:

- Currently we have both nsMessenger setting allowPlugins to false on the message pane docShell, as well as the content policy denying the load of plugins if the pref was set appropriately.

- The patch make it so that nsMsgContentPolicy is now solely responsible for detecting the plugin load and setting allowPlugins to false on the docShell if the pref requires it. This covers us enough for not loading the plugin.

This operates in the same way as we do for Javascript.

I'm not sure that its setting it on the root same type docShell, but it is definitely setting it on an appropriate docShell; from my testing this seems to also mean we don't have to set the value if we're going to allow the load.

- All this means that nsMessenger no longer needs to set allowPlugins to false on the message pane docShell.

- The removal of mSearchContext from the destructor is just clean-up.

- MozMill tests are included, I'm currently running them through try server.

- I've changed the name of the preference from mailnews.message_display.allow.plugins to mailnews.message_display.allow_plugins.

This is a concious decision to force change so that people will need to re-consider if they need this changed or not. For example, RSS feed users wanting plugins will no longer need to toggle this preference as we'll allow plugins in feeds automatically. I did think of changing the name, but given the mailnews.message_display.disable_remote_image pref, I couldn't really think of a better option.

Comment 3

[Mark Banner (:standard8)]

Ok, Mozmill tests currently fail in packaged format because they don't have the test plugin installed by default. I'll work on resolving how to fix that.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 513468 [details] [diff] [review]
The fix

This looks fine, esp if you use an actual out param for the docshell thing..

Comment 5

[David :Bienvenu]

Comment on attachment 513468 [details] [diff] [review]
The fix

2011?

+ * Portions created by the Initial Developer are Copyright (C) 2001
+ * the Initial Developer. All Rights Reserved.

This certainly looks more sane that what we had before...

Comment 6

[Mark Banner (:standard8)]

Attachment: The fix v2

Changed the docShell parameter to be a proper out param.

Comment 7

[[e-mail]]

Comment on attachment 515034 [details] [diff] [review]
The fix v2

The nsMessenger changes I understand. In fact, I wouldn't be surprised if they turned out to be unnecessary all along, since as far as I can tell the content policy blocks all plugins in mail windows anyway.

The nsMsgContentPolicy changes I don't understand. I could understand it if either a) plugins were disabled at the same time as script, and then potentially enabled at the same time as script, or b) content policy was tweaked to only apply plugin policy to mail/news URLs. But this patch seems to have a hybrid approach, with the plugins being disabled during the plugin content policy check (which sounds like a really bad idea to me) and no sign of them being enabled.

EDIT: so I ran this through a debugger, and it showed me that GetDocshellForDisabling was throwing in the plugin case, which turns out to be unsurprising, since the only objects that have frame loaders are frames (well, any embedded objects with HTML/XML content, but that still excludes plugins). So what's really happening is this:
1. If the plugin pref is enabled, then the plugin is allowed.
2. Otherwise if the load isn't owned by a mail/news URL, then GetDocshellForDisabling returns a null docShell, and the plugin is allowed.
3. Otherwise GetDocshellForDisabling fails, and the plugin is only blocked because of the "fallback" code.

Comment 8

[Mark Banner (:standard8)]

Attachment: The fix v3

Neil's right, and setting it on document load seems much more logical as well.

I've also updated onLocationChange so that it sets plugins on/off as necessary, this fixes the case of email message to non-email message being displayed in the message pane. I've also added an extra test for that specific case.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 515607 [details] [diff] [review]
The fix v3

Re-requesting review from David in case he wants to take another quick look.

Comment 10

[Mark Banner (:standard8)]

Attachment: The fix v4

I was just testing this on try server and realised that putting code to always be executed inside assertions was not a good idea.

Comment 11

[David :Bienvenu]

Comment on attachment 515686 [details] [diff] [review]
The fix v4

looks ok, though the copyright still says 2001

passes mozmill tests on a release build for me.

Comment 12

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/f06490cf5786

Comment 13

[Joe Sabash [:JoeS1]]

Plugins still seem to be working in news posts _without_ setting the new pref:
mailnews.message_display.allow_plugins to "true"
Didn't try mail, and the old pref seems to still be there.

I'm assuming this made today's nightly because of the existence of the new pref.
Mozilla/5.0 (Windows NT 5.0; rv:2.0b13pre) Gecko/20110302 Thunderbird/3.3a3pre ID:20110302044201

Comment 14

[Mark Banner (:standard8)]

(In reply to comment #13)
> Plugins still seem to be working in news posts _without_ setting the new pref:
> mailnews.message_display.allow_plugins to "true"

That's strange. Can you file a follow-up bug on this and point me at a suitable test newsgroup?

> Didn't try mail, and the old pref seems to still be there.

The old pref will only be there if you've previously set it to the non-default value.

Comment 15

[Joe Sabash [:JoeS1]]

My mistake..The pref was being ignored because of an experimental extension that I'm testing. Anyhow, I posted a test at news://news.mozilla.org:119/[e-mail]