Closed Bug 1379786 Opened 7 years ago Closed 7 years ago

Clean up various pointless uses of the security manager

Categories

(Core :: Security: CAPS, enhancement)

Product:
Core
Component:
Security: CAPS
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1379786, part 1 - Remove dead code related to the security manager.
59 bytes, text/x-review-board-request
mrbkap
: review+
Details
Bug 1379786, part 2 - Use nsContentUtils::GetSystemPrincipal in various places.
59 bytes, text/x-review-board-request
mrbkap
: review+
Details
Bug 1379786, part 3 - Remove sSecurityManager from nsJSEnvironment.cpp.
59 bytes, text/x-review-board-request
mrbkap
: review+
Details
Bug 1379786, part 4 - Use GetIsSystemPrincipal() method instead of going through secman in CHECK_PRINCIPAL_AND_DATA.
59 bytes, text/x-review-board-request
mrbkap
: review+
Details
Bug 1379786, part 5 - Call GetIsSystemPrincipal() rather than keeping alive some refs to the sec man in XUL template code.
59 bytes, text/x-review-board-request
mrbkap
: review+
Details 
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.

Attachment

General

Creator:
Created:
Updated:
Size: