1340710 - Speed up principal equality and subsumption checks

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[(no longer active)]

I'm planning to speed up nsIPrincipal::Equals a bit.  I think for a subset of our principals, we should be able to create an nsIAtom that hashes a string that represents everything that needs to match for both principals if Equals is to be true.  In BasePrincipal::Equals(), we would first check the nsIAtom pointer of the two principals, and if they both exist we just compare them, otherwise we fall back to the slow path of the double Subsumes() check.

The first useful observation is that currently there is no way for two principals of different kinds to be equal.  Knowing this, we define the atoms for each principal kind:

* For system principal, we can use a static atom.

* For null principal, we can use the uuid as the atom string.

* For expanded principal, since the equality requires one by one matching of the sorted whitelist, we first try to retrieve the atom for each such principal, and then those will be used to generate the atom string.

* For codebase principal, there are some cases that are impossible to support this way.  These are basically all of the special cases in NS_SecurityCompareURIs.

  ** Non-HTTP/HTTPS schemes.  It's unsafe to assume we can use string comparisons for arbitrary URIs (for example javascript URIs) and file:// already needs special handling, so we might as well only do this for HTTP(S).

  ** ORIGIN_IS_FULL_SPEC for Thunderbird and Mozilla suite (see bug 1340677).

In the rest of the cases, we use the lower case triple of scheme, host and port + the OAs as the atom string.  We can include the OAs unconditionally since Equals() never considers document.domain.  All of the nsIURI manipulations must happen exactly as it currently does in NS_SecurityCompareURIs.

Comment 1

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #0)
>   ** Non-HTTP/HTTPS schemes.  It's unsafe to assume we can use string
> comparisons for arbitrary URIs (for example javascript URIs) and file://
> already needs special handling, so we might as well only do this for HTTP(S).

Can javascript: URIs ever be principal origin URLs? They should always inherit a principal by default, and if inheritance is disabled, they should wind up with a null principal.

And for file: URLs, I think we should be able to choose an atom string that will reliably tell us when double subsumes is true, just not when one principal subsumes the other, but not the reverse.

Comment 2

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #1)
> (In reply to :Ehsan Akhgari from comment #0)
> >   ** Non-HTTP/HTTPS schemes.  It's unsafe to assume we can use string
> > comparisons for arbitrary URIs (for example javascript URIs) and file://
> > already needs special handling, so we might as well only do this for HTTP(S).
> 
> Can javascript: URIs ever be principal origin URLs? They should always
> inherit a principal by default, and if inheritance is disabled, they should
> wind up with a null principal.

I'm not 100% sure, but this is super generic code, and I'd rather be safe than sorry...  It's much better to treat some known URIs not all existing kinds except for a blacklist.

> And for file: URLs, I think we should be able to choose an atom string that
> will reliably tell us when double subsumes is true, just not when one
> principal subsumes the other, but not the reverse.

Yeah I suppose we should be able to use the path that nsIFile::Equals uses under the hood to ensure that we will do the same thing as NS_SecurityCompareURIs currently does.  But that's a platform dependent behavior and I'd rather not add knowledge of that grossness to the principal code if we can avoid it.  Do file URIs matter for the use cases that bug 1334263 cares about?  I suppose we may have jar: URIs and such?

Comment 3

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #2)
> Do file URIs matter for the use cases that bug 1334263 cares about?

They might, but I'm not sure how much. We definitely want to support content
scripts in file: URLs, and not handling them here would cause us to take the
slow path for them.

> I suppose we may have jar: URIs and such?

I don't think we'll have jar: URIs in any of the circumstances that matter
here. We'll definitely have resource:, chrome:, and moz-extension: URLs that
are backed by jar: or file: URIs, but they don't follow file: URL origin
rules.

Comment 4

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #3)
> (In reply to :Ehsan Akhgari from comment #2)
> > Do file URIs matter for the use cases that bug 1334263 cares about?
> 
> They might, but I'm not sure how much. We definitely want to support content
> scripts in file: URLs, and not handling them here would cause us to take the
> slow path for them.
> 
> > I suppose we may have jar: URIs and such?
> 
> I don't think we'll have jar: URIs in any of the circumstances that matter
> here. We'll definitely have resource:, chrome:, and moz-extension: URLs that
> are backed by jar: or file: URIs, but they don't follow file: URL origin
> rules.

OK, as long as we have to handle any URL that is backed by file:// we have to support it here otherwise they will all fall back to the slow path (note that NS_SecurityCompareURIs first calls NS_GetInnermostURI so it will see through all nested URIs.)

This shouldn't be too much work, I'll just do it here.  :-)  Thanks for bringing it up!

Comment 5

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #4)
> OK, as long as we have to handle any URL that is backed by file:// we have
> to support it here otherwise they will all fall back to the slow path (note
> that NS_SecurityCompareURIs first calls NS_GetInnermostURI so it will see
> through all nested URIs.)

resource: and moz-extension: URIs don't implement nsINestedURI, so that
shouldn't be an issue. The directory-based same-origin rules that apply to
file: URLs don't apply to those schemes, and applying them would break a lot
of code.

Comment 6

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #5)
> (In reply to :Ehsan Akhgari from comment #4)
> > OK, as long as we have to handle any URL that is backed by file:// we have
> > to support it here otherwise they will all fall back to the slow path (note
> > that NS_SecurityCompareURIs first calls NS_GetInnermostURI so it will see
> > through all nested URIs.)
> 
> resource: and moz-extension: URIs don't implement nsINestedURI, so that
> shouldn't be an issue. The directory-based same-origin rules that apply to
> file: URLs don't apply to those schemes, and applying them would break a lot
> of code.

OK, so perhaps we should consider adding fast-paths for those too, but maybe in a follow-up bug.  Let's remeasure before doing that.

Valentin, do you know what the status of moz-extension URIs is these days?  Are we planning to make them a nested URI?

Comment 7

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #6)
> OK, so perhaps we should consider adding fast-paths for those too, but maybe
> in a follow-up bug.  Let's remeasure before doing that.

There's probably not much point in doing this at all if we don't add fast paths for those, at least for the sake of bug 1334263. Extension content scripts are always expanded principals that inherit from one moz-extension: principal and one content principal.

In the other cases where this matters, we're dealing almost exclusively with the system principal, which we could just special case to do pointer comparison. Its SubsumesInternal check always returns true, so most of the overhead in those comparisons is in unnecessary virtual calls for the double subsumption checks.

Comment 8

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #7)
> (In reply to :Ehsan Akhgari from comment #6)
> > OK, so perhaps we should consider adding fast-paths for those too, but maybe
> > in a follow-up bug.  Let's remeasure before doing that.
> 
> There's probably not much point in doing this at all if we don't add fast
> paths for those, at least for the sake of bug 1334263. Extension content
> scripts are always expanded principals that inherit from one moz-extension:
> principal and one content principal.

Based on your description those principals always fall in the slow path with the patches that I have.

> In the other cases where this matters, we're dealing almost exclusively with
> the system principal, which we could just special case to do pointer
> comparison. Its SubsumesInternal check always returns true, so most of the
> overhead in those comparisons is in unnecessary virtual calls for the double
> subsumption checks.

My patches convert that into a pointer comparison without any virtual calls.  :-)

Comment 9

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #8)
> My patches convert that into a pointer comparison without any virtual calls.
> :-)

Sure, but my point is that if we don't handle moz-extension: URLs, the system
principal case is the only one where this really gains us anything, and there
are simpler ways to handle that.

Comment 10

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #9)
> (In reply to :Ehsan Akhgari from comment #8)
> > My patches convert that into a pointer comparison without any virtual calls.
> > :-)
> 
> Sure, but my point is that if we don't handle moz-extension: URLs, the system
> principal case is the only one where this really gains us anything, and there
> are simpler ways to handle that.

Oh I understand now.  OK, looks like moz-extensions: URLs are just substituting URLs (and hence nsStandardURLs), so I should be able to use the exact same code as I have for HTTP(S) for moz-extension: as well.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

It seems the question has already been answered.

Comment 12

[(no longer active)]

Attachment: Part 1: Remove nsPrincipal::SetURI() since it's unused

Comment 13

[(no longer active)]

Attachment: Part 2: Factor out the logic to retrieve the URI to check in NS_SecurityCompareURIs

Comment 14

[(no longer active)]

Attachment: Part 3: Add a fast path for nsIPrincipal::Equals()

Comment 15

[(no longer active)]

Attachment: Part 4: Clear our dynamic atom before shutdown in leakchecking builds

The overhead of doing this is unnecessary in normal builds,
so we hide the necessary code behind the NS_FREE_PERMANENT_DATA
macro which determines whether we're in a leak checking build.

Comment 16

[(no longer active)]

One thing to note is that I really hate part 4.  I had to do this because netwerk/test/unit_ipc/test_channel_id.js was hitting <http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/xpcom/ds/nsAtomTable.cpp#409> at the end and I realized that this is a problem that will bite the next person who juggles things in a way that we end up with a living principal object past XPCOM shutdown so decided to fix it by doing things properly and not holding the dynamic atoms alive after that point.

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8839723 [details] [diff] [review]
Part 3: Add a fast path for nsIPrincipal::Equals()

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

::: caps/BasePrincipal.cpp
@@ +346,5 @@
>    NS_ENSURE_TRUE(aOther, NS_ERROR_INVALID_ARG);
> +
> +  // If both principals have an equality key, use that for fast comparisons.
> +  // Otherwise fall back to the slow path below.
> +  if (mEqualityKey && Cast(aOther)->mEqualityKey) {

Is there any way for one principal to have a key and the other not to, but for them still to be equal? It seems like there shouldn't be, so we should only need the slow path if neither has an equality key.

::: caps/nsPrincipal.cpp
@@ +108,5 @@
> +  //     This only happens in Thunderbird and SeaMonkey.
> +  //   * If the URI isn't an nsIStandardURL, and also in error cases.
> +  nsAutoCString scheme;
> +  MOZ_ALWAYS_SUCCEEDS(baseURI->GetScheme(scheme));
> +  ToLowerCase(scheme);

This should already be guaranteed.

@@ +147,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (!scheme.EqualsLiteral("http") &&
> +      !scheme.EqualsLiteral("https")) {

Is there a reason we can't handle moz-extension: and resource: here?

Comment 18

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #17)
> Comment on attachment 8839723 [details] [diff] [review]
> Part 3: Add a fast path for nsIPrincipal::Equals()
> 
> Review of attachment 8839723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: caps/BasePrincipal.cpp
> @@ +346,5 @@
> >    NS_ENSURE_TRUE(aOther, NS_ERROR_INVALID_ARG);
> > +
> > +  // If both principals have an equality key, use that for fast comparisons.
> > +  // Otherwise fall back to the slow path below.
> > +  if (mEqualityKey && Cast(aOther)->mEqualityKey) {
> 
> Is there any way for one principal to have a key and the other not to, but
> for them still to be equal? It seems like there shouldn't be, so we should
> only need the slow path if neither has an equality key.

Hmm, I'm not 100% sure.  It seems like at least in cases where we may have returned early due to an error that's a possibility.  Do you see any downside with the check as it is?

> @@ +147,5 @@
> > +    return NS_OK;
> > +  }
> > +
> > +  if (!scheme.EqualsLiteral("http") &&
> > +      !scheme.EqualsLiteral("https")) {
> 
> Is there a reason we can't handle moz-extension: and resource: here?

Oh sorry, I meant to do that and forgot.  New patch upcoming.

Comment 19

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #18)
> Hmm, I'm not 100% sure.  It seems like at least in cases where we may have
> returned early due to an error that's a possibility.  Do you see any
> downside with the check as it is?

Just the small added inefficiency, but I'm not sure how often it's likely to come up in practice.

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8839722 [details] [diff] [review]
Part 2: Factor out the logic to retrieve the URI to check in NS_SecurityCompareURIs

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

::: netwerk/base/nsNetUtil.cpp
@@ +1597,5 @@
>      return mozilla::AddToHash(schemeHash, hostHash, NS_GetRealPort(baseURI));
>  }
>  
> +already_AddRefed<nsIURI>
> +NS_SecurityGetBaseURI(nsIURI* aURI)

BaseURI is a bad name for this, because it causes confusion with, well, base URIs, which are different.

So lets call this NS_GetSecurityURI instead.

Comment 21

[(no longer active)]

Attachment: Part 3: Add a fast path for nsIPrincipal::Equals()

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8839782 [details] [diff] [review]
Part 3: Add a fast path for nsIPrincipal::Equals()

Hm, I'm not really wild about the duplication of code here.

Can't we just atomize the result of GetOrigin instead? Why do we need this special equality key?

Also, we probably want to speed up Subsumes as well as the *ConsideringDomain variants, since those are actually the ones that get called during wrapping etc.

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8839724 [details] [diff] [review]
Part 4: Clear our dynamic atom before shutdown in leakchecking builds

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

(In reply to :Ehsan Akhgari from comment #16)
> One thing to note is that I really hate part 4.  I had to do this because
> netwerk/test/unit_ipc/test_channel_id.js was hitting
> <http://searchfox.org/mozilla-central/rev/
> 12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/xpcom/ds/nsAtomTable.cpp#409> at
> the end and I realized that this is a problem that will bite the next person
> who juggles things in a way that we end up with a living principal object
> past XPCOM shutdown so decided to fix it by doing things properly and not
> holding the dynamic atoms alive after that point.

Hm, is the issue that the principals are leaked, or just that they live longer than the atom table? If the former that seems like something we should fix. If the latter then it seems like we should just shut down the atom table later.

Comment 24

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22)
> Comment on attachment 8839782 [details] [diff] [review]
> Part 3: Add a fast path for nsIPrincipal::Equals()
> 
> Hm, I'm not really wild about the duplication of code here.
> 
> Can't we just atomize the result of GetOrigin instead? Why do we need this
> special equality key?

Where do you mean?  Perhaps for the http/https/moz-extension/resource special case, yes, but I wanted to ensure that the behavior is 100% same as NS_SecurityCompareURI.

> Also, we probably want to speed up Subsumes as well as the
> *ConsideringDomain variants, since those are actually the ones that get
> called during wrapping etc.

Those didn't come up in the profile I was trying to optimize, but yeah it would be nice.  Unfortunately this strategy doesn't work for Subsumes.  We could make it work for EqualsConsideringDomain with an extra atom for that case.  Do you think that's worth it?

Comment 25

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #23)
> Comment on attachment 8839724 [details] [diff] [review]
> Part 4: Clear our dynamic atom before shutdown in leakchecking builds
> 
> Review of attachment 8839724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to :Ehsan Akhgari from comment #16)
> > One thing to note is that I really hate part 4.  I had to do this because
> > netwerk/test/unit_ipc/test_channel_id.js was hitting
> > <http://searchfox.org/mozilla-central/rev/
> > 12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/xpcom/ds/nsAtomTable.cpp#409> at
> > the end and I realized that this is a problem that will bite the next person
> > who juggles things in a way that we end up with a living principal object
> > past XPCOM shutdown so decided to fix it by doing things properly and not
> > holding the dynamic atoms alive after that point.
> 
> Hm, is the issue that the principals are leaked, or just that they live
> longer than the atom table? If the former that seems like something we
> should fix. If the latter then it seems like we should just shut down the
> atom table later.

The latter.  The problem is that it's completely unclear when is a good time when we would have no principals alive.  Did you have a specific suggestion?  I had to do this because I couldn't come up with when this cleanup can happen.

Comment 26

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #24)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22)
> > Comment on attachment 8839782 [details] [diff] [review]
> > Part 3: Add a fast path for nsIPrincipal::Equals()
> > 
> > Hm, I'm not really wild about the duplication of code here.
> > 
> > Can't we just atomize the result of GetOrigin instead? Why do we need this
> > special equality key?
> 
> Where do you mean?  Perhaps for the http/https/moz-extension/resource
> special case, yes, but I wanted to ensure that the behavior is 100% same as
> NS_SecurityCompareURI.

There is lots of code in Gecko and the frontend that assumes that two principals are equal i.f.f. the entire origin (originNoSuffix + suffix) matches. All the storage stuff (which operates off main thread and can't use nsIPrincipal) does this, for example. If there's a case where they aren't equivalent, we should definitely fix that.
 
> > Also, we probably want to speed up Subsumes as well as the
> > *ConsideringDomain variants, since those are actually the ones that get
> > called during wrapping etc.
> 
> Those didn't come up in the profile I was trying to optimize, but yeah it
> would be nice.  Unfortunately this strategy doesn't work for Subsumes.

It does, at least in the affirmative case, which is where performance matters. Equals implies subsumes, so if the atoms compare equal, we're done.

>  We
> could make it work for EqualsConsideringDomain with an extra atom for that
> case.  Do you think that's worth it?

It is worth optimizing SubsumesConsideringDomain. It is probably sufficient to just track whether document.domain set with a boolean on BasePrincipal, and forward to the fast non-domain-considering variant if neither principal has document.domain set.

Comment 27

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #25)
> The latter.  The problem is that it's completely unclear when is a good time
> when we would have no principals alive.  Did you have a specific suggestion?

My suggestion would be to move the call to NS_ShutdownAtomTable() further down in ShutdownXPCOM. If you get all the way to the bottom of the function and the principal is still alive, that sure would look like a leaked principal to me.

Comment 28

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #26)
> (In reply to :Ehsan Akhgari from comment #24)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22)
> > > Comment on attachment 8839782 [details] [diff] [review]
> > > Part 3: Add a fast path for nsIPrincipal::Equals()
> > > 
> > > Hm, I'm not really wild about the duplication of code here.
> > > 
> > > Can't we just atomize the result of GetOrigin instead? Why do we need this
> > > special equality key?
> > 
> > Where do you mean?  Perhaps for the http/https/moz-extension/resource
> > special case, yes, but I wanted to ensure that the behavior is 100% same as
> > NS_SecurityCompareURI.
> 
> There is lots of code in Gecko and the frontend that assumes that two
> principals are equal i.f.f. the entire origin (originNoSuffix + suffix)
> matches. All the storage stuff (which operates off main thread and can't use
> nsIPrincipal) does this, for example. If there's a case where they aren't
> equivalent, we should definitely fix that.

OK, fair.

> > > Also, we probably want to speed up Subsumes as well as the
> > > *ConsideringDomain variants, since those are actually the ones that get
> > > called during wrapping etc.
> > 
> > Those didn't come up in the profile I was trying to optimize, but yeah it
> > would be nice.  Unfortunately this strategy doesn't work for Subsumes.
> 
> It does, at least in the affirmative case, which is where performance
> matters. Equals implies subsumes, so if the atoms compare equal, we're done.

Oh, I see what you mean!  Sure, sounds like a great idea.

> >  We
> > could make it work for EqualsConsideringDomain with an extra atom for that
> > case.  Do you think that's worth it?
> 
> It is worth optimizing SubsumesConsideringDomain. It is probably sufficient
> to just track whether document.domain set with a boolean on BasePrincipal,
> and forward to the fast non-domain-considering variant if neither principal
> has document.domain set.

Sounds good, will do.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #27)
> (In reply to :Ehsan Akhgari from comment #25)
> > The latter.  The problem is that it's completely unclear when is a good time
> > when we would have no principals alive.  Did you have a specific suggestion?
> 
> My suggestion would be to move the call to NS_ShutdownAtomTable() further
> down in ShutdownXPCOM. If you get all the way to the bottom of the function
> and the principal is still alive, that sure would look like a leaked
> principal to me.

This turned out to be a leak in the test.  Filed bug 1341954 for that.

Comment 29

[(no longer active)]

Comment on attachment 8839722 [details] [diff] [review]
Part 2: Factor out the logic to retrieve the URI to check in NS_SecurityCompareURIs

I won't need this any more.

Comment 30

[(no longer active)]

Attachment: Part 2: Add a fast path for nsIPrincipal::Equals() and nsIPrincipal::EqualsConsideringDomain()

Comment 31

[(no longer active)]

Attachment: Part 3: Add a fast path for nsIPrincipal::EqualsConsideringDomain() and nsIPrincipal::SubsumesConsideringDomain()

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8840271 [details] [diff] [review]
Part 2: Add a fast path for nsIPrincipal::Equals() and nsIPrincipal::EqualsConsideringDomain()

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

::: caps/nsPrincipal.cpp
@@ +98,5 @@
> +  if (!baseURI) {
> +    return NS_OK;
> +  }
> +
> +  // In some special cases we cannot create an atom for speeding up equality

Again, why aren't we delegating to GetOrigin to make this decision? Same for all the other principal types. See the code at http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/caps/nsPrincipal.cpp#129

I think this work can all just happen in BasePrincipal.

Comment 33

[Bobby Holley (:bholley)]

Also, assuming we go that route, we should probably just store mOriginNoSuffix and mOriginSuffix as atoms on the BasePrincipal and fix up the getters to use that.

Comment 34

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #32)
> Comment on attachment 8840271 [details] [diff] [review]
> Part 2: Add a fast path for nsIPrincipal::Equals() and
> nsIPrincipal::EqualsConsideringDomain()
> 
> Review of attachment 8840271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: caps/nsPrincipal.cpp
> @@ +98,5 @@
> > +  if (!baseURI) {
> > +    return NS_OK;
> > +  }
> > +
> > +  // In some special cases we cannot create an atom for speeding up equality
> 
> Again, why aren't we delegating to GetOrigin to make this decision? Same for
> all the other principal types. See the code at
> http://searchfox.org/mozilla-central/rev/
> b1044cf7c2000c3e75e8181e893236a940c8b6d2/caps/nsPrincipal.cpp#129
> 
> I think this work can all just happen in BasePrincipal.

I have to ask again, what do you mean exactly?  Can you please point to the specific part of my code that you think I can remove and hand off to GetOrigin()?  GetOrigin() has no special handling of file:// URIs (with or without strict file origin policy) so just blindly replacing all of this code with that will definitely do the wrong thing.  Same with ORIGIN_IS_FULL_SPEC, GetOrigin() is unaware of that, though I think that may be OK since the full spec of such origins is the correct equality key.  What about javascript: URIs, can we have codebase principals with such a codebase URI?  If yes, wouldn't using GetOrigin() cause a security problem there?

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #33)
> Also, assuming we go that route, we should probably just store
> mOriginNoSuffix and mOriginSuffix as atoms on the BasePrincipal and fix up
> the getters to use that.

Another issue is that subsumption of codebase principals is the only subsumption that takes OAs into account.  You're suggesting to do this all in BasePrincipal, which means we'd need a virtual call to Kind() when comparing principals to get the kind of the principal to know whether or not take the suffix into account, which isn't really a good outcome because the virtual calls are already showing up in this hot code.  Right now the only virtual call my patch imposes is the one to call Equals() and the like.  I prefer to not add more cost to the fast path if possible.

One way to avoid that would be to also have an mEqualityKey that points to the same atom as mOriginNoSuffix for non-codebase principals, and to an atom from the concatenation of these two for codebase principals, so that at comparison site we always have direct access to the right atom.  Does that sound like a good compromise?

Comment 35

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #34)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #32)
> > Comment on attachment 8840271 [details] [diff] [review]
> > Part 2: Add a fast path for nsIPrincipal::Equals() and
> > nsIPrincipal::EqualsConsideringDomain()
> > 
> > Review of attachment 8840271 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: caps/nsPrincipal.cpp
> > @@ +98,5 @@
> > > +  if (!baseURI) {
> > > +    return NS_OK;
> > > +  }
> > > +
> > > +  // In some special cases we cannot create an atom for speeding up equality
> > 
> > Again, why aren't we delegating to GetOrigin to make this decision? Same for
> > all the other principal types. See the code at
> > http://searchfox.org/mozilla-central/rev/
> > b1044cf7c2000c3e75e8181e893236a940c8b6d2/caps/nsPrincipal.cpp#129
> > 
> > I think this work can all just happen in BasePrincipal.
> 
> I have to ask again, what do you mean exactly?

Sorry, I thought we understood each other after the top of comment 28.

>  Can you please point to the
> specific part of my code that you think I can remove and hand off to
> GetOrigin()?

My point is that we already have lots of code in firefox that assumes that doing a string comparison of the results of GetOrigin (if it doesn't throw) is equivalent to doing Equals().

>  GetOrigin() has no special handling of file:// URIs (with or
> without strict file origin policy) so just blindly replacing all of this
> code with that will definitely do the wrong thing.

It's true that the code doesn't handle the non-strict file URI origin policy - we should probably fix that before doing this. But otherwise the code works just fine for file URIs. The HostPort of a file URI is the entire file path, so GetOrigin just returns the path (which is what we want).

I thought we had tests for this but I don't see them. We should probably add them in test_origin.js.

>  Same with
> ORIGIN_IS_FULL_SPEC, GetOrigin() is unaware of that, though I think that may
> be OK since the full spec of such origins is the correct equality key.

Given that this feature doesn't impact firefox, I don't really care about it. We should just file a followup bug and needinfo the TB/Suite folks to make sure that their URIs quack in a way that makes GetOrigin do the right thing.

>  What
> about javascript: URIs, can we have codebase principals with such a codebase
> URI?  If yes, wouldn't using GetOrigin() cause a security problem there?

Javascript URIs do not implement nsIStandardURL to my knowledge.

In general, I'm aware that the code in GetOrigin is hard to follow, and would certainly be in favor of cleaning it up a bit if we're going to rely on it more. But we certainly rely on it in the status quo.

> 
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #33)
> > Also, assuming we go that route, we should probably just store
> > mOriginNoSuffix and mOriginSuffix as atoms on the BasePrincipal and fix up
> > the getters to use that.
> 
> Another issue is that subsumption of codebase principals is the only
> subsumption that takes OAs into account.  You're suggesting to do this all
> in BasePrincipal, which means we'd need a virtual call to Kind() when
> comparing principals to get the kind of the principal to know whether or not
> take the suffix into account, which isn't really a good outcome because the
> virtual calls are already showing up in this hot code.  Right now the only
> virtual call my patch imposes is the one to call Equals() and the like.  I
> prefer to not add more cost to the fast path if possible.

We should solve this by just passing the enum to the superclass in the constructor and making Kind() non-virtual.

Comment 36

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #35)
> >  What
> > about javascript: URIs, can we have codebase principals with such a codebase
> > URI?  If yes, wouldn't using GetOrigin() cause a security problem there?
> 
> Javascript URIs do not implement nsIStandardURL to my knowledge.

javascript: URIs should never be used as an origin in a codebase principal, anyway. They always either inherit a principal, or fall back to a null principal. Same for (or similar) data: URIs, about: URIs, and a few other schemes. Can we just add some assertions to guarantee this rather than worrying about it as an unexpected corner case?

Comment 37

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #35)
> >  GetOrigin() has no special handling of file:// URIs (with or
> > without strict file origin policy) so just blindly replacing all of this
> > code with that will definitely do the wrong thing.
> 
> It's true that the code doesn't handle the non-strict file URI origin policy
> - we should probably fix that before doing this.

Oh, OK, it wasn't clear to me that we want to change the existing behavior here.  But fixing it sounds OK to me.  What should GetOrigin() return in this case, given that all file:// URIs are same-origin?

> But otherwise the code
> works just fine for file URIs. The HostPort of a file URI is the entire file
> path, so GetOrigin just returns the path (which is what we want).

Ah, I was missing this part, partly why I was confused about what you mean.

> >  Same with
> > ORIGIN_IS_FULL_SPEC, GetOrigin() is unaware of that, though I think that may
> > be OK since the full spec of such origins is the correct equality key.
> 
> Given that this feature doesn't impact firefox, I don't really care about
> it. We should just file a followup bug and needinfo the TB/Suite folks to
> make sure that their URIs quack in a way that makes GetOrigin do the right
> thing.

I double checked, since GetOrigin() will return the full spec we'll automatically do the right thing.  (Same reason why GetOrigin doesn't currently handle it.)

> > Another issue is that subsumption of codebase principals is the only
> > subsumption that takes OAs into account.  You're suggesting to do this all
> > in BasePrincipal, which means we'd need a virtual call to Kind() when
> > comparing principals to get the kind of the principal to know whether or not
> > take the suffix into account, which isn't really a good outcome because the
> > virtual calls are already showing up in this hot code.  Right now the only
> > virtual call my patch imposes is the one to call Equals() and the like.  I
> > prefer to not add more cost to the fast path if possible.
> 
> We should solve this by just passing the enum to the superclass in the
> constructor and making Kind() non-virtual.

OK.

(In reply to Kris Maglione [:kmag] from comment #36)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #35)
> > >  What
> > > about javascript: URIs, can we have codebase principals with such a codebase
> > > URI?  If yes, wouldn't using GetOrigin() cause a security problem there?
> > 
> > Javascript URIs do not implement nsIStandardURL to my knowledge.
> 
> javascript: URIs should never be used as an origin in a codebase principal,
> anyway. They always either inherit a principal, or fall back to a null
> principal. Same for (or similar) data: URIs, about: URIs, and a few other
> schemes. Can we just add some assertions to guarantee this rather than
> worrying about it as an unexpected corner case?

I'll add an assertion to catch them all.

Comment 38

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #37)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #35)
> > >  GetOrigin() has no special handling of file:// URIs (with or
> > > without strict file origin policy) so just blindly replacing all of this
> > > code with that will definitely do the wrong thing.
> > 
> > It's true that the code doesn't handle the non-strict file URI origin policy
> > - we should probably fix that before doing this.
> 
> Oh, OK, it wasn't clear to me that we want to change the existing behavior
> here.  But fixing it sounds OK to me.  What should GetOrigin() return in
> this case, given that all file:// URIs are same-origin?

I don't really care given that it's not a supported configuration, but I suppose it might be nice to make it greppable. file://UNIVERSAL_FILE_URI_ORIGIN maybe?

Comment 39

[(no longer active)]

Attachment: Part 2: De-virtualize BasePrincipal::Kind()

Comment 40

[(no longer active)]

Attachment: Part 3: Add an assertion to ensure that codebase principals are never constructed with URI schemes such as javascript:, about: and data:

Comment 41

[(no longer active)]

Attachment: Part 4: Fix nsIPrincipal::GetOrigin()'s handling of non-strict file:// URI origin policy

Comment 42

[(no longer active)]

Attachment: Part 5: Store BasePrincipal::{mOriginNoSuffix,mOriginSuffix} as a pair of atoms

This has the nice side effect of making nsIPrincipal::GetOrigin() a bit faster
by avoiding computing the origin each time.

Comment 43

[(no longer active)]

Attachment: Part 6: Add a fast path for nsIPrincipal::Equals() and nsIPrincipal::EqualsConsideringDomain()

Comment 44

[(no longer active)]

Attachment: Part 7: Add a fast path for nsIPrincipal::EqualsConsideringDomain() and nsIPrincipal::SubsumesConsideringDomain()

Comment 45

[(no longer active)]

Attachment: Part 8: Speed up the OriginAttributes comparison in BasePrincipal::Subsumes()

Comment 46

[Bobby Holley (:bholley)]

Comment on attachment 8841131 [details] [diff] [review]
Part 2: De-virtualize BasePrincipal::Kind()

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

::: caps/BasePrincipal.cpp
@@ +299,5 @@
>    return !!attrs.mPrivateBrowsingId;
>  }
>  
>  BasePrincipal::BasePrincipal()
> +  // mKind is initialized in the child classes.

This seems kind of error-prone. Why isn't this an argument to the BasePrincipal constructor?

Comment 47

[Bobby Holley (:bholley)]

Comment on attachment 8841132 [details] [diff] [review]
Part 3: Add an assertion to ensure that codebase principals are never constructed with URI schemes such as javascript:, about: and data:

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

Nice idea. r=me with that fix.

::: caps/nsPrincipal.cpp
@@ +93,5 @@
> +  // GetProtocolFlags function.
> +  bool hasFlag;
> +  Unused << hasFlag; // silence possible compiler warnings.
> +  MOZ_DIAGNOSTIC_ASSERT(
> +      NS_FAILED(NS_URIChainHasFlags(aCodebase,

It's not obvious to me how to interpret a failure in NS_URIChainHasFlags. I think we should assert NS_SUCCEEDED && !hasFlag.

Comment 48

[Bobby Holley (:bholley)]

Comment on attachment 8841131 [details] [diff] [review]
Part 2: De-virtualize BasePrincipal::Kind()

Sorry, meant to cancel review on this one.

Comment 49

[Bobby Holley (:bholley)]

Comment on attachment 8841133 [details] [diff] [review]
Part 4: Fix nsIPrincipal::GetOrigin()'s handling of non-strict file:// URI origin policy

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

Thanks for fixing this!

Comment 50

[Bobby Holley (:bholley)]

Comment on attachment 8841134 [details] [diff] [review]
Part 5: Store BasePrincipal::{mOriginNoSuffix,mOriginSuffix} as a pair of atoms

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

::: caps/BasePrincipal.cpp
@@ +731,5 @@
> +  MOZ_ASSERT(!mOriginSuffix);
> +
> +  // First compute the origin without the suffix.
> +  nsAutoCString originNoSuffix;
> +  GetOriginInternal(originNoSuffix);

Hm, I was under the impression that nsPrincipal::GetOriginInternal can in fact fail (see the cases in that function that return failure). I don't have an example offhand, but it's not immediately obvious to me that every non-well-behaved URI scheme also has URI_INHERITS_SECURITY_CONTEXT.

So we should either prove that GetOriginInternal will never fail and then assert that it doesn't (it would be great we could actually do this!), or handle the case here where mOriginNoSuffix is null (and therefore return the appropriate error code from GetOrigin/GetOriginNoSuffix, and fall back to NS_SecurityCompareURIs for equality/subsumes checks).

::: caps/BasePrincipal.h
@@ +298,5 @@
>    friend class ::nsExpandedPrincipal;
>  
> +  // Internal function, should be called after the constructor of a sub-class is
> +  // called.  We can't call this from within a constructor as this function
> +  // relies on being able to call virtual functions.

Well, more precisely we can't call it from within BasePrincipal's constructor. It would be totally fine in the virtual function sense to call this from the derived constructors, but the real problem is that principals use Init() to set up state and that state hasn't been set up yet.

I think it would be a bit clearer to call this FinishInit(), and call it as the last step of each of the derived Init() methods.

::: caps/nsExpandedPrincipal.h
@@ +18,5 @@
>  public:
>    nsExpandedPrincipal(nsTArray<nsCOMPtr<nsIPrincipal>> &aWhiteList,
>                        const mozilla::OriginAttributes& aAttrs);
>  
> +  void Init();

Hm, this is a bit error-prone. Can you make the constructor private and expose a ::Create method that constructs and initializes?

::: caps/nsSystemPrincipal.h
@@ +42,5 @@
>    }
>  
>    virtual nsresult GetScriptLocation(nsACString &aStr) override;
>  
> +  void Init();

Even though there's only one callsite, let's use a static ::Create method for this one too.

Comment 51

[Bobby Holley (:bholley)]

Comment on attachment 8841135 [details] [diff] [review]
Part 6: Add a fast path for nsIPrincipal::Equals() and nsIPrincipal::EqualsConsideringDomain()

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

As noted above, we'll either need to make GetOriginInternal infallible, or this will need to consider the possibility that mOriginNoSuffix is null.

::: caps/BasePrincipal.cpp
@@ +351,5 @@
> +  // Two principals are considered to be equal if their origins are the same.
> +  // If the two principals are codebase principals, their origin attributes
> +  // (aka the origin suffix) must also match.
> +  // If the two principals are null principals, they're only equal if they're
> +  // the same object.

The first thing we should do in this function is compare the kind. If the kinds aren't equal, we can bail out.

@@ +353,5 @@
> +  // (aka the origin suffix) must also match.
> +  // If the two principals are null principals, they're only equal if they're
> +  // the same object.
> +  auto other = Cast(aOther);
> +  if (Kind() == eNullPrincipal && other->Kind() == eNullPrincipal) {

I think it's clearest to do the same treatment for system principal.

@@ +359,5 @@
> +  } else if (Kind() == eCodebasePrincipal && other->Kind() == eCodebasePrincipal) {
> +    *aResult = mOriginNoSuffix == other->mOriginNoSuffix &&
> +               mOriginSuffix == other->mOriginSuffix;
> +  } else {
> +    *aResult = mOriginNoSuffix == other->mOriginNoSuffix;

Why do we need this branch?

@@ +414,5 @@
> +    }
> +  } else if (mOriginNoSuffix == other->mOriginNoSuffix) {
> +    *aResult = true;
> +    return NS_OK;
> +  }

It seems like all of this should just delegate to the logic in Equals(), which should probably be split out into a helper to avoid a second virtual call.

Comment 52

[Bobby Holley (:bholley)]

Comment on attachment 8841144 [details] [diff] [review]
Part 8: Speed up the OriginAttributes comparison in BasePrincipal::Subsumes()

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

To be extra safe, please assert that mOriginSuffix is non-null.

r=me with that.

Comment 53

[Bobby Holley (:bholley)]

Comment on attachment 8841136 [details] [diff] [review]
Part 7: Add a fast path for nsIPrincipal::EqualsConsideringDomain() and nsIPrincipal::SubsumesConsideringDomain()

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

r=me with that.

::: caps/BasePrincipal.cpp
@@ +375,5 @@
> +  // If neither of the principals have document.domain set, we use the fast path
> +  // in Equals().  Otherwise, we fall back to the slow path below.
> +  auto other = Cast(aOther);
> +  if (!mDomainSet && !other->mDomainSet) {
> +    return Equals(aOther, aResult);

We should make this a non-virtual helper.

@@ +439,5 @@
> +  // If neither of the principals have document.domain set, we hand off to
> +  // Subsumes() which has fast paths for some special cases. Otherwise, we fall
> +  // back to the slow path below.
> +  if (!mDomainSet && !Cast(aOther)->mDomainSet) {
> +    return Subsumes(aOther, aResult);

This too.

Comment 54

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #50)
> ::: caps/BasePrincipal.cpp
> @@ +731,5 @@
> > +  MOZ_ASSERT(!mOriginSuffix);
> > +
> > +  // First compute the origin without the suffix.
> > +  nsAutoCString originNoSuffix;
> > +  GetOriginInternal(originNoSuffix);
> 
> Hm, I was under the impression that nsPrincipal::GetOriginInternal can in
> fact fail (see the cases in that function that return failure). I don't have
> an example offhand, but it's not immediately obvious to me that every
> non-well-behaved URI scheme also has URI_INHERITS_SECURITY_CONTEXT.
> 
> So we should either prove that GetOriginInternal will never fail and then
> assert that it doesn't (it would be great we could actually do this!), or
> handle the case here where mOriginNoSuffix is null (and therefore return the
> appropriate error code from GetOrigin/GetOriginNoSuffix, and fall back to
> NS_SecurityCompareURIs for equality/subsumes checks).

It turns out that GetOriginInternal can fail and there is nothing that we can do about it.  For example, browser_cookies_exceptions.js runs this code <http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/browser/components/preferences/permissions.js#96> where we attempt to create a URI and principal out of a string like "localhost:12345", which parses as a simple URI with scheme localhost.  If you try to call GetOriginInternal on this principal it will fail because nsSimpleURI::GetHostPath() always fails.

We should just deal with this like my older patches did, make mOriginNoSuffix null in this case and fall back to the slow path.

I'm not sure I understand the part of your comment about URI_INHERITS_SECURITY_CONTEXT.  The significance of the URI_INHERITS_SECURITY_CONTEXT assertion in my patch is to basically ensure codebase principals are always created through createCodebasePrincipal where we have this check <http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/caps/BasePrincipal.cpp#646>.  There's no connection to whether or not GetOriginInternal fails here.

Comment 55

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #54)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #50)
> > ::: caps/BasePrincipal.cpp
> > @@ +731,5 @@
> > > +  MOZ_ASSERT(!mOriginSuffix);
> > > +
> > > +  // First compute the origin without the suffix.
> > > +  nsAutoCString originNoSuffix;
> > > +  GetOriginInternal(originNoSuffix);
> > 
> > Hm, I was under the impression that nsPrincipal::GetOriginInternal can in
> > fact fail (see the cases in that function that return failure). I don't have
> > an example offhand, but it's not immediately obvious to me that every
> > non-well-behaved URI scheme also has URI_INHERITS_SECURITY_CONTEXT.
> > 
> > So we should either prove that GetOriginInternal will never fail and then
> > assert that it doesn't (it would be great we could actually do this!), or
> > handle the case here where mOriginNoSuffix is null (and therefore return the
> > appropriate error code from GetOrigin/GetOriginNoSuffix, and fall back to
> > NS_SecurityCompareURIs for equality/subsumes checks).
> 
> It turns out that GetOriginInternal can fail and there is nothing that we
> can do about it.  For example, browser_cookies_exceptions.js runs this code
> <http://searchfox.org/mozilla-central/rev/
> 4039fb4c5833706f6880763de216974e00ba096c/browser/components/preferences/
> permissions.js#96> where we attempt to create a URI and principal out of a
> string like "localhost:12345", which parses as a simple URI with scheme
> localhost.  If you try to call GetOriginInternal on this principal it will
> fail because nsSimpleURI::GetHostPath() always fails.
> 
> We should just deal with this like my older patches did, make
> mOriginNoSuffix null in this case and fall back to the slow path.

That sounds like the right thing to me.

> I'm not sure I understand the part of your comment about
> URI_INHERITS_SECURITY_CONTEXT.  The significance of the
> URI_INHERITS_SECURITY_CONTEXT assertion in my patch is to basically ensure
> codebase principals are always created through createCodebasePrincipal where
> we have this check
> <http://searchfox.org/mozilla-central/rev/
> 4039fb4c5833706f6880763de216974e00ba096c/caps/BasePrincipal.cpp#646>. 
> There's no connection to whether or not GetOriginInternal fails here.

That matches my interpretation. I just thought that you might have been assuming there was a connection, since you weren't checking the result of GetOriginInternal.

Comment 56

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #54)
> It turns out that GetOriginInternal can fail and there is nothing that we
> can do about it.  For example, browser_cookies_exceptions.js runs this code
> <http://searchfox.org/mozilla-central/rev/
> 4039fb4c5833706f6880763de216974e00ba096c/browser/components/preferences/
> permissions.js#96> where we attempt to create a URI and principal out of a
> string like "localhost:12345", which parses as a simple URI with scheme
> localhost.  If you try to call GetOriginInternal on this principal it will
> fail because nsSimpleURI::GetHostPath() always fails.

Hm. Is there a reason we can't just prevent these from being used as codebase principal origin URLs altogether? In other words, throw when trying to create the principal, rather than when trying to construct the origin string?

Comment 57

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #56)
> (In reply to :Ehsan Akhgari from comment #54)
> > It turns out that GetOriginInternal can fail and there is nothing that we
> > can do about it.  For example, browser_cookies_exceptions.js runs this code
> > <http://searchfox.org/mozilla-central/rev/
> > 4039fb4c5833706f6880763de216974e00ba096c/browser/components/preferences/
> > permissions.js#96> where we attempt to create a URI and principal out of a
> > string like "localhost:12345", which parses as a simple URI with scheme
> > localhost.  If you try to call GetOriginInternal on this principal it will
> > fail because nsSimpleURI::GetHostPath() always fails.
> 
> Hm. Is there a reason we can't just prevent these from being used as
> codebase principal origin URLs altogether? In other words, throw when trying
> to create the principal, rather than when trying to construct the origin
> string?

I would certainly love if we could do that. Just needs somebody to make it happen and deal with the fallout.

Comment 58

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #57)
> (In reply to Kris Maglione [:kmag] from comment #56)
> > (In reply to :Ehsan Akhgari from comment #54)
> > > It turns out that GetOriginInternal can fail and there is nothing that we
> > > can do about it.  For example, browser_cookies_exceptions.js runs this code
> > > <http://searchfox.org/mozilla-central/rev/
> > > 4039fb4c5833706f6880763de216974e00ba096c/browser/components/preferences/
> > > permissions.js#96> where we attempt to create a URI and principal out of a
> > > string like "localhost:12345", which parses as a simple URI with scheme
> > > localhost.  If you try to call GetOriginInternal on this principal it will
> > > fail because nsSimpleURI::GetHostPath() always fails.
> > 
> > Hm. Is there a reason we can't just prevent these from being used as
> > codebase principal origin URLs altogether? In other words, throw when trying
> > to create the principal, rather than when trying to construct the origin
> > string?
> 
> I would certainly love if we could do that. Just needs somebody to make it
> happen and deal with the fallout.

I'm gonna start saying no, sorry.  There has been enough scope creep here already.  :-)  I think the slight change in the semantics we're making in this bug is probably enough risk to accumulate for an optimization, we should stop making more semantic changes here.  (And on a more practical grounds, I do need to be able to land my patches here some day, and I'm much further away from that point now than when I started to work on this...)

If someone files a follow-up for that, I may take a look later in my spare time.

Comment 59

[Andrea Marchesini [:baku]]

Ehsan, funny thing, we are working on similar code. See bug 1340163, but I also implement the correct error propagation for origin computation and I fixed all the tests: my patch is green on try and it's ready to land. Smaug is going to review it today. Ping me on IRC so we discuss what lands first and who will rebase his patches.

Comment 60

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini [:baku] from comment #59)
> Ehsan, funny thing, we are working on similar code. See bug 1340163, but I
> also implement the correct error propagation for origin computation and I
> fixed all the tests: my patch is green on try and it's ready to land. Smaug
> is going to review it today. Ping me on IRC so we discuss what lands first
> and who will rebase his patches.

Per IRC discussion, I would like the patches in this bug (Ehsan's) to land first.

Comment 61

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #58)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #57)
> > (In reply to Kris Maglione [:kmag] from comment #56)
> > > Hm. Is there a reason we can't just prevent these from being used as
> > > codebase principal origin URLs altogether? In other words, throw when trying
> > > to create the principal, rather than when trying to construct the origin
> > > string?
> >
> > I would certainly love if we could do that. Just needs somebody to make it
> > happen and deal with the fallout.
>
> I'm gonna start saying no, sorry.  There has been enough scope creep here
> already.  :-)

Fair enough. I think it makes more sense as a separate bug, but I think it
makes sense either way, especially since it would simplify this code.

> I think the slight change in the semantics we're making in this bug is
> probably enough risk to accumulate for an optimization, we should stop
> making more semantic changes here.

I'm not actually sure that would be a semantic change at all. The test that
you linked tests that GetOrigin throws when "localhost:12345" is incorrectly
parsed as a URL. But any principal being created with that origin is a bug,
and I don't think we have any defined semantics for how such principals would
be handled.

Comment 62

[(no longer active)]

Andrea, sorry for the conflict.  :(  I looked at your patches but not very closely, and you had moved a lot of code around, so I couldn't easily understand what had changed, so I can't actually say how similar our changes are.  But from your changes to permissions.js, I think our patches are definitely not equivalent, since my patch doesn't change the behavior that the test exercising this code hits.  I'll try to work on the remaining test failures tonight and see how many more I can fix.  There is currently one timeout and a few assertions that I still need to look into...  (Note the patches in the bug aren't 100% current yet.)

Comment 63

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #61)
> (In reply to :Ehsan Akhgari from comment #58)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #57)
> > > (In reply to Kris Maglione [:kmag] from comment #56)
> > > > Hm. Is there a reason we can't just prevent these from being used as
> > > > codebase principal origin URLs altogether? In other words, throw when trying
> > > > to create the principal, rather than when trying to construct the origin
> > > > string?
> > >
> > > I would certainly love if we could do that. Just needs somebody to make it
> > > happen and deal with the fallout.
> >
> > I'm gonna start saying no, sorry.  There has been enough scope creep here
> > already.  :-)
> 
> Fair enough. I think it makes more sense as a separate bug, but I think it
> makes sense either way, especially since it would simplify this code.

Yeah I definitely agree!  Interpreting "locahost" as a scheme in localhost:12345 is silly.  :-)

> > I think the slight change in the semantics we're making in this bug is
> > probably enough risk to accumulate for an optimization, we should stop
> > making more semantic changes here.
> 
> I'm not actually sure that would be a semantic change at all. The test that
> you linked tests that GetOrigin throws when "localhost:12345" is incorrectly
> parsed as a URL. But any principal being created with that origin is a bug,
> and I don't think we have any defined semantics for how such principals would
> be handled.

I agree that this case is a bug, but we have _a_ behavior in reality here which code can be depending on, and I'm worried about things such as an add-on relying on some (possibly incorrect even) behavior that we have when such a principal exists.  This is what I mean by a semantic change.  Maybe I'm worrying about it too much, but I've seen too many of these unexpected regressions to be optimistic about changes to code that is so common to so many different parts of the code base.  :(  Not saying that we shouldn't do it at all, just not as part of an optimization.

Comment 64

[Andrea Marchesini [:baku]]

> Yeah I definitely agree!  Interpreting "locahost" as a scheme in
> localhost:12345 is silly.  :-)
> 
> > > I think the slight change in the semantics we're making in this bug is
> > > probably enough risk to accumulate for an optimization, we should stop
> > > making more semantic changes here.

Note that all this is already fixed in my patches. I would recommend you to do no spend to much time on it, or at least take this part from my patches.

My approach is: any principal _must_ have a valid origin. Currently you can create an invalid principal from a valid URL; the error handling is moved into GetOrigin(). My approach is to move the creation of the origin into the CTOR of the principal, plus, in case the creation of the origin fails, it's better to return a nullprincipal.

Note that there are many other tests who will fail if getOrigin do not throw. For instance something related to resource: URLs loaded in workers.

Comment 65

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #51)
> @@ +359,5 @@
> > +  } else if (Kind() == eCodebasePrincipal && other->Kind() == eCodebasePrincipal) {
> > +    *aResult = mOriginNoSuffix == other->mOriginNoSuffix &&
> > +               mOriginSuffix == other->mOriginSuffix;
> > +  } else {
> > +    *aResult = mOriginNoSuffix == other->mOriginNoSuffix;
> 
> Why do we need this branch?

For expanded principals.

Comment 66

[(no longer active)]

Attachment: Part 2: De-virtualize BasePrincipal::Kind()

Comment 67

[(no longer active)]

Attachment: Part 3: Add an assertion to ensure that codebase principals are never constructed with URI schemes such as javascript:, about: and data:

Comment 68

[(no longer active)]

Attachment: Part 4: Fix nsIPrincipal::GetOrigin()'s handling of non-strict file:// URI origin policy

Comment 69

[(no longer active)]

Attachment: Part 5: Make nsIPrincipal.origin throw for about:blank codebase URI principals

Two about:blank codebase URI principals are only equal if their
object identities are the same, but not if their string
serializations happen to be equal (as they always will be.)  In order
to ensure that we always get this right in places where we compare
the origin properties of two principals to check for their equality,
we should ensure that the origin getter would throw so that we never
incorrectly conclude that two such principals are equal.

We will soon start returning a null principal instead of a codebase
principal under this situation.

Comment 70

[(no longer active)]

Attachment: Part 6: Store BasePrincipal::{mOriginNoSuffix,mOriginSuffix} as a pair of atoms

This has the nice side effect of making nsIPrincipal::GetOrigin() a bit faster
by avoiding computing the origin each time.

Comment 71

[(no longer active)]

Attachment: Part 7: Add a fast path for nsIPrincipal::Equals() and nsIPrincipal::EqualsConsideringDomain()

Comment 72

[(no longer active)]

Attachment: Part 8: Add a fast path for nsIPrincipal::EqualsConsideringDomain() and nsIPrincipal::SubsumesConsideringDomain()

Comment 73

[(no longer active)]

Attachment: Part 9: Speed up the OriginAttributes comparison in BasePrincipal::Subsumes()

Comment 74

[Bobby Holley (:bholley)]

Comment on attachment 8843478 [details] [diff] [review]
Part 4: Fix nsIPrincipal::GetOrigin()'s handling of non-strict file:// URI origin policy

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

This patch looks identical to the one I already r+ed?

Comment 75

[Bobby Holley (:bholley)]

Comment on attachment 8843479 [details] [diff] [review]
Part 5: Make nsIPrincipal.origin throw for about:blank codebase URI principals

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

::: caps/nsPrincipal.cpp
@@ +165,5 @@
>    // to handle.
>    bool isBehaved;
>    if ((NS_SUCCEEDED(origin->SchemeIs("about", &isBehaved)) && isBehaved) ||
> +      (NS_SUCCEEDED(origin->SchemeIs("moz-safe-about", &isBehaved)) && isBehaved &&
> +       !origin->GetSpecOrDefault().EqualsLiteral("moz-safe-about:blank")) ||

Please add a comment saying that we generally consider about:foo to be same-origin with about:foo, but don't for about:blank URIs, which can be generated by lots of different sources.

Comment 76

[Bobby Holley (:bholley)]

Comment on attachment 8843480 [details] [diff] [review]
Part 6: Store BasePrincipal::{mOriginNoSuffix,mOriginSuffix} as a pair of atoms

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

::: caps/BasePrincipal.cpp
@@ +736,5 @@
> +    // If GetOriginInternal fails, we will get a null atom for mOriginNoSuffix,
> +    // which we deal with anywhere mOriginNoSuffix is used.
> +    // Once this is made infallible we can remove those null checks.
> +    mOriginNoSuffix = nullptr;
> +    mOriginSuffix = nullptr;

This contradicts the code in GetOriginSuffix that asserts that mOriginSuffix is non-null, right?

I could go either way on this one, but I think I'd rather guarantee that mOriginSuffix is non-null, which probably means inverting the computation order in this function and removing the null-out.

Comment 77

[Bobby Holley (:bholley)]

Comment on attachment 8843481 [details] [diff] [review]
Part 7: Add a fast path for nsIPrincipal::Equals() and nsIPrincipal::EqualsConsideringDomain()

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

::: caps/BasePrincipal.cpp
@@ +345,5 @@
>    return SubsumesInternal(aOther, aConsideration);
>  }
>  
> +bool
> +BasePrincipal::EqualsInternal(nsIPrincipal* aOther)

Let's call this FastEquals and make it public. File a followup to add corresponding Fast{Equals,Subsumes}{,ConsideringDomain} methods, and use them from WrapperFactory::Rewrap to avoid the virtual call?

@@ +362,5 @@
> +  if (Kind() == eNullPrincipal || Kind() == eSystemPrincipal) {
> +    return this == other;
> +  }
> +
> +  if (mOriginNoSuffix && other->mOriginNoSuffix) {

You can skip the |&& other->mOriginNoSuffix| here.

@@ +430,5 @@
> +  if (Kind() == eNullPrincipal && other->Kind() == eNullPrincipal) {
> +    *aResult = (this == other);
> +    return NS_OK;
> +  }
> +  if (mOriginNoSuffix && other->mOriginNoSuffix && EqualsInternal(aOther)) {

Like above, you can remove the |&& other->mOriginNoSuffix|.

Comment 78

[Bobby Holley (:bholley)]

Comment on attachment 8843482 [details] [diff] [review]
Part 8: Add a fast path for nsIPrincipal::EqualsConsideringDomain() and nsIPrincipal::SubsumesConsideringDomain()

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

::: caps/BasePrincipal.cpp
@@ +424,5 @@
>           Cast(aOther)->SubsumesInternal(this, DontConsiderDocumentDomain);
>  }
>  
> +bool
> +BasePrincipal::SubsumesInternal(nsIPrincipal* aOther)

As mentioned elsewhere, let's call this FastSubsumes. Especially because SubsumesInternal is already the name of the virtual hook (and you only avoid collisions because the virtual hook is 2-arg), which makes the state of the world with this patch super confusing.

Comment 79

[Bobby Holley (:bholley)]

Comment on attachment 8843483 [details] [diff] [review]
Part 9: Speed up the OriginAttributes comparison in BasePrincipal::Subsumes()

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

::: caps/BasePrincipal.cpp
@@ +338,5 @@
>    // sub-principals individually, null principals do only simple checks for
>    // pointer equality, and system principals are immune to origin attributes
>    // checks, so only do this check for codebase principals.
>    if (Kind() == eCodebasePrincipal &&
> +      mOriginSuffix != Cast(aOther)->mOriginSuffix) {

Please MOZ_ASSERT above that mOriginSuffix is non-null.

Comment 80

[(no longer active)]

Unfortunately there still seem to be quite a few remaining leaks on try which will prevent me from landing this any time soon.  I spent a few hours today debugging only one of them to no avail.  I'll file them all, making them block this bug.  :(

Comment 81

[Bobby Holley (:bholley)]

I don't understand. The only thing that this patch has to do with leaks is that principals now hold onto an additional atom or two, and those atoms are being leaked along with the (already-leaked) principals, right? Is the issue just that we're tripping the asserts at [1]?

If so, that certainly shouldn't block landing this bug. We just need to update expectations somehow. We could add a Cu API to set an atom leak threshold from those tests, and then modify the assertion to take that threshold into account.

These patches are very useful, and we shouldn't let them rot over this.

[1] http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/xpcom/ds/nsAtomTable.cpp#427

Comment 82

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #81)
> I don't understand. The only thing that this patch has to do with leaks is
> that principals now hold onto an additional atom or two, and those atoms are
> being leaked along with the (already-leaked) principals, right? Is the issue
> just that we're tripping the asserts at [1]?

Yes, see comment 23.  What happens in each one of these bugs is we crash during shutdown due to the "[System Principal]" dynamic atom still being alive, which causes assertions: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee7fd44ea01bd989c7f2248011c227f1c7dce8ba>

> If so, that certainly shouldn't block landing this bug. We just need to
> update expectations somehow. We could add a Cu API to set an atom leak
> threshold from those tests, and then modify the assertion to take that
> threshold into account.

Well, it's a bit more complicated than that.

First things first, the root cause of this all is that we never had any leak checking in xpcshell tests (bug 1341961).  So, things leak there.  And in general there is no protection against leaks elsewhere (for example, bug 1343863 happens during mach package just due to the fact that we run the xpcshell binary there.)

The failure in question isn't about leaks in the usual way, but about this assertion: <http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/xpcom/ds/nsAtomTable.cpp#409>.  IOW, when we shut down XPCOM, all dynamic atoms must be dead.

There are various workarounds for this issue that could unblock me to be able to land my patches here:

1) Attachment 8839724 [details] [diff] which directly addresses the leak in question, but sucks in all sorts of obvious ways.
2) I could add a static atom for "[System Principal]" and use it similar to attachment 8839782 [details] [diff] [review].  This bypasses the assertion by just ensuring that the only principal we currently stumble over is a static one.  I have already added a workaround in bug 1342560 so this won't be the worst thing but obviously the first person who adds a leak involving any other principal than the system principal will be unable to land their code.
3) Convince froydnj to relax this assertion (which he was strongly against when I asked on IRC early on when I hit it)

What do you prefer?

> These patches are very useful, and we shouldn't let them rot over this.

Yeah.  I'm especially worries since I don't know if I have any more ideas that I didn't try already for bug 1344584...  And even if that is fixed there are a few more unfixed ones left too...

Comment 83

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #82)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #81)
> > I don't understand. The only thing that this patch has to do with leaks is
> > that principals now hold onto an additional atom or two, and those atoms are
> > being leaked along with the (already-leaked) principals, right? Is the issue
> > just that we're tripping the asserts at [1]?
> 
> Yes, see comment 23.  What happens in each one of these bugs is we crash
> during shutdown due to the "[System Principal]" dynamic atom still being
> alive, which causes assertions:
> <https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ee7fd44ea01bd989c7f2248011c227f1c7dce8ba>
> 
> > If so, that certainly shouldn't block landing this bug. We just need to
> > update expectations somehow. We could add a Cu API to set an atom leak
> > threshold from those tests, and then modify the assertion to take that
> > threshold into account.
> 
> Well, it's a bit more complicated than that.
> 
> First things first, the root cause of this all is that we never had any leak
> checking in xpcshell tests (bug 1341961).  So, things leak there.  And in
> general there is no protection against leaks elsewhere (for example, bug
> 1343863 happens during mach package just due to the fact that we run the
> xpcshell binary there.)
> 
> The failure in question isn't about leaks in the usual way, but about this
> assertion:
> <http://searchfox.org/mozilla-central/rev/
> e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/xpcom/ds/nsAtomTable.cpp#409>. 
> IOW, when we shut down XPCOM, all dynamic atoms must be dead.
> 
> There are various workarounds for this issue that could unblock me to be
> able to land my patches here:
> 
> 1) Attachment 8839724 [details] [diff] which directly addresses the leak in
> question, but sucks in all sorts of obvious ways.

Yeah.

> 2) I could add a static atom for "[System Principal]" and use it similar to
> attachment 8839782 [details] [diff] [review].  This bypasses the assertion
> by just ensuring that the only principal we currently stumble over is a
> static one.

Oh hey, that's a great idea! Let's do that.

>  I have already added a workaround in bug 1342560

Hm, I don't understand why that patch was necessary, given that the empty string is already a static atom. If the issue is that we do certain principal things before initializing the static atoms, we should just move atom initialization earlier in startup. It's entirely under our own control.

 so this won't
> be the worst thing but obviously the first person who adds a leak involving
> any other principal than the system principal will be unable to land their
> code.

Well sure, but they can always use the additional fine-grained mechanism I proposed in comment 81.

> 3) Convince froydnj to relax this assertion (which he was strongly against
> when I asked on IRC early on when I hit it)
> 
> What do you prefer?

Option 2, definitely.

> 
> > These patches are very useful, and we shouldn't let them rot over this.
> 
> Yeah.  I'm especially worries since I don't know if I have any more ideas
> that I didn't try already for bug 1344584...  And even if that is fixed
> there are a few more unfixed ones left too...

Comment 84

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #82)
> but obviously the first person who adds a leak involving any other principal
> than the system principal will be unable to land their code.

That doesn't sound like a bad thing...

Comment 85

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Yeah, it is pretty bogus to have leak checking assertions where we don't care about leaks. It looks like this static atoms code tries to finesse this by only running for a shutdown GC, but that is clearly insufficient, given the XPCShell case. Maybe the atoms code could only do the leak check if XPCOM_MEM_BLOAT_LOG is set in the environment? I don't know if that is set in XPCShell or not, but looking at where that is set it might not be.

Comment 86

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #85)
> Yeah, it is pretty bogus to have leak checking assertions where we don't
> care about leaks. It looks like this static atoms code tries to finesse this
> by only running for a shutdown GC, but that is clearly insufficient, given
> the XPCShell case. Maybe the atoms code could only do the leak check if
> XPCOM_MEM_BLOAT_LOG is set in the environment? I don't know if that is set
> in XPCShell or not, but looking at where that is set it might not be.

Is XPCOM_MEM_BLOAT_LOG defined for normal debug builds that developers run outside of test harnesses? If so, this seems like a nice solution to me.

Comment 87

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #86)
> Is XPCOM_MEM_BLOAT_LOG defined for normal debug builds that developers run
> outside of test harnesses? If so, this seems like a nice solution to me.

I think it isn't, but if it isn't, we're not detecting regular leaks either. There's no reason why atom leaks are more important than, say, nsGlobalWindow leaks.

Comment 88

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #85)
> Yeah, it is pretty bogus to have leak checking assertions where we don't
> care about leaks. It looks like this static atoms code tries to finesse this
> by only running for a shutdown GC, but that is clearly insufficient, given
> the XPCShell case.

Do we really not care about leaks, though? I'd certainly like to know if my
xpcshell tests are leaking in ways I don't expect. I think it's more an issue
that no-one has gone to the trouble of adding leak checking to xpcshell, and
greening up existing tests. Basic shutdown sanity checks still seem
appropriate, though.

> Maybe the atoms code could only do the leak check if XPCOM_MEM_BLOAT_LOG is
> set in the environment? I don't know if that is set in XPCShell or not, but
> looking at where that is set it might not be.

It shouldn't be. I'm pretty sure it's normally only set by the mochitest
harness.

Comment 89

[Andrew McCreight [:mccr8] (PTO until 12-2)]

If you want to discuss enabling leak checks in xpcshell, please discuss that in bug 1341961. Until that is fixed, we do not check leaks in xpcshell builds, and so we should not produce an error for leaks in those builds.

Comment 90

[Kris Maglione [:kmag]]

We don't do the same kind of harness-level leak checking that we do in mochitests, but we still do the normal shutdown finalization that we do in other debug builds, and we still check that, e.g., there are no JS objects alive at compartment destruction and runtime shutdown.

This isn't a question of adding more leak checking, it's about whether we want to remove existing leak checking. Working around the existing system principal leaks to get this landed makes sense to me, but making it easier to land future leaks is a different matter.

Comment 91

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #90)
> We don't do the same kind of harness-level leak checking that we do in
> mochitests, but we still do the normal shutdown finalization that we do in
> other debug builds, and we still check that, e.g., there are no JS objects
> alive at compartment destruction and runtime shutdown.

Note that XPCShell uses a different shutdown path than the normal browser.  There is certainly nothing that currently prevents an XPCShell test from dying with JS objects and the like being alive (as evidenced by, let's say, bug 1344588 where a JS object, among other things, is leaked.)  That is to say, NS_LogTerm() is called there, it just doesn't do anything.

> This isn't a question of adding more leak checking, it's about whether we
> want to remove existing leak checking. Working around the existing system
> principal leaks to get this landed makes sense to me, but making it easier
> to land future leaks is a different matter.

Let's be clear what we are talking about here.  Right now you cannot leak *dynamic* atoms in xpcshell tests (you can leak static atoms just fine, as my try push demonstrates).  Leaking anything else whatsoever is fair game.  The proposal at hand is to make bug 1341961 be where we prohibit leaking all objects, dynamic atoms included.

Comment 92

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #91)
> Note that XPCShell uses a different shutdown path than the normal browser.
> There is certainly nothing that currently prevents an XPCShell test from
> dying with JS objects and the like being alive (as evidenced by, let's say,
> bug 1344588 where a JS object, among other things, is leaked.)  That is to
> say, NS_LogTerm() is called there, it just doesn't do anything.

I was actually talking about a different set of shutdown leak checks in the JS
GC code:

http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/js/src/jsgc.cpp#3578-3581

I'd expect those to also run in xpcshell, but it looks like they don't because
xpcshell creates its own JSContext and keeps it alive until after the last
GC.

> Let's be clear what we are talking about here.  Right now you cannot leak
> *dynamic* atoms in xpcshell tests (you can leak static atoms just fine, as
> my try push demonstrates).  Leaking anything else whatsoever is fair game.
> The proposal at hand is to make bug 1341961 be where we prohibit leaking all
> objects, dynamic atoms included.

Right. I guess my concern is mostly that if we do want to do full shutdown
leak checks for xpcshell tests, we shouldn't make it easier to add more leaks
in the mean time. And even if we don't, I still think we probably shouldn't
make it easier to add more leaks in the mean time...

Comment 93

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] from comment #92)
> I'd expect those to also run in xpcshell, but it looks like they don't
> because xpcshell creates its own JSContext and keeps it alive until after
> the last GC.

And, as it turns out, adding JS_DestroyContext(cx) after NS_ShutdownXPCOM in XPCShellImpl.cpp actually fixes a lot of the leaks that you filed.

Comment 94

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #93)
> (In reply to Kris Maglione [:kmag] from comment #92)
> > I'd expect those to also run in xpcshell, but it looks like they don't
> > because xpcshell creates its own JSContext and keeps it alive until after
> > the last GC.
> 
> And, as it turns out, adding JS_DestroyContext(cx) after NS_ShutdownXPCOM in
> XPCShellImpl.cpp actually fixes a lot of the leaks that you filed.

Ah interesting!  XPCShell differences bite again.  :(

(In reply to Kris Maglione [:kmag] from comment #92)
> > Let's be clear what we are talking about here.  Right now you cannot leak
> > *dynamic* atoms in xpcshell tests (you can leak static atoms just fine, as
> > my try push demonstrates).  Leaking anything else whatsoever is fair game.
> > The proposal at hand is to make bug 1341961 be where we prohibit leaking all
> > objects, dynamic atoms included.
> 
> Right. I guess my concern is mostly that if we do want to do full shutdown
> leak checks for xpcshell tests, we shouldn't make it easier to add more leaks
> in the mean time. And even if we don't, I still think we probably shouldn't
> make it easier to add more leaks in the mean time...

I agree with that in the abstract, but in practice we don't have that many dynamic atoms, so we aren't giving up that much.  What I do think we should do is actually get someone to work on fixing bug 1341961.  I'm hoping to get some traction there.  Besides the selfish reason of wanting to get this landed, I think the perf gain from this is worth the risk of dynamic atom leaks creeping in given how infrequently people use them.

Comment 95

[(no longer active)]

I think I'm good to land this once bug 1344848 gets merged to inbound.

Comment 96

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #95)
> I think I'm good to land this once bug 1344848 gets merged to inbound.

I think it's probably acceptable to just cherry-pick that commit and include it with your push, assuming it goes green on inbound.

Comment 97

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #96)
> (In reply to :Ehsan Akhgari from comment #95)
> > I think I'm good to land this once bug 1344848 gets merged to inbound.
> 
> I think it's probably acceptable to just cherry-pick that commit and include
> it with your push, assuming it goes green on inbound.

OK, fair!  These days a merge may take a day...

Comment 98

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7b8a9a8989
Part 1: Remove nsPrincipal::SetURI() since it's unused; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb0ea9136535
Part 2: De-virtualize BasePrincipal::Kind(); r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/405aa6574c00
Part 3: Add an assertion to ensure that codebase principals are never constructed with URI schemes such as javascript:, about: and data:; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/334ee7b3b302
Part 4: Fix nsIPrincipal::GetOrigin()'s handling of non-strict file:// URI origin policy; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/69abc17ea32f
Part 5: Make nsIPrincipal.origin throw for about:blank codebase URI principals; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/c798f52cc53e
Part 6: Store BasePrincipal::{mOriginNoSuffix,mOriginSuffix} as a pair of atoms; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/512a7e6dfe6b
Part 7: Add a fast path for nsIPrincipal::Equals() and nsIPrincipal::EqualsConsideringDomain(); r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bf97b0dd70
Part 8: Add a fast path for nsIPrincipal::EqualsConsideringDomain() and nsIPrincipal::SubsumesConsideringDomain(); r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cd9e592cc85
Part 9: Speed up the OriginAttributes comparison in BasePrincipal::Subsumes(); r=bholley

Comment 99

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd02b882459
Part 10 - Remove BasePrincipal::EqualsIgnoringAddonId which somehow crept back in during the last rebase

Comment 100

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c7b8a9a8989
https://hg.mozilla.org/mozilla-central/rev/cb0ea9136535
https://hg.mozilla.org/mozilla-central/rev/405aa6574c00
https://hg.mozilla.org/mozilla-central/rev/334ee7b3b302
https://hg.mozilla.org/mozilla-central/rev/69abc17ea32f
https://hg.mozilla.org/mozilla-central/rev/c798f52cc53e
https://hg.mozilla.org/mozilla-central/rev/512a7e6dfe6b
https://hg.mozilla.org/mozilla-central/rev/c9bf97b0dd70
https://hg.mozilla.org/mozilla-central/rev/8cd9e592cc85
https://hg.mozilla.org/mozilla-central/rev/5dd02b882459