Clean up various pointless uses of the security manager

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(5 attachments)

Around the time of bug 451731, the security manager was used in various places in nsJSEnvironment.cpp, but at some point in the intervening decade that stopped being necessary, so hopefully we can remove this variable. It is possible that startup or shutdown rely on it somehow.
Assignee: nobody → continuation
There's a fair amount of this.
Component: DOM → Security: CAPS
Summary: Remove sSecurityManager from nsJSEnvironment.cpp → Clean up various pointless uses of the security manager
I looked at every use of NS_SCRIPTSECURITYMANAGER_CONTRACTID in the code base and figured out if it could be easily eliminated. Some places were entirely unused, while others just needed the system principal or to check if something was the system principal. I left a few places alone in XPCShell-related files because I didn't want to deal with possible build bustage there.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac15badc3a3ef5ce3f9c15c8960a25cfd14227ec
I guess arguably nsContentUtils::IsSystemPrincipal is better, because it is just a comparison rather than a virtual call. (for parts 4 and 5).
Comment on attachment 8885361 [details]
Bug 1379786, part 1 - Remove dead code related to the security manager.

https://reviewboard.mozilla.org/r/156210/#review161814
Attachment #8885361 - Flags: review?(mrbkap) → review+
Comment on attachment 8885362 [details]
Bug 1379786, part 2 - Use nsContentUtils::GetSystemPrincipal in various places.

https://reviewboard.mozilla.org/r/156212/#review161816

::: chrome/nsChromeProtocolHandler.cpp:194
(Diff revision 1)
> -        nsCOMPtr<nsIPrincipal> principal;
> -        rv = securityManager->GetSystemPrincipal(getter_AddRefs(principal));
> -        if (NS_FAILED(rv)) return rv;
> -
>          nsCOMPtr<nsISupports> owner = do_QueryInterface(principal);
>          result->SetOwner(owner);

Can we not simply do `result->SetOwner(nsContentUtils::GetSystemPrincipal())`?
Attachment #8885362 - Flags: review?(mrbkap) → review+
Comment on attachment 8885363 [details]
Bug 1379786, part 3 - Remove sSecurityManager from nsJSEnvironment.cpp.

https://reviewboard.mozilla.org/r/156214/#review161818
Attachment #8885363 - Flags: review?(mrbkap) → review+
Comment on attachment 8885364 [details]
Bug 1379786, part 4 - Use GetIsSystemPrincipal() method instead of going through secman in CHECK_PRINCIPAL_AND_DATA.

https://reviewboard.mozilla.org/r/156216/#review161820

I did not know `CHECK_PRINCIPAL_AND_DATA` before, but I hate it. Is there any reason it can't be a function taking something like a `nsresult (nsIContentPolicy::*func)(...)` and calling that on the policy?
Attachment #8885364 - Flags: review?(mrbkap) → review+
Comment on attachment 8885365 [details]
Bug 1379786, part 5 - Call GetIsSystemPrincipal() rather than keeping alive some refs to the sec man in XUL template code.

https://reviewboard.mozilla.org/r/156218/#review161824
Attachment #8885365 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #10)
> Can we not simply do`result->SetOwner(nsContentUtils::GetSystemPrincipal())`?
Good point. I'll do that.

(In reply to Blake Kaplan (:mrbkap) from comment #12)
> I did not know `CHECK_PRINCIPAL_AND_DATA` before, but I hate it. Is there
> any reason it can't be a function taking something like a `nsresult
> (nsIContentPolicy::*func)(...)` and calling that on the policy?

Probably not. I'll file a follow up bug, as I don't feel like dealing with it at the moment...
Blocks: 1381156
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1db76b40489f
part 1 - Remove dead code related to the security manager. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/08b4b4c7a713
part 2 - Use nsContentUtils::GetSystemPrincipal in various places. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/1a0b6dbd661f
part 3 - Remove sSecurityManager from nsJSEnvironment.cpp. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/185f5128f556
part 4 - Use GetIsSystemPrincipal() method instead of going through secman in CHECK_PRINCIPAL_AND_DATA. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/de8c698222c8
part 5 - Call GetIsSystemPrincipal() rather than keeping alive some refs to the sec man in XUL template code. r=mrbkap
You need to log in before you can comment on or make changes to this bug.