The capability manager as we knew it is dead, thanks to bug 757046. Now it's time to reap some serious code cleanup.

Not finished with the patches yet, but did an intermediate try push to see if anything's broken so far: https://tbpl.mozilla.org/?tree=Try&rev=9eb8ab6384e5

Removed enablePrivilege from a bunch of tests that didn't like the semantic change in nsContentUtils::IsCallerChrome, and repushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cc3dc816f277

Last try push had an out-of-date base. Trying again: https://tbpl.mozilla.org/?tree=Try&rev=b70b1f1ae770

Arg, certificate principals weren't as unused as I thought they were. It appears that we still need some of that machinery during addon install time. However, it _can_ be separated out of caps. Trying that: https://tbpl.mozilla.org/?tree=Try&rev=89c789d3b020

Almost, just one more bug with respect to js:// URIs: https://tbpl.mozilla.org/?tree=Try&rev=c4e891183f0b

This is green except for a windows-only failures in: browser/components/tabview/test/browser_tabview_bug663421.js browser/components/tabview/test/browser_tabview_snapping.js Which test Tab Preview, a windows-only feature. Investigating.

Ugh, can't reproduce locally. Trying various things, but in the mean time I'm going to nail down which changeset is causing the issues on try: https://tbpl.mozilla.org/?tree=Try&rev=6d234b9893d2 https://tbpl.mozilla.org/?tree=Try&rev=dd2ea0ac781d https://tbpl.mozilla.org/?tree=Try&rev=c60eb57b06e9

And somehow, I managed to trigger M-5 runs instead of M-o runs: https://tbpl.mozilla.org/?tree=Try&rev=e81ce372630c https://tbpl.mozilla.org/?tree=Try&rev=e9f868696570 https://tbpl.mozilla.org/?tree=Try&rev=a1d1474064f6

Alright, so the second patch, which rejiggers a bunch of privilege checks, is the culprit. Seeing as how I can't reproduce this locally, I'm pushing a bunch of logging code to try: https://tbpl.mozilla.org/?tree=Try&rev=6b3b6ac683a9

Attaching the patches here for reference. This function is also called to evaluate javascript:// URIs, at which point the chrome caller assumption isn't actually valid. Let's just do the modern thing here, and fix up the caller that breaks.

There's no longer any reason why "certificate principals" need to be principals at all. I tried to rip them out entirely, but it looks like they're still used vestigially at XPI install time to display author information. But there's no reason that they have to be porkbarreled into the security-critical objects that we pass around all over the place. So let's make them their own deal. I was tempted to call them "certificate holders", but that would involve renaming methods and cause more compat fuss than necessary.

This stuff is super ugly and confusing. I think we're better off without it.

Sigh, the logging wasn't super helpful. I split the offending commit up some more and pushed parts of it one by one. I pushed only the first half of changes. If they're all green, I'll push more: https://tbpl.mozilla.org/?tree=Try&rev=f75d0fd43b4b https://tbpl.mozilla.org/?tree=Try&rev=6a03cf6e3dc3 https://tbpl.mozilla.org/?tree=Try&rev=2bfd09e506e6 https://tbpl.mozilla.org/?tree=Try&rev=27b3637aa8cb https://tbpl.mozilla.org/?tree=Try&rev=56bfa263d5ab https://tbpl.mozilla.org/?tree=Try&rev=d3b111683de1

The parts in comment 21 were all green - the plot thickens! Pushing the rest of the changeset pieces incrementally: https://tbpl.mozilla.org/?tree=Try&rev=fa261aecc597 https://tbpl.mozilla.org/?tree=Try&rev=0cbc178cc62b https://tbpl.mozilla.org/?tree=Try&rev=d1bbfd18fe41 https://tbpl.mozilla.org/?tree=Try&rev=ee525ddd42e3 https://tbpl.mozilla.org/?tree=Try&rev=b5218c354c62 https://tbpl.mozilla.org/?tree=Try&rev=ba5cc48c8982

Ah, nsWindowWatcher! Of course... :\ Looks like I got the logical equivalencies ever-so-slightly wrong while simplifying some voodoo logic. Pushing the updated version for a win32 m-o run and crossing my fingers: https://tbpl.mozilla.org/?tree=Try&rev=22d90eb7bdea

Success! Looks like that did the trick. Uploading the updated patch, and flagging review on the entire set.

Comment on attachment 664473 [details] [diff] [review] Part 1 - Remove chrome check in xpc_EvalInSandbox. v1 Review of attachment 664473 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/jsurl/nsJSProtocolHandler.cpp @@ +290,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + // The nsXPConnect sandbox API gives us a wrapper to the sandbox for > + // our current compartment. Because our current context doesn't necessarily > + // subsumes that of the sandbox, we want to unwrap and enter the sandbox's subsumes -> subsume

Comment on attachment 664476 [details] [diff] [review] Part 4 - Remove principal capability and preference infrastructure. v1 Review of attachment 664476 [details] [diff] [review]: ----------------------------------------------------------------- It is really nice to see this code go away.

Comment on attachment 664927 [details] [diff] [review] Part 2 - Remove miscellaneous UniversalXPConnect checks sprinkled throughout gecko. v2 Review of attachment 664927 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsContentUtils.cpp @@ +1792,5 @@ > if (NS_FAILED(rv)) { > return false; > } > + if (is_caller_chrome) > + return true; {}, two-space @@ +6039,5 @@ > } > } > > + if (IsCallerChrome()) { > + return true; two-space ::: docshell/base/nsDocShell.cpp @@ +1668,5 @@ > { > + // We want to bypass this check for chrome callers, but only if there's > + // JS on the stack. System callers still need to do it. > + if (nsContentUtils::GetCurrentJSContext() && nsContentUtils::IsCallerChrome()) > + return true; {}, and I think 4-space @@ +1684,1 @@ > rv = originDocument->NodePrincipal()-> nsresult rv = ... ::: dom/base/nsDOMClassInfo.h @@ +552,1 @@ > } Inline this into the one caller instead (and remove the nsContentUtils.h include)

Comment on attachment 664477 [details] [diff] [review] Part 5 - Remove signed script security checks. v1 Review of attachment 664477 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/xbl/src/nsXBLBinding.cpp @@ +1507,5 @@ > if (NS_FAILED(rv) || !canExecute) { > return false; > } > > + return true; 'return NS_SUCCEEDED(rv) && canExecute;'?

Comment on attachment 664478 [details] [diff] [review] Part 6 - Separate certificate principals out from CAPS. v1 Review of attachment 664478 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsCertificatePrincipal.cpp @@ +31,5 @@ > +NS_IMETHODIMP > +nsCertificatePrincipal::GetCertificate(nsISupports** aCert) > +{ > + nsCOMPtr<nsISupports> cert = mCert; > + *aCert = cert.forget().get(); cert.forget(aCert);

Comment on attachment 664478 [details] [diff] [review] Part 6 - Separate certificate principals out from CAPS. v1 I don't understand the InstallTrigger stuff well enough to review this. The patch looks good to me though.

Comment on attachment 664927 [details] [diff] [review] Part 2 - Remove miscellaneous UniversalXPConnect checks sprinkled throughout gecko. v2 Review of attachment 664927 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/xbl/src/nsXBLContentSink.cpp @@ +730,5 @@ > // have a concept of enabling capabilities on a per-principal basis, > // but only on a per-principal-and-JS-stackframe basis! So for now > // this is basically equivalent to testing that we have the system > // principal, since there is no JS stackframe in sight here... > + if (nsContentUtils::IsSystemPrincipal(mDocument->NodePrincipal())) { The XXX comment above this can change a little now.

dveditz - is this a review you can easily do? If not, can you suggest someone else? The patch is mostly code removal, and I'd really like to land this before the branch.

Bug 777467 is a basecamp blocker and depends on this bug. I think it would be nice to have it marked as a blocker to focus attention and have that last review done.

I'm a little bit worried about blocking on this one. If it turns out to be a bigger amount of work that is remaining than expected, we should reevaluate.

I pushed the bottom patch because of a dependency by bug 797204. https://hg.mozilla.org/integration/mozilla-inbound/rev/ab15ebccc0c0

Comment on attachment 664478 [details] [diff] [review] Part 6 - Separate certificate principals out from CAPS. v1 Review of attachment 664478 [details] [diff] [review]: ----------------------------------------------------------------- r=dveditz ::: caps/idl/nsIPrincipal.idl @@ +95,1 @@ > * XXXbz except see bug 147145! You can zap this XXX note too -- bug 147145 (Freeze nsIPrincipal) is WONTFIX

Thanks Dan! Updated and pushed to inbound: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c0dc5df119 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/77749ba52bbf remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/50d1235983d3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6980e4e0ceec remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/42b11a0fe323 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a578debf08b9

https://hg.mozilla.org/mozilla-central/rev/d7c0dc5df119 https://hg.mozilla.org/mozilla-central/rev/77749ba52bbf https://hg.mozilla.org/mozilla-central/rev/50d1235983d3 https://hg.mozilla.org/mozilla-central/rev/6980e4e0ceec https://hg.mozilla.org/mozilla-central/rev/42b11a0fe323 https://hg.mozilla.org/mozilla-central/rev/a578debf08b9

(In reply to Bobby Holley (:bholley) from comment #8) > Arg, certificate principals weren't as unused as I thought they were. It > appears that we still need some of that machinery during addon install time. > However, it _can_ be separated out of caps. Trying that: All of that can be replaced by a new nsIZipReader::AreAllContentsOfThisJARSignedByTheSameTrustedCertificate() method that returns true/false. I am working on that now (as well as making this disk- and/or network- I/O-causing task asynchronous), as part of implementing signed package apps.

Bobby, my feeling is that bug should be marked FIXED. I think the [leave open] wasn't removed after the first patch landing. Also, could you push those patches to mozilla-aurora? a=blocking-basecamp. I would need that to push bug 777467 to aurora.

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/713b35535da4 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/9ffb80107340 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/ee28bae3cd54 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/82a70417b502 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/78cc1d3e1d5a remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/d3453d0161c5 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/4d04726fe87b