The default bug view has changed. See this FAQ.

Rip out non-UniversalXPConnect privilege manager functionality

RESOLVED FIXED in mozilla12

Status

()

Core
Security: CAPS
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({dev-doc-needed})

Trunk
mozilla12
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

We're aiming to get rid of the privilege manager entirely, so a good first step would be to kill things like UniversalBrowserRead, UniversalFileWrite, etc. This has a few advantages:

1 - I need tests to stop using them as a dependency for bug 622301.
2 - It simplifies the process later when we remove the API entirely.
3 - It simplifies the SpecialPowers conversion, since post-this-bug the only use of enablePrivilege should be |enablePrivilege("UniversalXPConnect")|, rather than one of the many variants we have now.
4 - It gives intranets etc a mild wakeup call that the API is going away soon. They'll have an easy temporary fix ("switch to UniversalXPConnect"), but hopefully recognize that they need to change things.

Patches coming right up.
Blocks: 622301
Pushed some patches to try for this: https://tbpl.mozilla.org/?tree=Try&rev=b8b1e8e4892e

Once they go green I'll flag for review.
Can we land a warning on use of these into beta or aurora to give people more heads-up?
Keywords: dev-doc-needed
(In reply to Boris Zbarsky (:bz) from comment #2)
> Can we land a warning on use of these into beta or aurora to give people
> more heads-up?

I thought you landed a warning for all uses of enablePrivilege?
Er, so I did.  Nevermind, then!
There were some test failures, which I've rectified hopefully. Attaching patches and flagging bz for review.
Created attachment 584698 [details] [diff] [review]
part 1 - Use UniversalXPConnect and UniversalXPConnect only in test coverage. v1
Attachment #584698 - Flags: review?(bzbarsky)
Created attachment 584699 [details] [diff] [review]
part 2 - Fix funky tests. v1

The conversion of checks for UniversalBrowserWrite to UniversalXPConnect caused these test fail, because they acquire UniversalXPConnect and actually doesn't want the results. In particular, they pass the security check in nsWindowWatcher::CalculateChromeFlags, which causes them to fail.

The first test doesn't actually need UniversalXPConnect, so we can remove it for free. The second one needs it, so we add some SpecialPowers goop to make it possible. It's not ideal, but it can go away when the wrapper in bug 702353 lands.
Attachment #584699 - Flags: review?(bzbarsky)
Created attachment 584700 [details] [diff] [review]
part 3 - Remove UniversalFoo from all.js. v1

I don't think we need this stuff at all anymore, since the security policy stuff is now burned into c++ in xpconnect/wrappers. However, I'm too afraid to do the removal myself.
Attachment #584700 - Flags: review?(bzbarsky)
Created attachment 584701 [details] [diff] [review]
part 4 - Remove usage of UniversalFoo in gecko. v1
Attachment #584701 - Flags: review?(bzbarsky)
Created attachment 584702 [details] [diff] [review]
part 5 - Remove UniversalFoo from caps (and thus, the tree). v1
Attachment #584702 - Flags: review?(bzbarsky)
Comment on attachment 584700 [details] [diff] [review]
part 3 - Remove UniversalFoo from all.js. v1

r=me
Attachment #584700 - Flags: review?(bzbarsky) → review+
Comment on attachment 584698 [details] [diff] [review]
part 1 - Use UniversalXPConnect and UniversalXPConnect only in test coverage. v1

r=me
Attachment #584698 - Flags: review?(bzbarsky) → review+
Comment on attachment 584699 [details] [diff] [review]
part 2 - Fix funky tests. v1

r=me

Make sure there's a followup on removing the new goop?
Attachment #584699 - Flags: review?(bzbarsky) → review+
Comment on attachment 584702 [details] [diff] [review]
part 5 - Remove UniversalFoo from caps (and thus, the tree). v1

Shouldn't CheckSameOriginDOMProp still check for UniversalXPConnect?  Or is that handled elsewhere?
Comment on attachment 584701 [details] [diff] [review]
part 4 - Remove usage of UniversalFoo in gecko. v1

Why not just nix the useless arg from IsCallerTrustedForCapability (and perhaps rename it to CallerHasUniversalXPConnectPrivs or something)?

r=me with something like that.
Attachment #584701 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #14)
> Comment on attachment 584702 [details] [diff] [review]
> part 5 - Remove UniversalFoo from caps (and thus, the tree). v1
> 
> Shouldn't CheckSameOriginDOMProp still check for UniversalXPConnect?  Or is
> that handled elsewhere?

This comment suggests that we check it later:
http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#1085

And I believe it's correct. In the only caller of CheckSameOriginDOMProp, we subsequently call CheckXPCPermissions:

http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#873

And we must, because within caps UniversalXPConnect doesn't imply the rest. So the current call to IsCapabilityEnabled would fail for just UniversalXPConnect.
Comment on attachment 584702 [details] [diff] [review]
part 5 - Remove UniversalFoo from caps (and thus, the tree). v1

r=me
Attachment #584702 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #13)
> Make sure there's a followup on removing the new goop?

Filed bug 714012.
Addressed bz's comments, and gave this one last push to try. Once it goes green, this can land: https://tbpl.mozilla.org/?tree=Try&rev=281a08aa5c58
Created attachment 584909 [details] [diff] [review]
part 2 - Fix funky tests. v2 r=bz

Arg, looks like I missed one other modal dialog test. Given that bz's on PTO, I'm flagging jdm for review on the changes to test_bug437361.html. bz's review carries on everything else.

I've pushed this to try for another run of the mochitest-2 suite only: https://tbpl.mozilla.org/?tree=Try&rev=741e3c3a5d35
Attachment #584699 - Attachment is obsolete: true
Attachment #584909 - Flags: review?(josh)
Attachment #584909 - Attachment description: Bug 713747 - Fix funky tests. v2 r=bz → part 2 - Fix funky tests. v2 r=bz
Also, given that this was the patch that introduced mozprefs.js and I'd never heard of it before, I'd wager a guess it was the only mochitest that used it. Given that we're all for SpecialPowers now, it should probably be removed from the tree, with any unique functionality being merged into SpecialPowers.
Comment on attachment 584909 [details] [diff] [review]
part 2 - Fix funky tests. v2 r=bz

r=me, but do you still need to include mozprefs.js?
Attachment #584909 - Flags: review?(josh) → review+
(In reply to Boris Zbarsky (:bz) from comment #22)
> Comment on attachment 584909 [details] [diff] [review]
> part 2 - Fix funky tests. v2 r=bz
> 
> r=me, but do you still need to include mozprefs.js?

Oh, good catch. I don't! And it looks like there's one other user in the tree.
Landed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c85e6c648d8f
http://hg.mozilla.org/integration/mozilla-inbound/rev/c991b7a9810a
http://hg.mozilla.org/integration/mozilla-inbound/rev/3efc3a946a7f
http://hg.mozilla.org/integration/mozilla-inbound/rev/3aa338192b8a
http://hg.mozilla.org/integration/mozilla-inbound/rev/e1b00c29bf03
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/c85e6c648d8f
https://hg.mozilla.org/mozilla-central/rev/c991b7a9810a
https://hg.mozilla.org/mozilla-central/rev/3efc3a946a7f
https://hg.mozilla.org/mozilla-central/rev/3aa338192b8a
https://hg.mozilla.org/mozilla-central/rev/e1b00c29bf03
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 714630
Comment on attachment 584909 [details] [diff] [review]
part 2 - Fix funky tests. v2 r=bz

Review of attachment 584909 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/bugs/test_bug504862.html
@@ -24,4 @@
>    window.addEventListener("message", onMsgRcv, false);
>  
> -  var subsequentDlg = "data:text/html,<html><body onload='opener.is(window.dialogArguments, \'my args\', \'subsequent dialog document did not get the right arguments.\'); close();'>";
> -

Just to confirm, was the removal (= not replacement) of this line intended?
(As in, was it not part of the test? Or is that part obsolete now?)
(In reply to Bobby Holley (:bholley) from comment #23)
> it looks like there's one other user in the tree.

I filed bug 714632.
(In reply to Serge Gautherie (:sgautherie) from comment #26)
> Just to confirm, was the removal (= not replacement) of this line intended?
> (As in, was it not part of the test? Or is that part obsolete now?)

Yeah, it was a dead variable.
Flags: in-testsuite+
Version: unspecified → Trunk

Comment 29

5 years ago
For those of us who relied on UniversalBrowserRead and UniversalBrowserWrite, what is the alternative?

I need to load a page from another domain in an iFrame, populate the form, and submit the form, using Javascript.  The purpose is to automate an online shipping tool with no API.
Short-term, use UniversalXPConnect.

Longer-term, enablePrivilege is going away.  We've been saying so for several years now.  Code that needs extra privileges should move into an extension....

Comment 31

5 years ago
Thanks.  UniversalXPConnect was not a drop-in replacement and required some rewriting.

If anyone has a tutorial that will allow me to convert my current code to an extension, I would love to see it.
You need to log in before you can comment on or make changes to this bug.