1360973 - Don't do the work needed for same origin referrer policies unless if we have one set

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[(no longer active)]

CheckSameOriginURI() can be expensive and shows up under profiles, so we should
try to avoid calling it unless if we have the same origin referrer policy.

Comment 1

[(no longer active)]

Attachment: t push -f my work# Attachment to Bug 1360973 - Don't do the work needed for same origin referrer policies unless if we have one set

Don't do the work needed for same origin referrer policies unless if we have one set

Comment 2

[(no longer active)]

Attachment: git show -w

Comment 3

[(no longer active)]

Comment on attachment 8863296 [details] [diff] [review]
t push -f my work# Attachment to Bug 1360973 - Don't do the work needed for same origin referrer policies unless if we have one set

Sorry this isn't quite right, I was building in the wrong ssh window :-(

I will fix up the patch tomorrow.

Comment 4

[Patrick McManus [:mcmanus]]

I'll delegate the review to :tnguyen when you get there..

Comment 5

[(no longer active)]

Attachment: Don't do the work needed for same origin referrer policies unless if we need to

CheckSameOriginURI() can be expensive and shows up under profiles, so we
should try to avoid calling it unless if we are going to use
isCrossOrigin.

Comment 6

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e538721dc72606690472fa09ed88a8a025f65fcf&selectedJob=95650690

Comment 7

[(no longer active)]

Attachment: git show -w

Comment 8

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8863440 [details] [diff] [review]
Don't do the work needed for same origin referrer policies unless if we need to

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

Thanks Ehsan

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1650,5 @@
> +  Maybe<bool> isCrossOrigin;
> +  if (mReferrerPolicy == REFERRER_POLICY_SAME_ORIGIN ||
> +      mReferrerPolicy == REFERRER_POLICY_ORIGIN_WHEN_XORIGIN ||
> +      mReferrerPolicy == REFERRER_POLICY_STRICT_ORIGIN_WHEN_XORIGIN ||
> +      gHttpHandler->ReferrerXOriginTrimmingPolicy() != 0) {

Should we check userReferrerTrimmingPolicy != 2?
Because 2 is the strictest policy (origin) and we may not have to compute isCrossOrigin

@@ +1744,5 @@
>    rv = clone->SetUserPass(EmptyCString());
>    if (NS_FAILED(rv)) return rv;
>  
>    nsAutoCString spec;
>  

I think we should move the computing part to here, because other condition checks may bail out early.

@@ +1763,5 @@
>    // "Strict" request from https->http case was bailed out, so here:
>    // "strict-origin" behaves the same as "origin".
>    // "strict-origin-when-cross-origin" behaves the same as "origin-when-cross-origin"
>    if (mReferrerPolicy == REFERRER_POLICY_ORIGIN ||
>        mReferrerPolicy == REFERRER_POLICY_STRICT_ORIGIN ||

These 2 values we also don't have to compute isCrossOrigin. That means if we had the strictest policy userReferrerTrimmingPolicy == 2, we may not have to compute isCrossOrigin

Comment 9

[(no longer active)]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #8)
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +1650,5 @@
> > +  Maybe<bool> isCrossOrigin;
> > +  if (mReferrerPolicy == REFERRER_POLICY_SAME_ORIGIN ||
> > +      mReferrerPolicy == REFERRER_POLICY_ORIGIN_WHEN_XORIGIN ||
> > +      mReferrerPolicy == REFERRER_POLICY_STRICT_ORIGIN_WHEN_XORIGIN ||
> > +      gHttpHandler->ReferrerXOriginTrimmingPolicy() != 0) {
> 
> Should we check userReferrerTrimmingPolicy != 2?
> Because 2 is the strictest policy (origin) and we may not have to compute
> isCrossOrigin

I checked against 0 because of userReferrerXOriginTrimmingPolicy being passed to std::max here.  IOW if userReferrerXOriginTrimmingPolicy is 0, std::max(foo, 0) would be foo, so there would be no side effect to running this branch.  Does that make sense?

> @@ +1744,5 @@
> >    rv = clone->SetUserPass(EmptyCString());
> >    if (NS_FAILED(rv)) return rv;
> >  
> >    nsAutoCString spec;
> >  
> 
> I think we should move the computing part to here, because other condition
> checks may bail out early.

Sure.  I guess technically that changes the behavior if some of the reordered code fails and returns an error code but hopefully nothing will depend on that.

I'll attach another version of my patch although I suspect it will need to change based on the rest of this comment, but that's OK.  :-)

> @@ +1763,5 @@
> >    // "Strict" request from https->http case was bailed out, so here:
> >    // "strict-origin" behaves the same as "origin".
> >    // "strict-origin-when-cross-origin" behaves the same as "origin-when-cross-origin"
> >    if (mReferrerPolicy == REFERRER_POLICY_ORIGIN ||
> >        mReferrerPolicy == REFERRER_POLICY_STRICT_ORIGIN ||
> 
> These 2 values we also don't have to compute isCrossOrigin. That means if we
> had the strictest policy userReferrerTrimmingPolicy == 2, we may not have to
> compute isCrossOrigin

Are you sure?  If we don't check isCrossOrigin for these two values, then if content specifies "origin-when-cross-origin" it will effectively be treated as "origin", and if content specifies "strict-origin-when-cross-origin" it will effectively be treated as "strict-origin".  That seems wrong to me...  What am I missing?

Comment 10

[(no longer active)]

Attachment: Don't do the work needed for same origin referrer policies unless if we need to

CheckSameOriginURI() can be expensive and shows up under profiles, so we
should try to avoid calling it unless if we are going to use
isCrossOrigin.

Comment 11

[Thomas Nguyen (:tnguyen)]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #9)
> (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #8)
> 
> Are you sure?  If we don't check isCrossOrigin for these two values, then if
> content specifies "origin-when-cross-origin" it will effectively be treated
> as "origin", and if content specifies "strict-origin-when-cross-origin" it
> will effectively be treated as "strict-origin".  That seems wrong to me... 
> What am I missing?
Sorry for confusing you. I meant REFERRER_POLICY_ORIGIN and REFERRER_POLICY_STRICT_ORIGIN, we do not have to compute isCrossOrigin though CheckSameOriginURI() (even gHttpHandler->ReferrerXOriginTrimmingPolicy() != 0). Because we expect to send origin no matter the site is cross origin.

Comment 12

[Thomas Nguyen (:tnguyen)]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #9)
> (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #8)
> > ::: netwerk/protocol/http/HttpBaseChannel.cpp
> > @@ +1650,5 @@
> > > +  Maybe<bool> isCrossOrigin;
> > > +  if (mReferrerPolicy == REFERRER_POLICY_SAME_ORIGIN ||
> > > +      mReferrerPolicy == REFERRER_POLICY_ORIGIN_WHEN_XORIGIN ||
> > > +      mReferrerPolicy == REFERRER_POLICY_STRICT_ORIGIN_WHEN_XORIGIN ||
> > > +      gHttpHandler->ReferrerXOriginTrimmingPolicy() != 0) {
> > 
> > Should we check userReferrerTrimmingPolicy != 2?
> > Because 2 is the strictest policy (origin) and we may not have to compute
> > isCrossOrigin
> 
> I checked against 0 because of userReferrerXOriginTrimmingPolicy being
> passed to std::max here.  IOW if userReferrerXOriginTrimmingPolicy is 0,
> std::max(foo, 0) would be foo, so there would be no side effect to running
> this branch.  Does that make sense?
The same here, I meant userReferrerTrimmingPolicy
We have userReferrerXOriginTrimmingPolicy = max(userReferrerTrimmingPolicy, userReferrerXOriginTrimmingPolicy)
So if userReferrerTrimmingPolicy == 2, then max(2, userReferrerXOriginTrimmingPolicy) == 2 no matter if cross origin

Comment 13

[Thomas Nguyen (:tnguyen)]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #12)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #9)
> > I checked against 0 because of userReferrerXOriginTrimmingPolicy being
> > passed to std::max here.  IOW if userReferrerXOriginTrimmingPolicy is 0,
> > std::max(foo, 0) would be foo, so there would be no side effect to running
> > this branch.  Does that make sense?
> The same here, I meant userReferrerTrimmingPolicy
> We have userReferrerXOriginTrimmingPolicy = max(userReferrerTrimmingPolicy,
> userReferrerXOriginTrimmingPolicy)
> So if userReferrerTrimmingPolicy == 2, then max(2,
> userReferrerXOriginTrimmingPolicy) == 2 no matter if cross origin
Typo
    userReferrerTrimmingPolicy =
      std::max(userReferrerTrimmingPolicy, userReferrerXOriginTrimmingPolicy);

Comment 14

[(no longer active)]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #11)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #9)
> > (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #8)
> > 
> > Are you sure?  If we don't check isCrossOrigin for these two values, then if
> > content specifies "origin-when-cross-origin" it will effectively be treated
> > as "origin", and if content specifies "strict-origin-when-cross-origin" it
> > will effectively be treated as "strict-origin".  That seems wrong to me... 
> > What am I missing?
> Sorry for confusing you. I meant REFERRER_POLICY_ORIGIN and
> REFERRER_POLICY_STRICT_ORIGIN, we do not have to compute isCrossOrigin
> though CheckSameOriginURI() (even
> gHttpHandler->ReferrerXOriginTrimmingPolicy() != 0). Because we expect to
> send origin no matter the site is cross origin. 

Oh, great point, I understand now.  Thanks!

Comment 15

[(no longer active)]

Attachment: Don't do the work needed for same origin referrer policies unless if we need to

CheckSameOriginURI() can be expensive and shows up under profiles, so we
should try to avoid calling it unless if we are going to use
isCrossOrigin.

Comment 16

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8864392 [details] [diff] [review]
Don't do the work needed for same origin referrer policies unless if we need to

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

Lgtm with the bellow comment

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1720,5 @@
> +        mReferrerPolicy != REFERRER_POLICY_STRICT_ORIGIN)) &&
> +      // 2 (origin-only) is already the strictest policy which we'd adopt if we
> +      // were cross-origin, so there is no point to compute whether we are or
> +      // not.
> +      gHttpHandler->ReferrerXOriginTrimmingPolicy() != 2) {

And plz check && gHttpHandler->ReferrerTrimmingPolicy() != 2 (because ReferrerTrimmingPolicy is applied in both cases cross/same origin. If we have the strictest ReferrerTrimmingPolicy, we also don't have to compute)

Comment 17

[(no longer active)]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #16)
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +1720,5 @@
> > +        mReferrerPolicy != REFERRER_POLICY_STRICT_ORIGIN)) &&
> > +      // 2 (origin-only) is already the strictest policy which we'd adopt if we
> > +      // were cross-origin, so there is no point to compute whether we are or
> > +      // not.
> > +      gHttpHandler->ReferrerXOriginTrimmingPolicy() != 2) {
> 
> And plz check && gHttpHandler->ReferrerTrimmingPolicy() != 2 (because
> ReferrerTrimmingPolicy is applied in both cases cross/same origin. If we
> have the strictest ReferrerTrimmingPolicy, we also don't have to compute)

D'oh, I think I should do this instead, and not additionally in fact, since the cross origin trimming policy doesn't matter here, it is the non-cross-origin policy that overrides the cross-origin policy.  Thankfully a test failure on the try server caught this mistake.  :-)  Thanks for spotting it!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e07dcfd3c4a5
Don't do the work needed for same origin referrer policies unless if we need to; r=tnguyen

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e07dcfd3c4a5