1220346 - Move referrer-setting into necko

Status: NEW

(Core :: Networking: HTTP, defect, P3)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Now that channel creation usually has access to the loading document or principal, we can use that information to set the referrer.

We will still need to let individual callsites override that default referrer, but more often than not some default logic should work fine.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdadc19f4d7a

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=235321bfb3f3

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aa5ec1c729a

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3d9add3648f

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c64e5de432c

Comment 6

[Tanvi Vyas[:tanvi]]

Hi Jonas, Does this replace the referrer with triggering Principal?  Or is this just about referrer policy?

Is this the patch I should be looking at https://hg.mozilla.org/try/rev/6c64e5de432c ?

Thanks!

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, this grabs the referrer from the triggering principal. And the referrer-policy from the loadingNode.

It's still too early to look at patches though as they are still failing a number of tests. But yes, the one you linked to is the latest version.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=023b97e0c30e

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d991c0ec32a

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 1: Automatically set a default referrer when creating http channels. Use default policy in XHR, fetch() and plugins. r=ckerschb

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 2: Use the default referrer in EventSource, sendBeacon, <object>, HTMLMediaElement, and XSLT

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 3: Use the default referrer in <a ping>

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 4: Use the default referrer in <script>

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 5: Use the default referrer in XSLT and nsSyncLoadService

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8690662 [details] [diff] [review]
Part 1: Automatically set a default referrer when creating http channels. Use default policy in XHR, fetch() and plugins. r=ckerschb

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

In general I am quite happy with the approach of centralizing the referrerPolicy within CSP. One downside (not even sure if that is really a downside) is that CSP functionality of ::toString() or also ::toJSON() will now potentially include a referrer policy that might not have been set through meta CSP but rather through meta referrer. Anyway, I think we can live with that.

The thing I don't fully understand is, why are you having these 'else' branches and explicitly set the referrer to a nullptr (e.g. in SendPing). Wouldn't it make more sense to initialize mReferrer to a nullptr in the constructor of the httpbaseChannel?

I think the renaming of the referrerpolicy-patch landed in the meantime. Can you please rebase the patches and I'll have those patches reviewed in no time.

::: ipc/glue/BackgroundUtils.cpp
@@ +228,5 @@
> +    triggeringPrincipalInfo = info;
> +  }
> +  else {
> +    triggeringPrincipalInfo = void_t();
> +  }

What is the advantage of using OptionalPrincipalInfo? The triggeringPrincipal will always be set to the loadingPrincipal in the LoadInfo constructor and hence will never be null, no?

::: netwerk/base/LoadInfo.cpp
@@ +536,5 @@
>  }
>  
> +already_AddRefed<nsIURI>
> +LoadInfo::GetReferrerAndPolicy(ReferrerPolicy* aPolicy)
> +{

As discussed in person, it might make more sense to have that functionality exposed as a static function on the ioservice (or also on the contentSecurityManager).

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8690663 [details] [diff] [review]
Part 2: Use the default referrer in EventSource, sendBeacon, <object>, HTMLMediaElement, and XSLT

As discussed over IRC, clearing those flags for now.

Comment 17

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 18

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.