Closed Bug 595397 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XUL, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: martijn.martijn, Assigned: sicking)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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
Attachment #474267 - Flags: review?(jonas)
Probably worth using nsContentUtils::AddBoolPrefVarCache for this.
Attached patch patch? (obsolete) — Splinter Review
Something like this?
Attachment #474267 - Attachment is obsolete: true
Attachment #475114 - Flags: review?(jonas)
Attachment #474267 - Flags: review?(jonas)
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.
Attachment #475114 - Flags: review?(jonas) → review-
Attached patch patch? (obsolete) — Splinter Review
Something like this?
Attachment #475244 - Flags: review?(jonas)
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.
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.
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?
Attached patch patch? (obsolete) — Splinter Review
Attachment #475848 - Flags: review?(jonas)
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.
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.
Assignee: nobody → jonas
Attachment #475114 - Attachment is obsolete: true
Attachment #475244 - Attachment is obsolete: true
Attachment #475848 - Attachment is obsolete: true
Attachment #484192 - Flags: review?(dveditz)
Attachment #475244 - Flags: review?(jonas)
Attachment #475848 - Flags: review?(jonas)
Marking this a blocker as I think it significantly improves our story for XUL devs.
blocking2.0: --- → final+
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 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
Attachment #484192 - Flags: review?(dveditz) → review+
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.)
Keywords: dev-doc-needed
I'll name the pref

dom.allow_XUL_XBL_for_file
Blocks: 614191
Will tests be updated to use the pref, to avoid the permmgr hack?
Checked in: http://hg.mozilla.org/mozilla-central/rev/42e9a38ecf63
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Summary: Global pref to allow XUL and XBL on all sites → Global pref to allow XUL and XBL for file://
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: