Bobby Holley (:bholley) Assignee
	Description • 9 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 1 • 9 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 2 • 9 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 3 • 9 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 4 • 9 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 5 • 9 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 6 • 9 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 12 • 9 years ago

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

This is green.

	Gabor Krizsanits (INACTIVE)
	Comment 13 • 9 years ago

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?

	Gabor Krizsanits (INACTIVE)
	Comment 14 • 9 years ago

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?

	Gabor Krizsanits (INACTIVE)
	Comment 15 • 9 years ago

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!

	Gabor Krizsanits (INACTIVE)
	Comment 16 • 9 years ago

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!

	Bobby Holley (:bholley) Assignee
	Comment 17 • 9 years ago

(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.

	Pulsebot
	Comment 18 • 9 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 19 • 9 years ago

Thanks for the reviews!

	Daniel Holbert [:dholbert]
	Comment 20 • 9 years ago

(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

	Phil Ringnalda (:philor)
	Comment 21 • 9 years ago

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