595397 - Global pref to allow XUL and XBL for file://

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

Attachment: patch

I think it would be beneficial to have a global pref that allows XUL and XBL on all sites.
In the patch I've named the preference: dom.allowXULXBL_for_all_sites

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Probably worth using nsContentUtils::AddBoolPrefVarCache for this.

Comment 2

[Martijn Wargers (dead)]

Attachment: patch?

Something like this?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 475114 [details] [diff] [review]
patch?

No, this doesn't work. First of all it's unneccesary to have one listener (and one member) for *every* document. They will all be set to the same value anyway. Second, since the listener is never unregistered, it means that they'll keep modify those memory locations even after the document has been deleted.

Just use a static member, and only set up the listener when the first document is created or some such.

Comment 4

[Martijn Wargers (dead)]

Attachment: patch?

Something like this?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You missed the "first" in "when the first document is created" :)

As it is, this will set up a new listener for every document that is created.

Comment 6

[Martijn Wargers (dead)]

I didn't miss "first", I just don't know when the first document is created, so I'm taking wild guess of where I need to add the code.

Comment 7

[Henri Sivonen (:hsivonen)]

If it's easier to flip this pref than to add sites to the whitelist, won't the most probable outcome be that users allow XUL for all sites and run with security holes? What's the use case for the pref?

Comment 8

[Martijn Wargers (dead)]

Attachment: patch?

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 475848 [details] [diff] [review]
patch?

Throwing up a management dialog would be easy, just call chrome://browser/content/preferences/permissions.xul with an appropriate param block. Finding a place to hook it would be harder, especially for a feature we want to discourage. Easy for an extension to do though.

I'm torn about allowing the global pref, people using XUL in content are already requiring a specific browser (Firefox, not IE or other brands) so why not require a specific version? On the other hand 3.6.x is probably only good for another year at most so that's not the same kind of solution as Microsoft had telling people to keep using IE 6 for ten years (with MS supporting it for most of that time).

I'd like to work in "unsafe" or a synonym into the pref name if we're going to have a global. Also, I don't like the mix of camelcase and underscores -- pick one or the other (yeah, I know that's the permissionManager token sicking used, but that's a different API).

Maybe
  dom.unsafe_allow_remote_XUL
  dom.allow_unsafe_remote_XUL
  dom.allow_unsafe_XUL_for_all_sites
  dom.allow_XUL_for_unsafe_sites
  ?

Does the pref need to mention both xul and xbl? Since the permission is tied together it's probably good enough to mention only the more prominent XUL.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Create a pref for turning on XUL for file:

I talked with dveditz about this some more, and we came to the conclusion that having a pref for enabling XUL/XBL for file: might be a good compromise between risk and benefit.

This allows XUL/XBL developers to easily test while developing, and also gives xulrunner apps an easy way to use local non-chrome XUL/XBL.


Martijn: Please let us know if this isn't good enough for what you need.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking this a blocker as I think it significantly improves our story for XUL devs.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 484192 [details] [diff] [review]
Create a pref for turning on XUL for file:

>+  nsContentUtils::AddBoolPrefVarCache("dom.allowXULXBL_for_file",

Please add this pref to the all.js defaults so the developers you're aiming this option at will be able to find it. Much easier to filter on XULXBL in about:config than to right-click, select "new", type the entire pref correctly, and set the value.

Would still prefer an underscore between "allow" and "XULXBL" but I guess that's just style and I won't insist on it. The rest looks good.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 484192 [details] [diff] [review]
Create a pref for turning on XUL for file:

I've changed my mind about the default pref, it's just going to chew up memory for zillions of people who will never need it.
   r=dveditz

Comment 14

[Daniel Veditz [:dveditz]]

We should document the pref, though, wherever we document the removal of remote XUL/XBL. (should we also document the per-site permission manager override? That's not exactly something a user could do, but a XUL-app developer might need to roll out a tool to set it for their users.)

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll name the pref

dom.allow_XUL_XBL_for_file

Comment 16

[[e-mail]]

Will tests be updated to use the pref, to avoid the permmgr hack?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in: http://hg.mozilla.org/mozilla-central/rev/42e9a38ecf63

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That's what bug	614191 is about.

Comment 19

[Eric Shepherd [:sheppy]]

Noted this change here:

https://developer.mozilla.org/en/Using_Remote_XUL
https://developer.mozilla.org/en/Remote_XUL
https://developer.mozilla.org/en/Firefox_4_for_developers#Miscellaneous_XUL_changes