972478 - Disabling script on a docshell should not affect chrome pages

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

While investigating things for bug 971650, I realized that this seems to be broken. Patch coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Wait.  Why shouldn't it?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Wait.  Why shouldn't it?

Well, it didn't historically - CheckFunctionAccess would always bail out in the SystemPrincipal case.

In general, the pattern seems to be that chrome script always gets to run. I'm certainly open to changing this if there are reasons to though.

Comment 3

[Bobby Holley (:bholley)]

(Chrome is unaffected by Domain Policy, for example, even in whitelist mode).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Seems to me like if people disable script on a docshell altogether they really mean for it to be disabled no matter what's loaded in there.... but it probably doesn't matter much in practice.

Comment 5

[Bobby Holley (:bholley)]

I'm mostly just worried about sloppy addons accidentally disabling browser UI. The docshell flag is a pretty big hammer, and I think it's not great to expand its scope.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Docshell scriptability should only affect non-immune principals. v1

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8375685 [details] [diff] [review]
Docshell scriptability should only affect non-immune principals. v1

r=me

Comment 8

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=36af66b014d1

Comment 9

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60e089842cb6

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8375685 [details] [diff] [review]
Docshell scriptability should only affect non-immune principals. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 840488
User impact if declined: Addons might break browser UI
Testing completed (on m-c, etc.): Just pushed to m-i.
Risk to taking this patch (and alternatives if risky): low risk.
String or IDL/UUID changes made by this patch: None.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

I will wait it reaches m-c before uplifting it.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60e089842cb6

Comment 13

[Bobby Holley (:bholley)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> I will wait it reaches m-c before uplifting it.

Yeah totally. With patches like this I just write the approval request early-on so that I don't forget about it, with the assumption that the release drivers will let it bake for a few days before approving it.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

No worries! I am just explaining since some developers have different expectations ;)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a6805d8b360
https://hg.mozilla.org/releases/mozilla-beta/rev/7e47e1271dd5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/7e47e1271dd5