1343389 - stylo: Implement nsIPrincipal::DefinitelyEquals and use it in nsCSSValueTokenStream::operator==

Status: RESOLVED DUPLICATE of bug 1347817

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

In bug 1340710, Ehsan is adding origin atoms to nsIPrincipals so that we can compare them cheaply 99% of the time without messing around with URIs.

This is good timing, because we're hitting some parallelism hazards in nsCSSValueTokenStream::operator== (and possibly elsewhere) from CalcStyleDifference, where we try to compare principals off-main-thread.

I'm pretty sure it should be fine to return false from operator== for a few weird principals that don't have a useful origin. So when Ehsan's work lands, we can add a method to nsIPrincipal that just compares the atoms and returns false when they aren't there.

Comment 1

[(no longer active)]

Are we 100% sure there are no cases that we should care about where a codebase principal won't have its atoms set up?  For example, about:home will get a principal with a simple URI which will fails in GetOriginInternal() in GetHostPort(), wouldn't it?  Perhaps we should just setup the atoms using the full spec in the case of simple URIs?

Comment 2

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #1)
> Are we 100% sure there are no cases that we should care about where a
> codebase principal won't have its atoms set up?  For example, about:home
> will get a principal with a simple URI which will fails in
> GetOriginInternal() in GetHostPort(), wouldn't it?

No, about:home works because about: URIs are whitelisted in GetOrigin.

> Perhaps we should just
> setup the atoms using the full spec in the case of simple URIs?

That would make me a bit nervous. I'd rather stick with the conservative thing to start, and then work on removing all the cases where GetOrigin fails. Sounds like baku has already done a bunch of that work.

Comment 3

[(no longer active)]

OK sounds good to me.  baku, can you please let me know what your plans are, since I'm changing stuff around this code as well?

Comment 4

[Andrea Marchesini [:baku]]

My plan is to land my code when I receive a positive review. I discussed that patch with smaug for more than 1 week, and finally it's back compatible and green on try. What about if you write your patches on top of mine?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini [:baku] from comment #4)
> My plan is to land my code when I receive a positive review. I discussed
> that patch with smaug for more than 1 week, and finally it's back compatible
> and green on try. What about if you write your patches on top of mine?

As I said in bug 1340710 comment 60 and over IRC, I want Ehsan's changes to land before the patches in bug 1340163, because Ehsan's patches are more conservative (i.e. they won't change observable behavior) and I would like the ability to back out bug 1340163 for addon-compat issues without losing the performance optimization.

Comment 6

[Andrea Marchesini [:baku]]

My patches fix a regression: bug 1340163. I think my patch should land asap. Plus also my patches do not introduce any observable changes.
I'll discuss this with smaug as well.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini [:baku] from comment #6)
> My patches fix a regression: bug 1340163. I think my patch should land asap.

I don't think the extra few days of delay this will add outweigh the benefits of landing Ehsan's patches first. That said, we should confirm with him that he's able to get them finished up quickly.

> Plus also my patches do not introduce any observable changes.

Your patches create a nullprincipal for non-well-behaved URIs, right? That sure sounds like a behavior change to me.

> I'll discuss this with smaug as well.

Sorry, but it's my module.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Avoid non-threadsafe principal comparisons in nsCSSValueTokenStream::operator==. v1

MozReview-Commit-ID: 7E2DcD3CCKD

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8844662 [details] [diff] [review]
Avoid non-threadsafe principal comparisons in nsCSSValueTokenStream::operator==. v1

Ehsan for the principal bits, heycam for the style bits.

Comment 10

[Cameron McCormack (:heycam)]

I guess nsCSSValueTokenStream::operator== is called from nsCSSValue::operator==.  Where is nsCSSValue::operator== called from during restyling?  I'm wary of making operator== itself mean "definitely equals if we returned true, may or may not be equal if we returned false", and would rather a separate method to mean that (like we have DefinitelyEqualsURIs on URLValueData).  If it's not too invasive, can we have a DefinitelyEquals method on nsCSSValue and nsCSSValueTokenStream, so that it is clearer what the behaviour is?

I think we'll need similar fixes for URLValueData, where nsCSSValue::operator== is calling in to its Equals function, which is not safe to call OMT.

Comment 11

[Bobby Holley (:bholley)]

Hm. It might also be worth just waiting for baku's patches in bug 1340163, which means that we'll always be able to use the inline path to compare principals. That will make this problem go away.

Comment 12

[Cameron McCormack (:heycam)]

We still should have a DefinitelyEquals on nsCSSValue to handle URLValueData comparisons, though, so I'd say it's probably worth having that method there.  Unless there is a plan for that to work soon OMT too.

Comment 13

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> Hm. It might also be worth just waiting for baku's patches in bug 1340163,
> which means that we'll always be able to use the inline path to compare
> principals. That will make this problem go away.

Yeah that sounds like a better idea to me.

Comment 14

[Bobby Holley (:bholley)]

The race here will just go away when bug 1340163 lands.