Last Comment Bug 615675 - Allow plugins to work by default in non-email situations
: Allow plugins to work by default in non-email situations
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 3.3a3
Assigned To: Mark Banner (:standard8, afk until Dec)
:
:
Mentors:
Depends on: 637295
Blocks: 505437 617906
  Show dependency treegraph
 
Reported: 2010-11-30 13:08 PST by Mark Banner (:standard8, afk until Dec)
Modified: 2011-03-03 04:37 PST (History)
4 users (show)
standard8: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP (19.24 KB, patch)
2011-02-17 16:08 PST, Mark Banner (:standard8, afk until Dec)
no flags Details | Diff | Splinter Review
The fix (19.45 KB, patch)
2011-02-18 07:51 PST, Mark Banner (:standard8, afk until Dec)
mozilla: review+
bzbarsky: feedback+
Details | Diff | Splinter Review
The fix v2 (19.53 KB, patch)
2011-02-25 02:57 PST, Mark Banner (:standard8, afk until Dec)
neil: superreview-
Details | Diff | Splinter Review
The fix v3 (21.02 KB, patch)
2011-02-28 06:56 PST, Mark Banner (:standard8, afk until Dec)
neil: superreview+
Details | Diff | Splinter Review
The fix v4 (21.08 KB, patch)
2011-02-28 11:32 PST, Mark Banner (:standard8, afk until Dec)
mozilla: review+
Details | Diff | Splinter Review

Description Mark Banner (:standard8, afk until Dec) 2010-11-30 13:08:31 PST
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, afk until Dec) 2011-02-17 16:08:03 PST
Created attachment 513303 [details] [diff] [review]
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, afk until Dec) 2011-02-18 07:51:46 PST
Created attachment 513468 [details] [diff] [review]
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, afk until Dec) 2011-02-18 07:57:57 PST
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 [:bz] (still a bit busy) 2011-02-18 09:19:33 PST
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 2011-02-22 09:23:53 PST
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, afk until Dec) 2011-02-25 02:57:00 PST
Created attachment 515034 [details] [diff] [review]
The fix v2

Changed the docShell parameter to be a proper out param.
Comment 7 neil@parkwaycc.co.uk 2011-02-28 05:24:20 PST
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, afk until Dec) 2011-02-28 06:56:07 PST
Created attachment 515607 [details] [diff] [review]
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, afk until Dec) 2011-02-28 07:09:05 PST
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, afk until Dec) 2011-02-28 11:32:12 PST
Created attachment 515686 [details] [diff] [review]
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 2011-02-28 15:57:49 PST
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, afk until Dec) 2011-03-02 04:02:23 PST
Checked in: http://hg.mozilla.org/comm-central/rev/f06490cf5786
Comment 13 Joe Sabash [:JoeS1] 2011-03-02 18:47:52 PST
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, afk until Dec) 2011-03-03 00:18:18 PST
(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] 2011-03-03 04:37:08 PST
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/mailman.815.1299155530.4383.test-multimedia@lists.mozilla.org

Note You need to log in before you can comment on or make changes to this bug.