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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(9 attachments)

14.68 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
8.14 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
6.03 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
12.49 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
21.76 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.79 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.18 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.12 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.96 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
Splitting this out of bug 1163254, since it's a nice independent chunk, and can unblock Christoph sooner.
Blocks: 1129999
Depends on: 1164567
This is a special-snowflake reference counting system that's tied to
JSPrincipals, so it makes sense to consolidate this on nsJSPrincipals.
Attachment #8605394 - Flags: review?(gkrizsanits)
The existing setup adds a lot of complication and not a lot of value.
Attachment #8605396 - Flags: review?(gkrizsanits)
The goal here is to provide a common superclass for _all_ the principal
implementations, rather than just nsPrincipal and nsExpandedPrincipal.
Attachment #8605397 - Flags: review?(gkrizsanits)
Losing the NS_DECL_NSIPRINCIPAL isn't great, but I think it's worth it to share
more code.
Attachment #8605398 - Flags: review?(gkrizsanits)
Attachment #8605400 - Flags: review?(gkrizsanits)
Attachment #8605403 - Flags: review?(gkrizsanits)
(In reply to Bobby Holley (:bholley) from comment #2)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=55ad99bcd0b5

This is green.
Blocks: 1164977
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?
Attachment #8605394 - Flags: review?(gkrizsanits) → review+
Attachment #8605396 - Flags: review?(gkrizsanits) → review+
Attachment #8605397 - Flags: review?(gkrizsanits) → review+
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?
Attachment #8605398 - Flags: review?(gkrizsanits) → review+
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!
Attachment #8605399 - Flags: review?(gkrizsanits) → review+
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!
Attachment #8605400 - Flags: review?(gkrizsanits) → review+
Attachment #8605401 - Flags: review?(gkrizsanits) → review+
Attachment #8605402 - Flags: review?(gkrizsanits) → review+
Attachment #8605403 - Flags: review?(gkrizsanits) → review+
(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.
Thanks for the reviews!
(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
See Also: → 1165835
You need to log in before you can comment on or make changes to this bug.