Closed
Bug 1164292
Opened 8 years ago
Closed 8 years ago
Clean up various CAPS pieces and make nsExpandedPrincipal::origin do something useful
Categories
(Core :: Security: CAPS, defect)
Core
Security: CAPS
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(9 files)
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.
Assignee | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=562b39389e25
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=55ad99bcd0b5
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
The existing setup adds a lot of complication and not a lot of value.
Attachment #8605396 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
Losing the NS_DECL_NSIPRINCIPAL isn't great, but I think it's worth it to share more code.
Attachment #8605398 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8605399 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8605400 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8605401 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8605402 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8605403 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #2) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=55ad99bcd0b5 This is green.
Comment 13•8 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?
Attachment #8605394 -
Flags: review?(gkrizsanits) → review+
Updated•8 years ago
|
Attachment #8605396 -
Flags: review?(gkrizsanits) → review+
Updated•8 years ago
|
Attachment #8605397 -
Flags: review?(gkrizsanits) → review+
Comment 14•8 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?
Attachment #8605398 -
Flags: review?(gkrizsanits) → review+
Comment 15•8 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!
Attachment #8605399 -
Flags: review?(gkrizsanits) → review+
Comment 16•8 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!
Attachment #8605400 -
Flags: review?(gkrizsanits) → review+
Updated•8 years ago
|
Attachment #8605401 -
Flags: review?(gkrizsanits) → review+
Updated•8 years ago
|
Attachment #8605402 -
Flags: review?(gkrizsanits) → review+
Updated•8 years ago
|
Attachment #8605403 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 17•8 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.
Comment 18•8 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
Assignee | ||
Comment 19•8 years ago
|
||
Thanks for the reviews!
Comment 20•8 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
Comment 21•8 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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•