Closed Bug 713747 Opened 12 years ago Closed 12 years ago

Rip out non-UniversalXPConnect privilege manager functionality

Categories

(Core :: Security: CAPS, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 1 obsolete file)

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.
Attached patch part 2 - Fix funky tests. v1 (obsolete) — Splinter Review
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)
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)
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
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.
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
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....
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.