Last Comment Bug 713747 - Rip out non-UniversalXPConnect privilege manager functionality
: Rip out non-UniversalXPConnect privilege manager functionality
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
Depends on:
Blocks: 622301 714630
  Show dependency treegraph
 
Reported: 2011-12-27 13:48 PST by Bobby Holley (PTO through June 13)
Modified: 2013-04-22 07:44 PDT (History)
11 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Use UniversalXPConnect and UniversalXPConnect only in test coverage. v1 (31.35 KB, patch)
2011-12-28 20:51 PST, Bobby Holley (PTO through June 13)
bzbarsky: review+
Details | Diff | Review
part 2 - Fix funky tests. v1 (3.20 KB, patch)
2011-12-28 20:51 PST, Bobby Holley (PTO through June 13)
bzbarsky: review+
Details | Diff | Review
part 3 - Remove UniversalFoo from all.js. v1 (2.59 KB, patch)
2011-12-28 20:51 PST, Bobby Holley (PTO through June 13)
bzbarsky: review+
Details | Diff | Review
part 4 - Remove usage of UniversalFoo in gecko. v1 (19.86 KB, patch)
2011-12-28 20:52 PST, Bobby Holley (PTO through June 13)
bzbarsky: review+
Details | Diff | Review
part 5 - Remove UniversalFoo from caps (and thus, the tree). v1 (3.74 KB, patch)
2011-12-28 20:52 PST, Bobby Holley (PTO through June 13)
bzbarsky: review+
Details | Diff | Review
part 2 - Fix funky tests. v2 r=bz (5.83 KB, patch)
2011-12-29 20:06 PST, Bobby Holley (PTO through June 13)
bzbarsky: review+
Details | Diff | Review

Description Bobby Holley (PTO through June 13) 2011-12-27 13:48:04 PST
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.
Comment 1 Bobby Holley (PTO through June 13) 2011-12-27 16:17:11 PST
Pushed some patches to try for this: https://tbpl.mozilla.org/?tree=Try&rev=b8b1e8e4892e

Once they go green I'll flag for review.
Comment 2 Boris Zbarsky [:bz] 2011-12-27 16:48:09 PST
Can we land a warning on use of these into beta or aurora to give people more heads-up?
Comment 3 Bobby Holley (PTO through June 13) 2011-12-27 16:52:29 PST
(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?
Comment 4 Boris Zbarsky [:bz] 2011-12-27 18:17:16 PST
Er, so I did.  Nevermind, then!
Comment 5 Bobby Holley (PTO through June 13) 2011-12-28 20:50:18 PST
There were some test failures, which I've rectified hopefully. Attaching patches and flagging bz for review.
Comment 6 Bobby Holley (PTO through June 13) 2011-12-28 20:51:21 PST
Created attachment 584698 [details] [diff] [review]
part 1 - Use UniversalXPConnect and UniversalXPConnect only in test coverage. v1
Comment 7 Bobby Holley (PTO through June 13) 2011-12-28 20:51:39 PST
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.
Comment 8 Bobby Holley (PTO through June 13) 2011-12-28 20:51:54 PST
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.
Comment 9 Bobby Holley (PTO through June 13) 2011-12-28 20:52:22 PST
Created attachment 584701 [details] [diff] [review]
part 4 - Remove usage of UniversalFoo in gecko. v1
Comment 10 Bobby Holley (PTO through June 13) 2011-12-28 20:52:37 PST
Created attachment 584702 [details] [diff] [review]
part 5 - Remove UniversalFoo from caps (and thus, the tree). v1
Comment 11 Boris Zbarsky [:bz] 2011-12-28 21:07:26 PST
Comment on attachment 584700 [details] [diff] [review]
part 3 - Remove UniversalFoo from all.js. v1

r=me
Comment 12 Boris Zbarsky [:bz] 2011-12-28 21:11:26 PST
Comment on attachment 584698 [details] [diff] [review]
part 1 - Use UniversalXPConnect and UniversalXPConnect only in test coverage. v1

r=me
Comment 13 Boris Zbarsky [:bz] 2011-12-28 21:12:26 PST
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?
Comment 14 Boris Zbarsky [:bz] 2011-12-28 21:14:11 PST
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 15 Boris Zbarsky [:bz] 2011-12-28 21:17:07 PST
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.
Comment 16 Bobby Holley (PTO through June 13) 2011-12-28 22:05:08 PST
(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 17 Boris Zbarsky [:bz] 2011-12-28 22:08:25 PST
Comment on attachment 584702 [details] [diff] [review]
part 5 - Remove UniversalFoo from caps (and thus, the tree). v1

r=me
Comment 18 Bobby Holley (PTO through June 13) 2011-12-28 22:10:53 PST
(In reply to Boris Zbarsky (:bz) from comment #13)
> Make sure there's a followup on removing the new goop?

Filed bug 714012.
Comment 19 Bobby Holley (PTO through June 13) 2011-12-28 22:34:15 PST
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
Comment 20 Bobby Holley (PTO through June 13) 2011-12-29 20:06:34 PST
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
Comment 21 Bobby Holley (PTO through June 13) 2011-12-29 20:08:35 PST
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 22 Boris Zbarsky [:bz] 2011-12-29 20:13:12 PST
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?
Comment 23 Bobby Holley (PTO through June 13) 2011-12-29 20:18:37 PST
(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 26 Serge Gautherie (:sgautherie) 2012-01-02 07:56:46 PST
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?)
Comment 27 Serge Gautherie (:sgautherie) 2012-01-02 08:10:14 PST
(In reply to Bobby Holley (:bholley) from comment #23)
> it looks like there's one other user in the tree.

I filed bug 714632.
Comment 28 Bobby Holley (PTO through June 13) 2012-01-02 11:18:29 PST
(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.
Comment 29 Mango 2012-05-11 06:21:23 PDT
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.
Comment 30 Boris Zbarsky [:bz] 2012-05-11 08:02:25 PDT
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 Mango 2012-05-11 08:15:56 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.