1164292 - Clean up various CAPS pieces and make nsExpandedPrincipal::origin do something useful

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

Splitting this out of bug 1163254, since it's a nice independent chunk, and can unblock Christoph sooner.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=562b39389e25

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55ad99bcd0b5

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist refcounting into nsJSPrincipals. v1

This is a special-snowflake reference counting system that's tied to
JSPrincipals, so it makes sense to consolidate this on nsJSPrincipals.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Re-implement dumpImpl in terms of GetScriptLocation. v1

The existing setup adds a lot of complication and not a lot of value.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Rebrand nsBasePrincipal into mozilla::BasePrincipal and give it its own file. v1

The goal here is to provide a common superclass for _all_ the principal
implementations, rather than just nsPrincipal and nsExpandedPrincipal.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Make all nsIPrincipal implementations inherit BasePrincipal and hoist some repeated code. v1

Losing the NS_DECL_NSIPRINCIPAL isn't great, but I think it's worth it to share
more code.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Switch nsIPrincipal::origin to ACString. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 6 - Order the nsEP whitelist array. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 7 - Properly implement nsExpandedPrincipal::GetOrigin. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 8 - Twiddle format of GetScriptLocation to match what we use for GetOrigin. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 9 - Tests. v1

Comment 12

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #2)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=55ad99bcd0b5

This is green.

Comment 13

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8605394 [details] [diff] [review]
Part 1 - Hoist refcounting into nsJSPrincipals. v1

Review of attachment 8605394 [details] [diff] [review]:
-----------------------------------------------------------------

Hurray \o/
one small nit:

::: caps/nsJSPrincipals.cpp
@@ +23,5 @@
> +nsJSPrincipals::AddRef()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  NS_PRECONDITION(int32_t(refcount) >= 0, "illegal refcnt");
> +  nsrefcnt count = ++refcount;

If this is main-thread only why do we need this nsrefcnt tmp var here?

Comment 14

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8605398 [details] [diff] [review]
Part 4 - Make all nsIPrincipal implementations inherit BasePrincipal and hoist some repeated code. v1

Review of attachment 8605398 [details] [diff] [review]:
-----------------------------------------------------------------

I wish I could select more than 5 lines for adding comments...

::: caps/nsNullPrincipal.h
@@ +45,5 @@
> +  NS_IMETHOD GetHashValue(uint32_t* aHashValue) override;
> +  NS_IMETHOD GetURI(nsIURI** aURI) override;
> +  NS_IMETHOD GetDomain(nsIURI** aDomain) override;
> +  NS_IMETHOD SetDomain(nsIURI* aDomain) override;
> +  NS_IMETHOD GetOrigin(char** aOrigin) override;

Now that we have all these methods copy pasted at 4 different places, could we introduce a macro for them?

Comment 15

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8605399 [details] [diff] [review]
Part 5 - Switch nsIPrincipal::origin to ACString. v1

Review of attachment 8605399 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great!

Comment 16

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8605400 [details] [diff] [review]
Part 6 - Order the nsEP whitelist array. v1

Review of attachment 8605400 [details] [diff] [review]:
-----------------------------------------------------------------

A new we're going to need this one day :) Thanks!

Comment 17

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> If this is main-thread only why do we need this nsrefcnt tmp var here?

I'm not entirely sure, but it seems pretty harmless, so I'm just going to copy the existing code verbatim for now.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> @@ +45,5 @@
> > +  NS_IMETHOD GetHashValue(uint32_t* aHashValue) override;
> > +  NS_IMETHOD GetURI(nsIURI** aURI) override;
> > +  NS_IMETHOD GetDomain(nsIURI** aDomain) override;
> > +  NS_IMETHOD SetDomain(nsIURI* aDomain) override;
> > +  NS_IMETHOD GetOrigin(char** aOrigin) override;
> 
> Now that we have all these methods copy pasted at 4 different places, could
> we introduce a macro for them?

My subsequent patches in the next bugs hoist more and more of this stuff into BasePrincipal, so I'm just going to leave it for now.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d99fd0b0a973
https://hg.mozilla.org/integration/mozilla-inbound/rev/2444ca5b0194
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9583ef3d16b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f283549fee9
https://hg.mozilla.org/integration/mozilla-inbound/rev/217e812d2331
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf20e65c4187
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d6fb7d2bd18
https://hg.mozilla.org/integration/mozilla-inbound/rev/9386e2d74b32
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e940b4892ee

Comment 19

[Bobby Holley (:bholley)]

Thanks for the reviews!

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Pulsebot from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f9583ef3d16b
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1f283549fee9

These commits were missing 'override' annotations on GetCsp/SetCsp in
BasePrincipal & nsSystemPrincipal. This causes clang 3.6 & newer to spam
a "-Winconsistent-missing-override" build warning, which busts
--enable-warnings-as-errors builds (with clang 3.6+).

I pushed a followup to annotate these as 'override', with blanket r+
that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a811d943f29

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d99fd0b0a973
https://hg.mozilla.org/mozilla-central/rev/2444ca5b0194
https://hg.mozilla.org/mozilla-central/rev/f9583ef3d16b
https://hg.mozilla.org/mozilla-central/rev/1f283549fee9
https://hg.mozilla.org/mozilla-central/rev/217e812d2331
https://hg.mozilla.org/mozilla-central/rev/cf20e65c4187
https://hg.mozilla.org/mozilla-central/rev/4d6fb7d2bd18
https://hg.mozilla.org/mozilla-central/rev/9386e2d74b32
https://hg.mozilla.org/mozilla-central/rev/6e940b4892ee
https://hg.mozilla.org/mozilla-central/rev/3a811d943f29