1113431 - (CVE-2015-2711) <meta name="referrer"> is ignored for navigations from the context menu and via a middle-click

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alex Verstak]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.38 Safari/537.36

Steps to reproduce:

1. Open https://scholar.google.com/scholar?q=news&mref=origin .
Middle-click on "Creating reality: How TV news distorts events" from ERIC to open the link in a new tab.
On the ERIC page, open the console and type document.referrer to see HTTP Referer.

2. Repeat 1, but left-click on the ERIC page to open it in the same tab, not a new tab.

3. Repeat 1, but from plain HTTP - http://scholar.google.com/scholar?q=news&mref=origin .


Actual results:

1. document.referrer is empty - incorrect, HTTPS -> HTTP in a new tab ignores meta referrer.
2. document.referrer = "https://scholar.google.com" - correct, HTTPS -> HTTP in the same tab respects meta referrer.
3. document.referrer = "http://scholar.google.com" - correct, HTTP -> HTTP in a new tab respects meta referrer.


Expected results:

1-2. document.referrer = "https://scholar.google.com".
3. document.referrer = "http://scholar.google.com".

Basically, it appears that the meta referrer implementation in Bug 704320 is incomplete.  When the source page is HTTPS, following a link to an HTTP page in the same tab respects meta referrer, but opening a link in a new tab does not.  Both cases respect meta-referrer when the source page is plain HTTP.

(Ditto for mref=unsafe-url - this is respected from HTTPS in the same tab, but not in a new tab.)

Comment 1

[Alex Verstak]

(This is obviously on Aurora, not Chrome. :-)

User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:36.0) Gecko/20100101 Firefox/36.0
Built from https://hg.mozilla.org/releases/mozilla-aurora/rev/8408727d213e

Comment 2

[Boris Zbarsky [:bzbarsky]]

Presumably the front-end code handling the middle-click needs to set the referrer policy (based on the meta tag), not just the referrer.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Maybe here?  
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#446

Was this encountered with remote tabs (e10s) enabled?

Comment 4

[Alex Verstak]

It's the same with and without e10s.  Tried Aurora (doesn't support e10s) and Nightly (e10s enabled and e10s disabled).  All three clear Referer in this case - opening a link from HTTPS to HTTP in a new tab, with meta-referrer set to "origin".

Comment 5

[Alex Verstak]

Two issues here:

1. UI code - _loadURIWithFlags() in browser.js - doesn't pass referrer policy to docshell, because it knows docshell as nsIWebNavigation whose loadURI() doesn't have a referrer policy parameter.  It's a direct call without e10s, and a sequence that leads to restoreTabContent() in ContentRestore.jsm with e10s; both call webNavigation.loadURI() at the end.

2. Links opened via the context menu get no referrer at all, because _loadURIWithFlags() is passed the following as referrer: (a) chrome://browser/content/browser.xul for "Open link in new tab", and (b) undefined for "Open link in new window".  This looks like a bigger issue; it strips Referer for websites that do nothing special - plain HTTP, no meta referrer.  (Stable and aurora pass Referer in this case; nightly doesn't.)

To address #1, we could do the following:

A. Add a function that passes referrer policy to nsIWebNavigation, e.g., loadURL2().

B. Add referrer policy to nsIDOMDocument.  Or rather make the one already in nsIDocument.h scriptable.

C. Propagate B to A - handleLinkClick() in browser.js gets the doc from event.target.ownerDocument, then calls openLinkIn() -> loadOneTab() -> addTab() -> _LoadURIWithFlags(), which eventually calls into nsIWebNavigation.  Likewise for the context menu - _openLinkInParameters() in nsContextMenu.js gets the doc and fills in the params to openLinkIn(), etc. (This would only propagate the referrer policy; fixing referrers in #2 is a separate issue.)

Thoughts?  Better alternatives?

Comment 6

[Matthew N. [:MattN]]

[Tracking Requested - why for this release]: We're shipping a feature (<meta name="referrer">) partially broken.

Gavin, do you have opinions on comment 5? This is going out the door in 36 in a broken state :(

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Alex Verstak from comment #5)
> 2. Links opened via the context menu get no referrer at all, because
> _loadURIWithFlags() is passed the following as referrer: (a)
> chrome://browser/content/browser.xul for "Open link in new tab", and (b)
> undefined for "Open link in new window".  This looks like a bigger issue; it
> strips Referer for websites that do nothing special - plain HTTP, no meta
> referrer.  (Stable and aurora pass Referer in this case; nightly doesn't.)

This is bug 1118502. Affects current DevEdition/Nightly, and we're already tracking it to ensure it is fixed before we hit release.

> To address #1, we could do the following:
> 
> A. Add a function that passes referrer policy to nsIWebNavigation, e.g.,
> loadURL2().
> 
> B. Add referrer policy to nsIDOMDocument.  Or rather make the one already in
> nsIDocument.h scriptable.
> 
> C. Propagate B to A - handleLinkClick() in browser.js gets the doc from
> event.target.ownerDocument, then calls openLinkIn() -> loadOneTab() ->
> addTab() -> _LoadURIWithFlags(), which eventually calls into
> nsIWebNavigation.  Likewise for the context menu - _openLinkInParameters()
> in nsContextMenu.js gets the doc and fills in the params to openLinkIn(),
> etc. (This would only propagate the referrer policy; fixing referrers in #2
> is a separate issue.)
> 
> Thoughts?  Better alternatives?

This sounds about right.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I don't have a great sense of the severity of this issue. It seems like something we can follow-up to fix in 37 despite shipping <meta name="referrer"> in 36, but maybe it would be preferred to delay shipping the feature entirely until this bug is fixed?

Comment 9

[Daniel Veditz [:dveditz]]

IMHO it's better to ship the feature and fix this later. It's an edge case that behaves as if the feature didn't exist so it's not any worse than before, and sites can start to benefit from the feature in the majority of normal use .

Comment 10

[Matthew N. [:MattN]]

(In reply to Daniel Veditz [:dveditz] from comment #9)
> It's an edge case

I personally don't think something as common as middle-clicking a link is an "edge case". I would guess that >10% of my navigations via links are in a new tab.

Comment 11

[Alex Verstak]

+1 for shipping meta-referrer in 36.  FWIW, it wouldn't be entirely without precedent - https://code.google.com/p/chromium/issues/detail?id=124750 :-)

Would be good to mention the caveats in the release notes - this bug, and also bug 1113438, which is already fixed in 37.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think it's fine to ship <meta name="referrer"> without this bug being fixed, as long as we plan to fix it at some point...

Note that this bug _does_ mean that we'll send referrers in some cases when the page asks not to, so it's not quite as low-priority as comment 0 makes it sound.  But it's certainly no worse than complete lack of <meta name="referrer"> support.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Seems that people are happy with shipping the feature with that bug. So, not tracking.
If you have a safe fix, this could land in beta 9 (go to build tomorrow).

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

[Tracking Requested - why for this release]:

(In reply to Boris Zbarsky [:bz] from comment #12)
> I think it's fine to ship <meta name="referrer"> without this bug being
> fixed, as long as we plan to fix it at some point...
> 
> Note that this bug _does_ mean that we'll send referrers in some cases when
> the page asks not to, so it's not quite as low-priority as comment 0 makes
> it sound.  But it's certainly no worse than complete lack of <meta
> name="referrer"> support.

Actually, it is *much* worse than complete lack of <meta name=referrer> support. Websites such as Facebook use feature detection to detect <meta name=referrer>, and when the feature detection indicates that it is supported, then they use direct links to third-party content, relying on meta referrer to strip sensitive information from the referrer URL. When the feature detection indicates the <meta name=referrer> is not supported, they use their own tricks (e.g. redirects) to ensure the sensitive information is stripped.

With this bug, their feature detection will detect <meta name=referrer> support, so these websites will not strip the sensitive information from the referrer URLs when linking to untrusted third-party content.

By the way, where was the intent to implement/ship thread for this feature? As far as I can tell, there wasn't one. So, I'll just point out here that the editor of the spec for this feature has already acknowledged that some of the issues I raised in the webappsec mailing list regarding this feature are significant and that major changes to the spec are likely, which is another reason it may be wise to hold back.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

OK. Seems that there is actually no consensus on this.

So, I see three potential solutions:
* Disable the feature
* Ship anyway
* Fix the bug (or workaround)

Gavin, what is your call on this?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Websites such as Facebook use feature detection

What do they detect, do you know?  We may be able to add support without enabling whatever it is they're detecting if that's what we want to do.

> By the way, where was the intent to implement/ship thread for this feature?

I'm not sure there was one...  You're right, there should have been.  Sid, what's the story with the spec issues?

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Boris Zbarsky [:bz] from comment #16)
> > By the way, where was the intent to implement/ship thread for this feature?
> 
> I'm not sure there was one...  You're right, there should have been.  Sid,
> what's the story with the spec issues?

Ack, yeah, sorry.  That slipped through my fingers.  Owen had been generously working on it for a very long time and when I picked it up to finish it I totally forgot to post the intent.

As Brian said, the spec is not complete or perfectly stable.  I wanted Firefox to provide a reference implementation to shake out any additional deployment issues with the spec sooner rather than later (which is why I pushed to get it done before the spec went to LC). Most of the issues are going to be deployment issues, and it's important to give web devs something to try out. There are, indeed, still things up for debate in the WebAppSec WG.  See for instance this thread:
http://lists.w3.org/Archives/Public/public-webappsec/2015Feb/0147.html

To Gavin: I think we should fix this bug, and _not_ disable the feature in the meantime.  We should be moving forward, not backwards, especially so we can find and fix more bugs like this sooner rather than later (and also shake out implementation problems in the spec).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Sid, giving web devs something to try out can be done by enabling the feature in Aurora only, no?

Past that, you didn't respond to Brian's point about feature detection.  I don't think we should be shipping the feature in its current state; we need to either make Facebook not detect it or something.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> Sid, giving web devs something to try out can be done by enabling the
> feature in Aurora only, no?

Sure, but performance improvements at scale is the real value of this feature and that's easiest to test once it's in released versions of browsers.  Devs can try it out in Aurora and extrapolate perf gains, but really what they want to know is what effect it will have on actual server load when their users' browsers can employ the feature.

In hindsight it may have been wise to hold in Aurora until the spec went to LC, but it's out now, shouldn't we really just fix the bug?  Regardless of how the spec changes, this bug will still be an implementation flaw and need to be fixed.

> Past that, you didn't respond to Brian's point about feature detection.  I
> don't think we should be shipping the feature in its current state; we need
> to either make Facebook not detect it or something.

I don't understand why this bug requires us to un-ship a feature.  This is not a bug in the spec, it is a bug in our implementation and we should fix it.  We should not blame my failure to catch this referrer-policy-application case on the spec.  If we fix this bug, Facebook's problem goes away, right?

While I totally agree with Brian that feature detection can be problematic, it is a problem with all optional security features; regardless of any spec state, if there's an implementation bug in a security feature that a web site doesn't know about and they rely on the feature, then it could cause security failures. Seems to me we just need to fix the bug (this bug is a flaw in our implementation).

Regarding spec churn: I believe Brian is referencing this message[0] from Mike.  From what I can tell from the WG discussion, the major change that will happen has to do with combining policies (i.e., changing the referrer policy after applying one).  Right now any attempt to combine policies shuts referrers off entirely.  Any change to this would be "more generous", referrer-wise.  Maybe I missed something else that is more concerning and would warrant turning off the feature in the meantime?

Mike, do you anticipate the spec will change substantially such that current implementations will be less secure or incompatible with the future revision of the spec?  Do you think sites' security would suffer if they start using the meta referrer we implemented in Firefox?

[0] http://lists.w3.org/Archives/Public/public-webappsec/2015Jan/0080.html

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I don't understand why this bug requires us to un-ship a feature

We haven't shipped this feature yet, unless I'm missing something.  I'm questioning whether we _should_ ship it in the current state, as opposed to holding it one more cycle or modifying it so it can't be detected for the moment.

> If we fix this bug, Facebook's problem goes away, right?

Yes, but how likely are we to fix this bug on beta 36?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

I'm all for "fixing this bug and moving forward", but:

a) someone needs to do that (can you take it, Sid?)
b) we can't realistically fix this bug in the 36 time frame, so unless we take other measures, we're going to end up shipping <meta name="referrer"> in Firefox 36 with this bug. That sounds acceptable to me, but it sounds like maybe we should reach out to known potential users, like Facebook, and warn them about it. Probably also worth a note in the release notes/MDN.

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> > I don't understand why this bug requires us to un-ship a feature
> 
> We haven't shipped this feature yet, unless I'm missing something.  I'm
> questioning whether we _should_ ship it in the current state, as opposed to
> holding it one more cycle or modifying it so it can't be detected for the
> moment.

/me facepalm... yes, sorry, you're right. I see more clearly now.

I don't think it's terrible to ship with this bug, and getting it out there in more browsers will cause more good than harm. From what I've heard out of Facebook and other implementers, much of the use-case for meta referrer is for subresources (for things like embedded games or third-party widgets), not clicked links.

So I'm with Gavin.  We  can fix it quickly and target 37 (especially if Alex is willing to help -- Alex: you've been tremendously helpful on the other referrer stuff).  And we should put a release note in 36 with this "known bug".  If it helps, I'm willing to do a mea culpa post on the security blog to say "oops, missed this thing, but don't worry we will fix it quickly".

If we want to do a last-minute tweak to 36 to make sure this thing doesn't get abused, we could (just for 36) prefix the feature to be like:
 <meta name="moz_referrer" instead of <meta name="referrer"
but I don't think that's necessary.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I'd still like to understand what sort of detection Facebook does.  Because say we ship with this relnoted.  What do we expect them to do?  UA-sniff the browser version?  That's generally something we try to avoid forcing people to do....

Comment 24

[Sylvestre Ledru [:Sylvestre]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #22)
> And we should put a release note in 36 with this "known bug".
We have this as a new feature for 36 [1]. Marking this bug as a know bug would give the impression that we are shipping broken feature. I am not sure it is what we want to do...

Worst case scenario: 37 is just 5 weeks after the 36 release... (and no Mea culpa to do)

[1] https://www.mozilla.org/en-US/mobile/36.0beta/releasenotes/

Comment 25

[Alex Verstak]

Sid - I should be able to help with this, but it'd probably take me several weeks to get to it, due to travel and schedule constraints.  If you want it done sooner, someone else should pick it up.  No rocket science here, just a bit of parameter propagation - see comment 7.

Shipping meta-referrer as-is in 36 (prefixed or unprefixed) would let me make Scholar HTTPS-only in Firefox, as it already is in Chrome and Safari.  I don't know how representative my use case is, but here's one concrete example of security improvements that this feature would enable here and now.

Re feature detection - the only way I can think of that would also work in Webkit is to load an iframe and check its document.referrer.  So, if compatibility is a major concern... let's not go there, it'd be saner to just ship it with a prefix. :-)

Comment 26

[Sid Stamm [:geekboy or :sstamm]]

Attachment: WIP

Thanks, Alex.

I ran out of time before getting around to testing or writing a test, but I had a thought; what if we take the NetUtil.applyReferrerPolicy implementation in bug 1073187, and then instead of propogating the referrer policy into nsIDOMDocument, just obtaining the referrer policy in browser.js and calculating the "effective referrer" there.

So taking the patch in attachment 8552570 [details] [diff] [review] on bug 1073187 (which we can probably land whenever), then applying this patch on top, I think we can:
* get the referrer and referrerPolicy from the doc
* compute the effective referrer
* stuff that (instead of the full referrer) into the params object (via handleLinkClick)
* use the effective/computed referrer when calling the various loaders in browser.js

It's basically what you said in comment 5, Alex, but instead we can compute and use the referrer higher in the stack instead of modifying nsIDOMDocument.

Does this look like a reasonable approach, Gavin?  I won't be able to follow through and finish this until next week (I'm taking off for 4 days of vacation in just over 24 hours) but I can at least try to get it started.

Comment 27

[Alex Verstak]

But then you'll probably need to also propagate the "don't mess with referrer" flag, which would defeat the point of doing it this way?  E.g., consider an HTTPS -> HTTP link with referrer=origin.  If you propagate referrer=https://just.host then wouldn't it get stripped at the end, because it's a downgrade?  That would be incorrect - referrer policy is supposed to override the defaults, which could make it more restrictive, or less restrictive.

Comment 28

[Alex Verstak]

But if you could pick some intermediate call site X and make sure that everyone upstream of X always passes it effectiveReferrer, then X could propagate (referrer=effectiveReferrer, referrerPolicy=unsafe-URL) downstream.  This might work, and would probably still look a lot cleaner than propagating referrerPolicy all over town.

The catch is that "everyone upstream" may be a fairly large set - {middle-click, context-menu, download, save-as, print} come to mind.  They should all be changed at some point, but it may not be a good idea to lump them all into the same patch.

Comment 29

[Mike West]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #19)
> Regarding spec churn: I believe Brian is referencing this message[0] from
> Mike.  From what I can tell from the WG discussion, the major change that
> will happen has to do with combining policies (i.e., changing the referrer
> policy after applying one).  Right now any attempt to combine policies shuts
> referrers off entirely.  Any change to this would be "more generous",
> referrer-wise.  Maybe I missed something else that is more concerning and
> would warrant turning off the feature in the meantime?

Brian (Hi, Brian!) is probably referencing his proposal at https://briansmith.org/referrer-01.html, and my response at https://lists.w3.org/Archives/Public/public-webappsec/2014Dec/0027.html. I've said similar things, possibly more strongly, in other more recent threads. I do think we need to make some wider changes to the referrer spec.

> Mike, do you anticipate the spec will change substantially such that current
> implementations will be less secure or incompatible with the future revision
> of the spec?  Do you think sites' security would suffer if they start using
> the meta referrer we implemented in Firefox?

I do anticipate changes to the spec. As I noted on the WebAppSec mailing list, I think Brian's general proposal is awesome, though I think it goes significantly further than is feasible in a number of areas. I do think we'll end up taking on several of his suggestions soonish (sorry, I've been distracted with CSP and MIX topics recently; I hope to get back to referrer in the near future).


In particular, splitting navigational referrers from subresource referrers is important, but I imagine that being implemented in a way that allows the current syntax to retain it's meaning (e.g. `<meta name="referrer" content="origin">` could be desugared into something like `<meta name="referrer" content="navigational-origin subresource-origin">`. My hope would be that current implementations would retain meaning in whatever new world we come up with.

However, I think his suggestion to drop `<meta>` entirely is unlikely to fly. Chrome and Safari have had `<meta>` support for years, and it's in wide use. I can't imagine that Chrome will change it's `<meta>` implementation in a backwards incompatible way in anything like a near-term timeframe.

Comment 30

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Alex Verstak from comment #27)
> But then you'll probably need to also propagate the "don't mess with
> referrer" flag, which would defeat the point of doing it this way?

Yeah, I see.  Thanks for the example.  To do this right, we probably need to propagate it all the way through to where the channel is created.

Unfortunately I'm hopping on a plane shortly and won't be online again until sometime on Tuesday.  If someone wants to pick this up and run with it, be my guest (or I can take a look next week).

Comment 31

[Sylvestre Ledru [:Sylvestre]]

So, looks like we are going to ship this new feature with this bug.

Jean-Yves, could you document this bug in MDN? (know issues).
(to reassign the ni to someone else)

Tracked for 37 to make sure we have a fix landing for this release.

Comment 32

[Boris Zbarsky [:bzbarsky]]

We have this tracked for 36 as well.  What will ensure that we fix for 37?

Do we have communications prepared for people who detect the feature about how they should detect the Firefox versions they can't rely on it in?

Is there a reason we're not just disabling the feature in 36 instead?

Comment 33

[Sylvestre Ledru [:Sylvestre]]

(In reply to Boris Zbarsky [:bz] from comment #32)
> We have this tracked for 36 as well.  What will ensure that we fix for 37?
We have been tracking it for only a week in 36. We have much more time for 37.

> Is there a reason we're not just disabling the feature in 36 instead?
I am following Gavin's call on this but I really don't have any issue to disable this feature for 37.

Can someone prepare a patch to disable the feature (in case we want to do it)? 36 RC build is today.

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Boris Zbarsky [:bz] from comment #16)
> > Websites such as Facebook use feature detection
> 
> What do they detect, do you know?  We may be able to add support without
> enabling whatever it is they're detecting if that's what we want to do.

How feature detection is typically done is mentioned in my proposal [1]. See [2] and [3].

There are serious problems with how meta referrer is specified, which are also pointed out in my proposal. In particular, the way mixed content is handled doesn't make sense at all.

I agree with Mike West that browsers that ship <meta referrer> will need to find some way to cope with existing web pages depending on it. They may struggle to do so, depending on how much the spec ends up changing. I'm hoping and expecting it will change *considerably*. If Mozilla wants to paint itself into the same corner that Google and Safari have painted themselves in, that's up to Mozilla. But, IMO, it would be better to actively participate in resolving the issues with the referrer draft spec and implement those fixes before activating the feature. Note that Mozilla just sent its first (AFAICT) contribution for improving the referrer control spec *last week* and that reviewing (and implementing?) a reasonable referrer policy seems to be a Q1 goal for the security team, according to [4]. IMO, it makes more sense to evaluate and decide upon a policy for referrer control *before* shipping a copy of Google's existing policy (which, it seems, will also change), instead of after.

[1] https://briansmith.org/referrer-01
[2] https://stackoverflow.com/questions/8370341/how-can-i-detect-rel-noreferrer-support/10020992
[3] https://www.facebook.com/notes/facebook-engineering/protecting-privacy-with-referrers/392382738919
[4] https://wiki.mozilla.org/SecurityEngineering/2015/Q1Goals

Comment 35

[Mike West]

Mozilla's decisions about the feature are, of course, Mozilla's to make. I'll note, however, that the `<meta>` implementation is widely used, and has solid performance and privacy benefits. I think it would be a shame if Firefox continued to force pages who require control over the `referer` header to push users through redirects to achieve it.

While I continue to agree with Brian that the Referrer Policy spec needs some work, I think we differ on the scope of that change, and in particular about whether or not it will continue to be backwards compatible with existing `<meta>` usage. Even if there was general agreement that `<meta>` should be deprecated, it's unlikely that we'll be removing it from Chrome in anything resembling near-term given that it's been in relatively wide use for ~3 years.

All that said, I'd echo Brian's call for comments on the spec. I do hope we'll make some real progress on it in the next few weeks, now that we're wrapping up CSP2 and MIX1.

Comment 36

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Mike West from comment #35)
> Mozilla's decisions about the feature are, of course, Mozilla's to make.
> I'll note, however, that the `<meta>` implementation is widely used, and has
> solid performance and privacy benefits. I think it would be a shame if
> Firefox continued to force pages who require control over the `referer`
> header to push users through redirects to achieve it.

I'm not advocating that Mozilla never implement referrer control. I'm advocating that Mozilla avoid shipping a referrer control mechanism in its next release where the implementation is significantly broken (the feature detection logic documented on stackoverflow and by Facebook gives misleading results that will lead some websites to do unsafe things) and where the specification has some significant flaws.

I don't think this is the place to debate how much the specification is likely to change. But, it is pretty clear that the specification is going to change significantly. I know that some people think my original proposal goes too far and I've already started a second revision based on Google's and Mozilla's feedback that addresses the concerns. Mozilla is also making proposals now.

I fail to see how the likelihood of it being painful for Chrome to remove or change its meta referrer implementation is an argument for rushing an implementation of the same into Firefox. Rather, I see it as an argument for making sure that the wrong thing doesn't ship in Firefox in the first place, to avoid that pain completely.

Comment 37

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Also, this type of scenerio is why Mozilla has this policy:
https://wiki.mozilla.org/WebAPI/ExposureGuidelines

I suggest that Mozilla stick to its own policies here. Then we could have this discussion in the Intent to Ship email thread.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> I am following Gavin's call on this

With all due respect, I don't think Gavin is the right person to make unilateral calls on web-facing features.

I agree with Brian: this needs to be disabled, at least for purposes of detection, on 36 for now, and we need to have an intent to ship.  I filed bug 1134606.

It looks like detection is done via use, so we need to actually disable entirely.

I will prepare a patch to do so, but this is NOT making me a happy camper in terms of people effectively owning their own stuff.

Comment 39

[Boris Zbarsky [:bzbarsky]]

One other thing: we also have referrer control via CSP in 37.  That presumably also has the issue described in this bug, though possibly not the spec-side issues...

Comment 40

[:Gavin Sharp [email: [e-mail]]]

I wasn't intending to make any unilateral calls :) Disabling the feature is fine by me.

Comment 41

[Sid Stamm [:geekboy or :sstamm]]

Yeah, disabling is the right thing for 36.  I'm working on a fix for this bug, but am strapped for time and it's turning out to be more complicated than I initially expected.  Would still appreciate Gavin's feedback on the WIP.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8563141 [details] [diff] [review]
WIP

>diff -r 12ca91298b43 browser/base/content/browser.js

>       case Ci.nsIBrowserDOMWindow.OPEN_NEWTAB :
>         let referrer = aOpener ? makeURI(aOpener.location.href) : null;
>+        if (referrer) {
>+          let referrerPolicy = doc.referrerPolicy;

"doc" is undefined here. I guess you can probably use aOpener.document. But is this code actually invoked for the STR from comment 0?

The approach in general seems fine.

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Attachment: broswer_referrer_tests.patch

Adding a WIP patch for the testcases because I do need some help figuring out what's going on.

Here is how the test works:
* I based the test on another test which I wrote a while back [1] but essentially performs the same task, we register a clickHandler, intercept the click and perform a call to gTestWin.contentAreaClick().
* In the other testcase [1], that strategy works fine.
* In the attached patch (in the clickHandler)
> aEvent.target = [object XULElement],
whereas (as the other test [1] shows), the
> aEvent.target = http://mochi.test:8888/browser/browser/base/content/test/general/file_meta_referrer_testserver.sjs
I don't know why and where that happens, but apparently this difference causes us to enter this branch [2] and hence not to perform the actual click.

Any help is highly appreciated!

@Gijs: I followed your suggestion, and tried to not register a clickHandler but instead 'faking' a eventObject and calling
> gTestWin.contentAreaClick(aEvent, true);
with that eventObject. Unfortunately I had to give up on that solution, because faking all the different objects within that event object became to cumbersome.

Thanks for any help - also, I am happy to rewrite the test if there is an easier solution to accomplish the same task, but at the moment I don't think there is. For all recommendations, please keep in mind that manipulatively we need tests for:
* Ctrl-click
* ctrl-drag and drop
* right-click open link in new tab
* middle click

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js#98
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5406

Comment 44

[:Gijs (out for now; he/him)]

You should waitForFocus(aTest, gTestWin.content) in the waitForSomeTabToLoad callback in setupTest. The other test does an executeSoon and waits for a load, *and* loads another tab manually before trying to click the link. Right now the simulated click seems to just focus the content, for whatever reason.

Comment 45

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sorry, this isn't an area of the code base I'm familiar with, and I don't think I can provide any useful information here :(

Comment 46

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #44)
> You should waitForFocus(aTest, gTestWin.content) in the waitForSomeTabToLoad
> callback in setupTest. The other test does an executeSoon and waits for a
> load, *and* loads another tab manually before trying to click the link.
> Right now the simulated click seems to just focus the content, for whatever
> reason.

Thanks Gijs - I wouldn't have figured myself. Surprises my that you can do a document.getElementById("") which returns the right thing but you can't perform the click on it. Thanks anyway, it works now!

Comment 47

[Alex Verstak]

Attachment: Propagate referrer policy throughout the UI.

Christoph - in case this helps you.  Unpolished and no automatic tests, but shows what/where to change.  Tested command-click and context menu's new {tab,window} by hand with meta-referrer={unsafe-URL,origin,origin-when-crossorigin}; all seem to work per current spec.

Comment 48

[Christoph Kerschbaumer [:ckerschb]]

Attachment: broswer_referrer_tests.patch

Ok - we have an automatic testcase, it's still WIP, but it works. Of course we can polish the testcode a little, but I think that's not high priority at the moment. Further, we potentially need to extend the tests anyway.

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Alex Verstak from comment #47)
> Created attachment 8568664 [details] [diff] [review]
> Propagate referrer policy throughout the UI.
> 
> Christoph - in case this helps you.  Unpolished and no automatic tests, but
> shows what/where to change.  Tested command-click and context menu's new
> {tab,window} by hand with
> meta-referrer={unsafe-URL,origin,origin-when-crossorigin}; all seem to work
> per current spec.

That's great - thanks for providing the patch - I'll take a look right away.

Comment 50

[Christoph Kerschbaumer [:ckerschb]]

Attachment: broswer_referrer_tests.patch

Alex, your code looks good, passes all the automatic tests - I need to fine grain them though, because at the moment I am only testing:
>   <meta name="referrer" content="origin">

Comment 51

[Christoph Kerschbaumer [:ckerschb]]

Attachment: broswer_referrer_tests.patch

@Alex: your patch seems to work really well - do you wanna finish it up since you already provided the whole patch? Or do you want me to take over? But I don't think there is every much to do anymore.

@Sid: Here are testcases, fully automatic, but they time out - please let me know which ones you want to keep. I don't think we need all of them on TBPL, but it's still nice to test offline to make sure everything works correctly. Whenever you get a chance, please take a look at the test - I am also happy to rewrite and simplify, just let me know.

Comment 52

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #51)
> @Sid: Here are testcases, fully automatic, but they time out - please let me
> know which ones you want to keep.

To solve the timeout, can you just split them into three test files (one for each case)?  I'd rather have more test coverage here than less, given the complexity of the feature.

Comment 53

[Alex Verstak]

@Christoph: it'd be best if you could take this over - I'm short on time.  A few things you might want to consider:

1. nsIWebNavigation::loadURIWithBase should probably be renamed to loadURIWithOptions and changed to take an options struct, similarly to nsIDocShell::loadURI.  A JS util function could convert the JS params dict to this struct, to make it easier to call.  (Changing nsIWebNavitation::loadURI seems like a non-starter - too many call sites; but changing loadURIWithBase sounds like a good idea - it's a subset, it's new, and it's only used in view-source.)

2. I only tested this with multi-process.  Not expecting any issues with single-process, but it certainly needs to be run through - single-process is using different browser & webNavigation objects.

3. The test should probably also cover origin-when-crossorigin.  Current tests cover propagation of the referrer policy, but not of the triggering principal.  It works, probably via some global state, but the test doesn't cover it.  A single same-origin case should be sufficient - if it uses the wrong triggering principal, it would truncate referrer to just origin.

(As an aside, if I were to pick just one test case for this bug, that would be HTTPS -> HTTP with meta-referrer=origin.  It would catch both failure to propagate the referrer policy, and also failure to trump the default policy.  My second choice would be origin-when-crossorigin, to test propagation of the triggering principal.  My third choice would be rel=noreferrer, to test that it trumps the meta-tag.  And then the cross-product of the default referrer policy cases - they're certainly most important, but also least likely to break.  As Sid says, the more the merrier, but if there's need to prioritize tests, those would be my picks.)

4. There's also save-as, view-source, and print.  And metro and maybe other forks (android?).  I'd file separate bugs for those - the scope of this one is already plenty large enough.

Comment 54

[Alex Verstak]

> 1. nsIWebNavigation::loadURIWithBase should probably be renamed to
> loadURIWithOptions and changed to take an options struct, similarly to
> nsIDocShell::loadURI.

Thinking more about it, renaming loadURIWithBase to loadURIWithOptions is probably a good idea, but passing a struct here seems like more boilerplate than it's worth.  Since all the non-trivial usage is from JS, it'd be simpler to write a JS wrapper that takes a params dict and calls loadURIWithOptions with the long complete list of parameters.

> 2. I only tested this with multi-process.  Not expecting any issues with
> single-process, but it certainly needs to be run through - single-process is
> using different browser & webNavigation objects.

Did a bit of manual testing with single-process too.  Works fine.

Leaving the rest in Christoph's able hands.

Comment 55

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1113431_referrer_tests.patch

Hey Sid,

I factored out the common code and extended the tests to cover the following policies:
* origin
* no-referrer
* no-referrer-when-downgrade
* origin-when-crossorigin
* unsafe-url

The tests cover going from:
* http->http
* http->https
* https->https
* https->http

Further, the tests include coverage for:
* simple left clicks
* right-click -> open link in tab
* ctrl-click

When thinking about it there are other scenarios as well, not sure if we need automated tests for those as well:
* drag and drop the URL into a new tab
* drag and drop the URL into a new window
* use keyboard navigation to open the link (e.g. using tab and then hitting ctrl-enter


Let me know what you think - all the tests work fine with Alex patch applied.

@Alex: Thanks again for providing the patch, I will follow your suggestions and finalize your work.

Comment 56

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Alex Verstak from comment #53)
> @Christoph: it'd be best if you could take this over - I'm short on time.  A
> few things you might want to consider:
> 
> 1. nsIWebNavigation::loadURIWithBase should probably be renamed to
> loadURIWithOptions and changed to take an options struct, similarly to
> nsIDocShell::loadURI.  A JS util function could convert the JS params dict
> to this struct, to make it easier to call.  (Changing
> nsIWebNavitation::loadURI seems like a non-starter - too many call sites;
> but changing loadURIWithBase sounds like a good idea - it's a subset, it's
> new, and it's only used in view-source.)

That sounds reasonable to me.

> 2. I only tested this with multi-process.  Not expecting any issues with
> single-process, but it certainly needs to be run through - single-process is
> using different browser & webNavigation objects.

Works fine on single process (at least passes all the automated tests we have so far).
 
> 3. The test should probably also cover origin-when-crossorigin.  Current
> tests cover propagation of the referrer policy, but not of the triggering
> principal.  It works, probably via some global state, but the test doesn't
> cover it.  A single same-origin case should be sufficient - if it uses the
> wrong triggering principal, it would truncate referrer to just origin.

That is a good point - I will incorporate such a test once I get feedback from Sid.

> (As an aside, if I were to pick just one test case for this bug, that would
> be HTTPS -> HTTP with meta-referrer=origin.  It would catch both failure to
> propagate the referrer policy, and also failure to trump the default policy.
> My second choice would be origin-when-crossorigin, to test propagation of
> the triggering principal.  My third choice would be rel=noreferrer, to test
> that it trumps the meta-tag.  And then the cross-product of the default
> referrer policy cases - they're certainly most important, but also least
> likely to break.  As Sid says, the more the merrier, but if there's need to
> prioritize tests, those would be my picks.)

Since I splitted the tests, they don't time out anymore and we can keep all of them.

> 4. There's also save-as, view-source, and print.  And metro and maybe other
> forks (android?).  I'd file separate bugs for those - the scope of this one
> is already plenty large enough.

I agree, this bug already covers a lot by itself.

Comment 57

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1113431_referrer.patch

Sid: I rebased Alex's patch and (for now) I just replaced loadURIWithBase to loadURIWithOptions. Even though I like Alex's suggestion in Comment 53 to use a struct that we can hand to loadURIWithOptions I am not sure if we want to introduce that change at the moment given that we have to uplift the change to 37.

I already had to rebase the patch and probably lots of things changed between 37 and now. In other words, I am worried that we introduce other problems if we perform too big of a change.

Currently loadURIWithOptions takes 7 arguments, which is not too bad in my opinion.

Anyway, I think the patch looks great by itself. What do you think?

Also, Alex left some comments in the code - can you also have a look at those and tell me what you think? e.g. saveURL.

So far this looks good, the patch passes all the automatic tests we have at the moment, but probably we should add some more (see my previous comment when I uploaded the latest test patch).

Comment 58

[Sylvestre Ledru [:Sylvestre]]

We won't fix that for 36

Comment 59

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8569532 [details] [diff] [review]
bug_1113431_referrer.patch

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

I think this looks good.  You'll obviously need review from browser/toolkit peers, and a super-review or at least interface design review for docshell (probably bz).  Here are a couple specific bits of feedback.

::: browser/base/content/browser.js
@@ +1982,5 @@
>  /* Required because the tab needs time to set up its content viewers and get the load of
>     the URI kicked off before becoming the active content area. */
>  function delayedOpenTab(aUrl, aReferrer, aCharset, aPostData, aAllowThirdPartyFixup)
>  {
> +  // TODO(averstak): referrerPolicy? this function appears unused.

Need to resolve or delete this todo.

::: browser/base/content/utilityOverlay.js
@@ +102,5 @@
>      params = {
>        allowThirdPartyFixup: aAllowThirdPartyFixup,
>        postData: aPostData,
>        referrerURI: aReferrerURI,
> +      // Default referrer policy is fine for UI links.

Please make the comment clear that you're omitting the referrer policy or set the default here.

@@ +198,5 @@
>      params = {
>        allowThirdPartyFixup: aAllowThirdPartyFixup,
>        postData: aPostData,
>        referrerURI: aReferrerURI
> +      // Default referrer policy is fine for UI links.

Here too, please.

@@ +233,5 @@
>        Components.utils.reportError("openUILink/openLinkIn was called with " +
>          "where == 'save' but without initiatingDoc.  See bug 814264.");
>        return;
>      }
> +    // TODO(averstak): referrerPolicy?

This todo needs addressing.

@@ +269,5 @@
>      allowThirdPartyFixupSupports.data = aAllowThirdPartyFixup;
>  
> +    var referrerPolicySupports = Cc["@mozilla.org/supports-PRInt32;1"].
> +                                 createInstance(Ci.nsISupportsPRInt32);
> +    referrerPolicySupports.data = aReferrerPolicy || 0;

Please use a referrer policy constant/enum instead of "0" here.
See nsIHttpChannel (http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#63).

::: docshell/base/nsDocShell.cpp
@@ +4859,5 @@
>  
>    loadInfo->SetLoadType(ConvertLoadTypeToDocShellLoadInfo(loadType));
>    loadInfo->SetPostDataStream(postStream);
>    loadInfo->SetReferrer(aReferringURI);
> +  loadInfo->SetReferrerPolicy(aReferrerPolicy);

Since this is an implementation of a semi-public interface, it's probably a good idea to do validation of the aReferrerPolicy argument value before setting loadInfo so we don't end up with some weird referrer policy value.

::: toolkit/components/viewsource/content/viewPartialSource.js
@@ +176,5 @@
> +  getWebNavigation().loadURIWithOptions((isHTML ?
> +                                         "view-source:data:text/html;charset=utf-8," :
> +                                         "view-source:data:application/xml;charset=utf-8,")
> +                                        + encodeURIComponent(tmpNode.innerHTML),
> +                                        loadFlags, null, 0, null, null,

Maybe clearer to use a constant/enum or explain why you're sending zero for this argument.

Comment 60

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8569473 [details] [diff] [review]
bug_1113431_referrer_tests.patch

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

These tests look pretty good.  I'm concerned they're still pretty long and may time out on some platforms, so a try run is warranted before getting these reviewed and landed.  Thanks for tackling this!

Comment 61

[Alex Verstak]

@Christoph - I have my evenings back, and can wrap this up if you prefer.  Please let me know.  My strategy would be to clean this up per Sid's comments, and to file followup bugs for anything that's non-trivial to do or to unittest.  Hoping that this makes it into 37, with some time to bake on the beta.

Comment 62

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Alex Verstak from comment #61)
> @Christoph - I have my evenings back, and can wrap this up if you prefer. 
> Please let me know.  My strategy would be to clean this up per Sid's
> comments, and to file followup bugs for anything that's non-trivial to do or
> to unittest.  Hoping that this makes it into 37, with some time to bake on
> the beta.

Alex, that's great - please do. I had everything on TRY the other day and seems tests time out:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f270fb202f56

The good news, your patch seems to work just fine. So, I am going to break the tests yet again (also including some same origin tests).

I will leave you patch untouched - thanks for doing all this - you are awesome!

Comment 63

[Alex Verstak]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #59)
> ::: browser/base/content/browser.js
> @@ +1982,5 @@
> >  /* Required because the tab needs time to set up its content viewers and get the load of
> >     the URI kicked off before becoming the active content area. */
> >  function delayedOpenTab(aUrl, aReferrer, aCharset, aPostData, aAllowThirdPartyFixup)
> >  {
> > +  // TODO(averstak): referrerPolicy? this function appears unused.
> 
> Need to resolve or delete this todo.

Deleted todo.  Seems unwise to increase the scope of this patch - it's plenty large enough already.

> @@ +233,5 @@
> >        Components.utils.reportError("openUILink/openLinkIn was called with " +
> >          "where == 'save' but without initiatingDoc.  See bug 814264.");
> >        return;
> >      }
> > +    // TODO(averstak): referrerPolicy?
> 
> This todo needs addressing.

Looks like bug 1073187; changed the comment to reference it.

> ::: docshell/base/nsDocShell.cpp
> @@ +4859,5 @@
> >  
> >    loadInfo->SetLoadType(ConvertLoadTypeToDocShellLoadInfo(loadType));
> >    loadInfo->SetPostDataStream(postStream);
> >    loadInfo->SetReferrer(aReferringURI);
> > +  loadInfo->SetReferrerPolicy(aReferrerPolicy);
> 
> Since this is an implementation of a semi-public interface, it's probably a
> good idea to do validation of the aReferrerPolicy argument value before
> setting loadInfo so we don't end up with some weird referrer policy value.

I don't think this is a good place to do validation.  It's just another stop on the long train to the http channel.  E.g., one could easily bypass it by QI'ing webnav to docshell and filling loadInfo directly.

Validation, if any, probably belongs all the way in HttpBaseChannel::SetReferrerWithPolicy, because that's the first and only function that does something useful with the value.

Thoughts?

Addressed all the other review comments.  Will slice it into smaller patches and send them out shortly.

Comment 64

[Alex Verstak]

Attachment: accept referrer policy in webNav.loadURIWithOptions

Renamed webNav.loadURIWithBase (added recently in bug 464222) to loadURIWithOptions and added the referrerPolicy argument.  Updated various wrappers and the only real call site - view-source.  Added an implementation to the e10s version.

Comment 65

[Alex Verstak]

Attachment: propagate referrer policy throughout the ui

Covers {command,middle}-click and open-in-new-{tab,window} from the context menu.  Tested manually and with Christoph's automatic test.

Comment 66

[Alex Verstak]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #62)
> I had everything on TRY the other day and seems tests time out:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f270fb202f56

Other alternatives:

1. requestLongerTimeout(240) at the top of test().

2. Reduce the number of test cases - see comment 53.

3. Parallelize the loads of the source pages, e.g., load them all into one tab with lots of iframes.  There'd still be 40 tabs/windows to open one-at-a-time after that, but at least it wouldn't open each source page in its own new tab too - so it should be 2X faster.

Comment 67

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8572479 [details] [diff] [review]
accept referrer policy in webNav.loadURIWithOptions

I'm not familiar with the background of loadURIWithBase, but assuming it is relatively new and that there are no compatibility issues with renaming it (including from possible use in add-ons), renaming it seems fine. I agree this is a better name.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8572479 [details] [diff] [review]
accept referrer policy in webNav.loadURIWithOptions

bz reviewed both bug 704320 and LoadURIWithBase, so would be better to get review from him.
But if he is busy, move the request back to my queue.

Comment 69

[:Gijs (out for now; he/him)]

Comment on attachment 8572483 [details] [diff] [review]
propagate referrer policy throughout the ui

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

I'll look at the rest tomorrow, but here are some initial comments from looking at the easy bits...

::: browser/components/sessionstore/ContentRestore.jsm
@@ +200,5 @@
>          }
>          let referrer = loadArguments.referrer ?
>                         Utils.makeURI(loadArguments.referrer) : null;
> +        let referrerPolicy = loadArguments.referrerPolicy ||
> +                             Ci.nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE;

Am I paranoid for wanting to encode this "default" somewhere else than in lots of hardcoded references to it, e.g. by adding a static readonly default policy property to nsIHttpChannel, rather than referring directly to this?

::: dom/webidl/Document.webidl
@@ +208,5 @@
> +   * Current referrer policy - one of the REFERRER_POLICY_* constants
> +   * from nsIHttpChannel.
> +   */
> +  [ChromeOnly]
> +  readonly attribute unsigned long? referrerPolicy;

I'm pretty sure I'm not officially able to review this bit of the patch. Should it have been in the other patch?

Comment 70

[Alex Verstak]

Attachment: Document and nsIWebNavigation changes for referrer policy in UI code

Sending this to bz because everyone is asking me to do that.

Full description in comment 64.  Added Document.referrerPolicy and nsIHttpChannel.REFERRER_POLICY_DEFAULT per request from Gijs.

Compatibility-wise, loadURIWithBase was added on 2014/2/20, and I updated all the references I could find on mozilla-central.  Not sure how to check the addons - https://mxr.mozilla.org/addons/ident?i=loadURIWithBase&filter= seems to require an AMO admin account.  I could, of course, easily add another function and leave this one alone, but my gut feeling is that it's not necessary here.

Comment 71

[Alex Verstak]

Attachment: propagate referrer policy throughout the ui

Addressed Gijs' feedback.  I do like REFERRER_POLICY_DEFAULT, I certainly do.

Comment 72

[Alex Verstak]

@Christoph - would you mind if I take over the tests too?  I'll still bug you to submit a try run - I don't have committer access - but I think it would be more efficient for a single person to own the bug.

If so, can you please upload the latest tests?  The patch here doesn't seem to have open-in-new-window.  This would save me a bit of time.

My strategy with tests would be to reduce the total number of cases to about twenty, per comment 53.  Opening a new tab takes several seconds on a debug build; doing this 40*4 times just doesn't seem to be worth it.  I think we'll get nearly all the benefit by testing propagation of two pieces of info - referrer policy and triggering principal - which can be covered with a small number of test cases.  There's little, if any, benefit from testing every combination of protocols and policy values - the UI code merely passes them on to the http channel, which is already covered by all-combinations tests.  Thoughts?

Comment 73

[:Gijs (out for now; he/him)]

(In reply to Alex Verstak from comment #70)
> Compatibility-wise, loadURIWithBase was added on 2014/2/20, and I updated
> all the references I could find on mozilla-central.  Not sure how to check
> the addons - https://mxr.mozilla.org/addons/ident?i=loadURIWithBase&filter=
> seems to require an AMO admin account.  I could, of course, easily add
> another function and leave this one alone, but my gut feeling is that it's
> not necessary here.

Indeed, this isn't called. That being said, I am worried about the add-on compat of the browser changes here... I'll have a look at that when I review (soon now, promise!).

Comment 74

[:Gijs (out for now; he/him)]

Comment on attachment 8573099 [details] [diff] [review]
propagate referrer policy throughout the ui

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

Please file a followup bug for seamonkey/thunderbird/comm-central, too. r- because of the browser.xml issue.

I've done a "positive" review, in that I've looked at all your changes and reviewed them. I haven't yet done a "negative" review, ie tried to go around the codebase to see if there's places you've missed. Before I do that (also considering this is for sure missing a bit of toolkit, and that might let you find more already), can you explain how you found / decided which bits to update here?

::: browser/base/content/browser.js
@@ +855,5 @@
>        LoadInOtherProcess(browser, {
>          uri: uri,
>          flags: flags,
>          referrer: referrer ? referrer.spec : null,
> +        referrerPolicy: referrerPolicy

Nit: add trailing comma, please (like previous line)

@@ +1193,3 @@
>        else if (window.arguments.length >= 3) {
>          loadURI(uriToLoad, window.arguments[2], window.arguments[3] || null,
> +                window.arguments[4] || false, window.arguments[5] || 0);

Nit: The 0 here should be a reference to the DEFAULT_POLICY thing you added, so that if we change the default, it'll magically Just Work. :-)

Probably makes sense to stick that in a temp var so as not to make this fn call completely unreadable.

@@ +4600,5 @@
>  nsBrowserAccess.prototype = {
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIBrowserDOMWindow, Ci.nsISupports]),
>  
> +  _openURIInNewTab: function(aURI, aReferrer, aReferrerPolicy, aIsPrivate,
> +                             aIsExternal, aForceNotRemote=false) {

addons mxr indicates this might break tabmix plus.

Because this is "private" (underscore prefix), I don't think we should make the function signature ugly just to avoid this (and even if we did, it'd likely mean tabmixplus would *still* need work to get the referrer policy thing right), but I'll add keywords and CC the author.

@@ +4651,5 @@
>      var newWindow = null;
>      var isExternal = (aContext == Ci.nsIBrowserDOMWindow.OPEN_EXTERNAL);
> +    var referrer = aOpener ? makeURI(aOpener.location.href) : null;
> +    var referrerPolicy =
> +        (aOpener && aOpener.document && aOpener.document.referrerPolicy) ||

Why are you falsy-checking aOpener.document.referrerPolicy? I guess it's equivalent now because of the ||, but if the default were different, this would ignore the int being set to 0 (which is also falsy in JS).

Let's instead do:

var referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT;
if (aOpener && aOpener.document) {
  referrerPolicy = aOpener.document.referrerPolicy;
}

Which should always work, AIUI.

@@ +4658,5 @@
>      if (aOpener && isExternal) {
>        Cu.reportError("nsBrowserAccess.openURI did not expect an opener to be " +
>                       "passed if the context is OPEN_EXTERNAL.");
>        throw Cr.NS_ERROR_FAILURE;
>      }

Hm, looks like you moved these definitions quite early. They should go after this check (which throws), at least - probably right before the definition of isPrivate would make sense?

::: browser/base/content/tabbrowser.xml
@@ +1854,5 @@
>                  flags |= Ci.nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL;
>                if (aAllowMixedContent)
>                  flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT;
>                try {
> +                b.loadURIWithFlags(aURI, {

b here is a <browser>, not a <tabbrowser>, so if you do this you need to update browser.xml to deal with this change, too...

That's a little bit of a can of worms, because it's toolkit... but I think you could probably make it take the same kind of object as you've written here, and call webNavigation.loadURIWithOptions instead of just loadURI.

This does make me wonder how you tested this change. We know from previous mucking about with aNoReferrer that our automated tests here are deficient and we break stuff when we change pretty much anything. These are already pretty big patches so I feel a little bad asking it, but if you *could* write *some* tests, at least, that would make me feel better.

@@ +5557,5 @@
>            <![CDATA[
> +            var params = arguments[1];
> +            if (typeof(params) == "number") {
> +              params = {
> +                flags: aFlags, referrerURI: aReferrerURI, charset: aCharset,

Nit: one per line, please

@@ +5558,5 @@
> +            var params = arguments[1];
> +            if (typeof(params) == "number") {
> +              params = {
> +                flags: aFlags, referrerURI: aReferrerURI, charset: aCharset,
> +                postData: aPostData

Nit: add trailing comma

@@ +5584,5 @@
> +            var params = arguments[1];
> +            if (typeof(params) == "number") {
> +              params = {
> +                flags: aFlags, referrerURI: aReferrerURI, charset: aCharset,
> +                postData: aPostData

Same here.

::: browser/base/content/utilityOverlay.js
@@ +87,5 @@
>   * accepted by openUILinkIn, plus "ignoreButton" and "ignoreAlt".
>   */
>  function openUILink(url, event, aIgnoreButton, aIgnoreAlt, aAllowThirdPartyFixup,
>                      aPostData, aReferrerURI) {
> +  const Ci = Components.interfaces;

Nit: I'd just:

const kDefaultPolicy = Components.interfaces.nsIHttpChannel.REFERRER_POLICY_DEFAULT;

@@ +190,5 @@
>   *   skipTabAnimation     (boolean)
>   *   allowPinnedTabHostChange (boolean)
>   */
>  function openUILinkIn(url, where, aAllowThirdPartyFixup, aPostData, aReferrerURI) {
> +  const Ci = Components.interfaces;

Same.

@@ +200,5 @@
>      params = {
>        allowThirdPartyFixup: aAllowThirdPartyFixup,
>        postData: aPostData,
> +      referrerURI: aReferrerURI,
> +      referrerPolicy: Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT

Nit: add trailing comma here so we don't have to touch again if we're adding more properties.

@@ +341,5 @@
>        flags |= Ci.nsIWebNavigation.LOAD_FLAGS_DISALLOW_INHERIT_OWNER;
>  
> +    w.gBrowser.loadURIWithFlags(url, {
> +      flags: flags, referrerURI: aNoReferrer ? null : aReferrerURI,
> +      referrerPolicy: aReferrerPolicy, postData: aPostData});

Nit: now that this is already multiline, this would be slightly easier to read (esp. with the ternary) if every property was on its own line.

Comment 75

[:Gijs (out for now; he/him)]

Marking as add-on compat, see comment #74 (but note that a bunch of other fn signature changes may also impact add-ons, as will the IDL changes in the Document/nsIWebNavigation code).

Comment 76

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Alex Verstak from comment #72)
> @Christoph - would you mind if I take over the tests too?  I'll still bug
> you to submit a try run - I don't have committer access - but I think it
> would be more efficient for a single person to own the bug.

Alex, I don't mind at all and I think that makes perfect sense. You could/should also assign the bug to yourself so you get the credit for doing all the work here.

> If so, can you please upload the latest tests?  The patch here doesn't seem
> to have open-in-new-window.  This would save me a bit of time.

It was included in the try run, find it here
> https://hg.mozilla.org/try/rev/f270fb202f56
and just click 'raw' so you can download a diff.

> My strategy with tests would be to reduce the total number of cases to about
> twenty, per comment 53.  Opening a new tab takes several seconds on a debug
> build; doing this 40*4 times just doesn't seem to be worth it.  I think
> we'll get nearly all the benefit by testing propagation of two pieces of
> info - referrer policy and triggering principal - which can be covered with
> a small number of test cases.  There's little, if any, benefit from testing
> every combination of protocols and policy values - the UI code merely passes
> them on to the http channel, which is already covered by all-combinations
> tests.  Thoughts?

Sid and I discussed that yesterday and since the bug needs to be uplifted, we would like to have tests for all the different combinations (basically what we have at the moment). Plus, we would also need one for same origin (as you mentioned previously) which is not included in the current tests.
One other reason why we want all the combinations is that we could eliminate the original tests or at least convert them to fit into this new 'referrer test suite' we are about to create with this bug.

But we can ask Sid again to make sure! Sid?

Comment 77

[Alex Verstak]

(In reply to :Gijs Kruitbosch from comment #74)
> I've done a "positive" review, in that I've looked at all your changes and
> reviewed them. I haven't yet done a "negative" review, ie tried to go around
> the codebase to see if there's places you've missed. Before I do that (also
> considering this is for sure missing a bit of toolkit, and that might let
> you find more already), can you explain how you found / decided which bits
> to update here?

It all works. :-)  With and without e10s.

Tested by hand - middle-click and open-link-in-new-{tab,window,private-window} from the context menu with various referrer policies, then printed document.referrer on the console.  See comment 0.

Also ran Christoph's excellent automatic tests - see his tests patch on top.  We need to trim these tests down a bit because they take too long, and cover interaction with rel=noreferrer.  But they actually do test exactly this bug.

You might want to apply the two patches and try it out yourself.  It's quick to test by hand - see comment 0.

Will address detailed feedback and upload another version in the evening, when I have a bit of time.  Thanks for the lightning fast review!

Comment 78

[Alex Verstak]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #76)
> Sid and I discussed that yesterday and since the bug needs to be uplifted,
> we would like to have tests for all the different combinations (basically
> what we have at the moment). Plus, we would also need one for same origin
> (as you mentioned previously) which is not included in the current tests.

Also rel=noreferrer should trump the meta-tag.

> One other reason why we want all the combinations is that we could eliminate
> the original tests or at least convert them to fit into this new 'referrer
> test suite' we are about to create with this bug.

I'm a noob here, so I have a very basic question - how long is too long for a UI test?  Suppose it takes ten minutes overall.  Is it reasonable to just shard it into ten 1-minute tests?  Or do I have to worry about the try-bot's overall capacity at this point?  How much of the overall try-bot capacity can I reasonably use for these tests - is there a guideline / limit?  Does it even run different UI tests in parallel, or am I going to slow down everyone's try runs by ten minutes no matter how the test is sharded?

Asking this because the http channel tests are much faster to run than the UI tests.  The channel tests can load lots of iframes in parallel, but the UI tests are limited to one click at a time, which takes several seconds per click.  If we're running up against the overall time limit, then it would make more sense to keep the exhaustive tests in the http channel, and keep the UI tests small and focused.

Also, keep in mind that adding interactions with rel=noreferrer will double the current test time.  And then someone's gonna land the referrer attribute, which would be another 3X.  And then there will be referrer CSP or something...  I'm afraid that no matter what the time limit is, all-combinations UI tests will exceed it soon enough.

Sid - thoughts?

Comment 79

[Sid Stamm [:geekboy or :sstamm]]

Yeah.

> If we're running up against the overall time limit, then it would make more sense 
> to keep the exhaustive tests in the http channel, and keep the UI tests small and 
> focused.

Do this.  Doesn't add a whole lot to do the exhaustive tests in an expensive way when we can check completeness in an easier way and spot-check using the UI tests.  Also the tests run on every check-in, so adding 10 min to the tests gets expensive quickly.

You can just spot-check rel=noreferrer too, don't need to be overly-complete since the "overrides" logic should be the same code path for many of the cases.

Comment 80

[Christoph Kerschbaumer [:ckerschb]]

Alex: I did some more manual testing and it 'seems drag and drop into a new tab' doesn't propagate the referrer. I will try to add an automatic testcase for that today. For now, I will use my current testsetup and then I let you incorporate the test into your setup. The integration should be fairly simple, I guess finding out how the drag and drop works automatically is the harder part.

Any chance you an you take a look at your patch and try to find out why it's not propagating for drag and drop operations?

Comment 81

[Alex Verstak]

@Christoph - IIRC, the drag-and-drop code path doesn't propagate referrer at all, even in the normal meta-referrer-less same-origin HTTP->HTTP scenario.  My patch didn't change this; it seems unnecessary.

If that's something we want to reconsider, it would be best to file a separate bug.  The code change should be trivial, but I'm not sure that we want to do this from the policy perspective.  Actually, I'm pretty sure that we don't - neither FF35 nor Chrome41 currently set referrer on drag-and-drop, even with meta-referrer=always.  This seems to be working as intended; but please do file a separate bug if you disagree.

(OTOH, if you have an automatic test that covers drag-and-drop, I'd love to include it, and test that there's no referrer in this case.)

Comment 82

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8573099 [details] [diff] [review]
propagate referrer policy throughout the ui

Clearing review flag until you've addressed comments from Gijs

Comment 83

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8573098 [details] [diff] [review]
Document and nsIWebNavigation changes for referrer policy in UI code

I guess the webnav change is OK.  Chances are not many binary things are using that API yet, and addons mxr shows no JS uses.

For the webidl change, though that return value should not be nullable.  The fact that it even compiles that way doesn't make me terribly happy, though I haven't figured out how to prevent it yet...

Please make it non-nullable.  sr=me with that.

Comment 84

[Christoph Kerschbaumer [:ckerschb]]

Attachment: dragdrop.patch

(In reply to Alex Verstak from comment #81)
> @Christoph - IIRC, the drag-and-drop code path doesn't propagate referrer at
> all, even in the normal meta-referrer-less same-origin HTTP->HTTP scenario. 
> My patch didn't change this; it seems unnecessary.

Yes, you are right. I should have looked at the code first but nevertheless, I don't understand why:
* right click - open link in new tab, and
* drag and drop into new tab
are semantically any different and why one propagates the referrer and the other does not.

Anyway, I will follow up with Sid and others and I think we have to file a follow up to get that fixed so that both of these cases propagate the referrer.

> If that's something we want to reconsider, it would be best to file a
> separate bug.  The code change should be trivial, but I'm not sure that we
> want to do this from the policy perspective.  Actually, I'm pretty sure that
> we don't - neither FF35 nor Chrome41 currently set referrer on
> drag-and-drop, even with meta-referrer=always.  This seems to be working as
> intended; but please do file a separate bug if you disagree.
> 
> (OTOH, if you have an automatic test that covers drag-and-drop, I'd love to
> include it, and test that there's no referrer in this case.)

I am attaching a testcase that tests drag and drop operations. Just a few notes:
* I had to copy in a few helper functions from:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/newtab/head.js#495
to make it work - unfortunately we end up having some duplicate code, but I think that's ok for testcode.

* Using 'todo_is' when performing the actual test - we have to update that, see the following link for any help:
https://developer.mozilla.org/en-US/docs/Mochitest#Test_functions
but I think the tricky part of the test is already there.


Please let me know if I can be of any more help - thanks again for taking on that work.

Comment 85

[Alex Verstak]

Attachment: Document and nsIWebNavigation changes for referrer policy in UI code

Made Document.referrerPolicy non-nullable per bz's request, plus bindings.conf magic to make that compile.  Carrying sr=bz.

@Sid - PTAL.  I think this addresses everyone's high-level API concerns, but I don't think I ever got a regular r+.

Comment 86

[Alex Verstak]

(In reply to :Gijs Kruitbosch from comment #74)
> Please file a followup bug for seamonkey/thunderbird/comm-central, too.

Yep, will do.  Also android and metro, right?

> I've done a "positive" review, in that I've looked at all your changes and
> reviewed them. I haven't yet done a "negative" review, ie tried to go around
> the codebase to see if there's places you've missed. Before I do that (also
> considering this is for sure missing a bit of toolkit, and that might let
> you find more already), can you explain how you found / decided which bits
> to update here?

Logging and divination...

> ::: browser/base/content/tabbrowser.xml
> @@ +1854,5 @@
> >                  flags |= Ci.nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL;
> >                if (aAllowMixedContent)
> >                  flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT;
> >                try {
> > +                b.loadURIWithFlags(aURI, {
> 
> b here is a <browser>, not a <tabbrowser>, so if you do this you need to
> update browser.xml to deal with this change, too...
> 
> That's a little bit of a can of worms, because it's toolkit... but I think
> you could probably make it take the same kind of object as you've written
> here, and call webNavigation.loadURIWithOptions instead of just loadURI.

It appears to call the loadURIWithOptions variants in tabbrowser.xml.  The one in toolkit/.../browser.xml was never called during my manual testing with various ways to open new tabs/windows.  Non-e10s calls the version in the @tabbrowser-browser bindings; and e10s calls a combination of @tabbrowser-remote-browser and @tabbrowser-browser - they're both in tabbrowser.xml and are both updated in this patch.  (I don't know how this works internally; I just added a bit of logging to track down which functions get called.)

I could update browser.xml too, of course, but I don't know how to test that?  Nothing that I've done triggered that code path.  I added logging to it too, but never saw it log.

> This does make me wonder how you tested this change. We know from previous
> mucking about with aNoReferrer that our automated tests here are deficient
> and we break stuff when we change pretty much anything. These are already
> pretty big patches so I feel a little bad asking it, but if you *could*
> write *some* tests, at least, that would make me feel better.

Yes, and they're good tests too, see comment 77 & Christoph's patch.  Will send you the tests patch when it's ready - sorry, didn't have time to finish this tonight.

Comment 87

[Alex Verstak]

Attachment: propagate referrer policy throughout the ui

Addressed Gijs' style comments.  Allow non-zero default referrer policy (hypothetical, future).  Tested by hand and with Christoph's lovely tests patch.

Comment 88

[:Gijs (out for now; he/him)]

(In reply to Alex Verstak from comment #86)
> (In reply to :Gijs Kruitbosch from comment #74)
> > Please file a followup bug for seamonkey/thunderbird/comm-central, too.
> 
> Yep, will do.  Also android and metro, right?

Yes.

> > I've done a "positive" review, in that I've looked at all your changes and
> > reviewed them. I haven't yet done a "negative" review, ie tried to go around
> > the codebase to see if there's places you've missed. Before I do that (also
> > considering this is for sure missing a bit of toolkit, and that might let
> > you find more already), can you explain how you found / decided which bits
> > to update here?
> 
> Logging and divination...

Heh, fair.

> > ::: browser/base/content/tabbrowser.xml
> > @@ +1854,5 @@
> > >                  flags |= Ci.nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL;
> > >                if (aAllowMixedContent)
> > >                  flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT;
> > >                try {
> > > +                b.loadURIWithFlags(aURI, {
> > 
> > b here is a <browser>, not a <tabbrowser>, so if you do this you need to
> > update browser.xml to deal with this change, too...
> > 
> > That's a little bit of a can of worms, because it's toolkit... but I think
> > you could probably make it take the same kind of object as you've written
> > here, and call webNavigation.loadURIWithOptions instead of just loadURI.
> 
> It appears to call the loadURIWithOptions variants in tabbrowser.xml.  The
> one in toolkit/.../browser.xml was never called during my manual testing
> with various ways to open new tabs/windows.  Non-e10s calls the version in
> the @tabbrowser-browser bindings; and e10s calls a combination of
> @tabbrowser-remote-browser and @tabbrowser-browser - they're both in
> tabbrowser.xml and are both updated in this patch.  (I don't know how this
> works internally; I just added a bit of logging to track down which
> functions get called.)

Ah, d'oh. Sorry, I should have realized that. You're right. I can tell you how it works: they are extensions of the default binding, and they are applied to the browsers that we create in tabbrowser through this bit of CSS: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#49

> I could update browser.xml too, of course, but I don't know how to test
> that?  Nothing that I've done triggered that code path.  I added logging to
> it too, but never saw it log.

Hmmmmm. For regular Firefox, this is not strictly necessary. For toolkit, it will be, I think.

I wonder how all this should work with view-source URLs. You can click links in those, and I expect they will use the regular toolkit browser binding, though I've not checked. I also don't know if the referrer policy of the original document does/should apply to the view-source document. (Though I expect it currently doesn't... not sure if that's an issue, and/or how much we care here.)

The other things that use regular browsers will probably still want updating... but we can leave that for a followup. We'd need to audit the relevant code (I suspect at least sidebars, social and Firefox Hello, off the top of my head). This patch is big enough as it is.

> > This does make me wonder how you tested this change. We know from previous
> > mucking about with aNoReferrer that our automated tests here are deficient
> > and we break stuff when we change pretty much anything. These are already
> > pretty big patches so I feel a little bad asking it, but if you *could*
> > write *some* tests, at least, that would make me feel better.
> 
> Yes, and they're good tests too, see comment 77 & Christoph's patch.  Will
> send you the tests patch when it's ready - sorry, didn't have time to finish
> this tonight.

Great!

Comment 89

[:Gijs (out for now; he/him)]

Comment on attachment 8573866 [details] [diff] [review]
propagate referrer policy throughout the ui

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

r+ on the assumption that tests. :-)

Comment 90

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8573850 [details] [diff] [review]
Document and nsIWebNavigation changes for referrer policy in UI code

Please don't binarynames stuff unless it's absolutely necessary.  Why not just give the nsIDocument member the right name?

Comment 91

[Lawrence Mandel [:lmandel] (use needinfo)]

Unless there is a real pressing reason to ship the feature in 37 (more than comment 19), given that the change looks non trivial, that it hasn't landed on m-c yet, and that we're already past the cutoff for slight risk in the shorter 37 cycle, I would prefer that we disable the feature again in 37 (like bug 1134606 did for 36) and target 38.

Sid - Can you live with this?

Comment 92

[Alex Verstak]

@bz - that doesn't compile; it would need to be called ReferrerPolicy which is also an enum.  I could add a thin wrapper called ReferrerPolicyFromJS to follow existing convention, but there doesn't seem to be a good way to avoid binarynames entirely.  Tried removing the typedef and using fully qualified mozilla::net::ReferrerPolicy, but that looks inconsistent with the rest of this code.  Thoughts?

Comment 93

[Boris Zbarsky [:bzbarsky]]

typedef mozilla::net::ReferrerPolicy to ReferrerPolicyEnum?

Comment 94

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #91)
> Sid - Can you live with this?

Yes. The patch for this bug has indeed turned out to be more complicated than I initially thought.

Comment 95

[Lawrence Mandel [:lmandel] (use needinfo)]

I have marked this bug as wontfix for 37. We can still consider uplift for 38.

Comment 96

[Alex Verstak]

Attachment: Document and nsIWebNavigation changes for referrer policy in UI code

Implemented bz's suggestion to avoid bindings.conf.  Carrying over his sr+.  Still need a regular review from someone, I think - flagging Sid.

Comment 97

[Alex Verstak]

(In reply to :Gijs Kruitbosch from comment #88)
> > I could update browser.xml too, of course, but I don't know how to test
> > that?  Nothing that I've done triggered that code path.  I added logging to
> > it too, but never saw it log.
> 
> Hmmmmm. For regular Firefox, this is not strictly necessary. For toolkit, it
> will be, I think.
> 
> I wonder how all this should work with view-source URLs. You can click links
> in those, and I expect they will use the regular toolkit browser binding,
> though I've not checked.

View-source doesn't seem to call browser.xml either, but I just went ahead and changed that anyway.  It's a backwards compatible change; and it would be odd to have different interfaces for different browser objects.  Also added a comment to one of the three versions in tabbrowser.xml - it doesn't look like it supports a params object, but it actually does.

> I also don't know if the referrer policy of the
> original document does/should apply to the view-source document. (Though I
> expect it currently doesn't... not sure if that's an issue, and/or how much
> we care here.)

It gets an empty referrer from view-source, which seems just fine to me.

> The other things that use regular browsers will probably still want
> updating... but we can leave that for a followup. We'd need to audit the
> relevant code (I suspect at least sidebars, social and Firefox Hello, off
> the top of my head). This patch is big enough as it is.

All right, followup it is.  I wouldn't even know all the places to look at... :-)

Comment 98

[Alex Verstak]

Attachment: propagate referrer policy throughout the ui

PTAL at toolkit browser.xml changes.  I know you already r+'ed the rest.

Comment 99

[Alex Verstak]

Attachment: test cases - click, ctrl-click, new-{tab,window}

Adopted from Christoph's excellent patch.  Narrowed down from 20 to 6 key cases, so it should be fast enough now.  Added plain-jane cases, interaction with rel=noreferrer, and a same-origin origin-when-crossorigin test.  Skipped drag-and-drop for now - ran out of time.  All four tests pass locally on linux-debug, and take 70-90 seconds a pop.

Comment 100

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8574270 [details] [diff] [review]
Document and nsIWebNavigation changes for referrer policy in UI code

Yeah, this seems reasonable....

Comment 101

[Alex Verstak]

Filed followup bugs: 1141140 (comm-central), 1141142 (android), 1141143 (metro), 1141146 (sidebars, etc.).  There's also 1073187 for download.

Comment 102

[:Gijs (out for now; he/him)]

Comment on attachment 8574272 [details] [diff] [review]
propagate referrer policy throughout the ui

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

r=me, but we shouldn't forget the followup per comment 97 / 88.

Comment 103

[:Gijs (out for now; he/him)]

Comment on attachment 8574273 [details] [diff] [review]
test cases - click, ctrl-click, new-{tab,window}

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

Can we put these in their own folder? "general" has something like 300 tests and if we keep dumping in there that will never improve... :-)

Also, I'm a little concerned that we're limiting the tests because of timeouts... Equally, I don't have quick fixes to split the tests up further, short of numbering them and splitting the test suites.

The actual bad news (besides nits/comments below) is that this won't run in --e10s mode, and ideally we shouldn't really add the tests before we've fixed that. However... it seems that the test relies on itself calling contentAreaClick. I don't know why it does that and why it can't rely on the browser's own handling here. I don't think we can call the equivalent e10s stuff ( http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#640 ) because that just defines an object as a local variable. We'd have to change that and stick it on |this| in order to access it from another content script... 

It seems like the "simple_click" code doesn't need to do this, though. Can we investigate fixing the content menu case to also not need more motivation than just synthesizing the click? :-)

::: browser/base/content/test/general/BrowserReferrerTestUtils.jsm
@@ +6,5 @@
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = [
> +  'BrowserReferrerTestUtils',

I feel bad saying this but... please use double quotes throughout all the JS here.

@@ +117,5 @@
> +    let query = '?scheme=' + escape(aTest.toScheme) +
> +                '&policy=' + escape(aTest.policy || '') +
> +                '&rel=' + escape(aTest.rel || '');
> +    aWin.gBrowser.addTab(aTest.fromScheme + BASE_URL + query);
> +    aWin.gBrowser.selectTabAtIndex(1);

aWin.gBrowser.selectedTab = aWin.gBrowser.addTab(...);

@@ +118,5 @@
> +                '&policy=' + escape(aTest.policy || '') +
> +                '&rel=' + escape(aTest.rel || '');
> +    aWin.gBrowser.addTab(aTest.fromScheme + BASE_URL + query);
> +    aWin.gBrowser.selectTabAtIndex(1);
> +    this.waitForSomeTabToLoad(aWin, aCallback);

BrowserTestUtils.browserLoaded(aWin.gBrowser.selectedBrowser).then(aCallback);

::: browser/base/content/test/general/file_referrer_policyserver.sjs
@@ +10,5 @@
> +
> +  request.queryString.split('&').forEach(function(val) {
> +    let [name, value] = val.split('=');
> +    query[name] = unescape(value);
> +  });

Let's use URLSearchParams instead.

@@ +14,5 @@
> +  });
> +
> +  let policy = query.policy;
> +  let metaReferrerTag =
> +      (policy && '<meta name="referrer" content="' + policy + '">') || '';

Nit: template string.

@@ +19,5 @@
> +  let linkUrl = query.scheme +
> +                'test1.example.com/browser/browser/base/content/test/general/' +
> +                'file_referrer_testserver.sjs';
> +  let relAttr =
> +      (query.rel && ' rel="' + query.rel + '"') || '';

Why is this its own line? Should fit perfectly well on the previous line, surely?

@@ +32,5 @@
> +             metaReferrerTag +
> +             '<title>Test referrer</title>' +
> +             '</head>' +
> +             '<body>' + linkTag + '</body>' +
> +             '</html>';

Please use a template string instead.

::: browser/base/content/test/general/file_referrer_testserver.sjs
@@ +23,5 @@
> +             '</head>' +
> +             '<body>' +
> +             '<div id="testdiv">' + referrer + '</div>' +
> +             '</body>' +
> +             '</html>';

More template string fodder.

Comment 104

[Alex Verstak]

(In reply to :Gijs Kruitbosch from comment #103)
> Also, I'm a little concerned that we're limiting the tests because of
> timeouts... Equally, I don't have quick fixes to split the tests up further,
> short of numbering them and splitting the test suites.

How long is too long, even with sharding?  See comment 78.

(They would also be duplicating an exhaustive channel test, so I'm not sure there would be any real benefit...)

> The actual bad news (besides nits/comments below) is that this won't run in
> --e10s mode, and ideally we shouldn't really add the tests before we've
> fixed that. However... it seems that the test relies on itself calling
> contentAreaClick. I don't know why it does that and why it can't rely on the
> browser's own handling here. I don't think we can call the equivalent e10s
> stuff (
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.
> js#640 ) because that just defines an object as a local variable. We'd have
> to change that and stick it on |this| in order to access it from another
> content script... 
> 
> It seems like the "simple_click" code doesn't need to do this, though. Can
> we investigate fixing the content menu case to also not need more motivation
> than just synthesizing the click? :-)

Removed the call to contentAreaClick, I'm not sure why it was there either.

But I have a more basic problem in e10s - waitForFocus() doesn't, says "child process window cannot be focused".  Will poke around when I have a bit of time.  This is even in the simple click test.

Comment 105

[Alex Verstak]

Attachment: test cases - click, ctrl-click, new-{tab,window}

WIP.  Addressed Gijs' style comments.  Made logic clearer.  Still no cigar with e10s, but for a different reason - waitForFocus() doesn't like me somehow.

Comment 106

[:Gijs (out for now; he/him)]

(In reply to Alex Verstak from comment #105)
> waitForFocus() doesn't like me somehow.

How?

You probably need to ensure that you either pass it a chrome window when calling it in a chrome process, and that you can access the function and have it work and pass it a content window when calling it in a content process.

Comment 107

[:Gijs (out for now; he/him)]

(In reply to Alex Verstak from comment #104)
> (In reply to :Gijs Kruitbosch from comment #103)
> > Also, I'm a little concerned that we're limiting the tests because of
> > timeouts... Equally, I don't have quick fixes to split the tests up further,
> > short of numbering them and splitting the test suites.
> 
> How long is too long, even with sharding?  See comment 78.

Ah. So any machine will only run one test in parallel (mostly because browser_ tests are basically integration tests that (in some cases) rely on OS-level focus and such, so parallelizing them is... not easy), so no need to worry about that.

AFAIK the only issue is ensuring that (debug/asan) tests (comfortably) complete within the timeout limit on the hardware we use. So yes, splitting in 10 * 1min tests is totally an option from that perspective.


> (They would also be duplicating an exhaustive channel test, so I'm not sure
> there would be any real benefit...)

I don't know what you mean here, although I guess you're saying there are unit tests for the underlying DOM/HttpChannel bits? That's nice, but it's still valuable to have the tests in this patch considering that in fact, those existing unit tests are passing but in practice the browser isn't doing the right thing. :-)

One thing we could potentially do is that rather than the integration-test-type approach the tests use, to directly call methods and ensure that they call the underlying DOM/webidl stuff with the right parameters as a result. But in the absence of a good mocking tool, that is difficult, and it would be quite tightly coupled to the current implementation, and will make it easy to miss stuff - both of which will make them less future-proof as tests go.

> > The actual bad news (besides nits/comments below) is that this won't run in
> > --e10s mode, and ideally we shouldn't really add the tests before we've
> > fixed that. However... it seems that the test relies on itself calling
> > contentAreaClick. I don't know why it does that and why it can't rely on the
> > browser's own handling here. I don't think we can call the equivalent e10s
> > stuff (
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.
> > js#640 ) because that just defines an object as a local variable. We'd have
> > to change that and stick it on |this| in order to access it from another
> > content script... 
> > 
> > It seems like the "simple_click" code doesn't need to do this, though. Can
> > we investigate fixing the content menu case to also not need more motivation
> > than just synthesizing the click? :-)
> 
> Removed the call to contentAreaClick, I'm not sure why it was there either.

\o/

> But I have a more basic problem in e10s - waitForFocus() doesn't, says
> "child process window cannot be focused".  Will poke around when I have a
> bit of time.  This is even in the simple click test.

(sorry for missing this comment initially)

Looking at this, it's likely because you're asking for a content window in aTestWindow, and SimpleTest.waitForFocus tries to find the browser for the window by asking the tabbrowser element in its own window, which obviously isn't going to work (wrong chrome window).

This is an issue with waitForFocus. See:

http://hg.mozilla.org/mozilla-central/annotate/c42e7e3bb0a0/testing/mochitest/tests/SimpleTest/SimpleTest.js#l823

Asking around there doesn't seem to be a way to determine the toplevel chrome window from the content window in e10s land. There are two possibilities:
a) add a param to pass the chrome window
b) write another inner function to get the parent window by looping over the windows of type "navigator:browser" using something like:

let wm = Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator);
let windows = wm.getEnumerator("navigator:browser");
while (windows.hasMoreElements()) {
  let win = windows.getNext();
  let tabbrowser = win.gBrowser;
  // see if the tabbrowser knows about the inner window, and if so, return that window.
}
// Nope!



Does that help?

Comment 108

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #107)
> (In reply to Alex Verstak from comment #104)
> > But I have a more basic problem in e10s - waitForFocus() doesn't, says
> > "child process window cannot be focused".  Will poke around when I have a
> > bit of time.  This is even in the simple click test.
> 
> (sorry for missing this comment initially)
> 
> Looking at this, it's likely because you're asking for a content window in
> aTestWindow, and SimpleTest.waitForFocus tries to find the browser for the
> window by asking the tabbrowser element in its own window, which obviously
> isn't going to work (wrong chrome window).
> 
> This is an issue with waitForFocus. See:
> 
> http://hg.mozilla.org/mozilla-central/annotate/c42e7e3bb0a0/testing/
> mochitest/tests/SimpleTest/SimpleTest.js#l823
> 
> Asking around there doesn't seem to be a way to determine the toplevel
> chrome window from the content window in e10s land. There are two
> possibilities:
> a) add a param to pass the chrome window
> b) write another inner function to get the parent window by looping over the
> windows of type "navigator:browser" using something like:
> 
> let wm =
> Components.classes["@mozilla.org/appshell/window-mediator;1"].
> getService(Components.interfaces.nsIWindowMediator);
> let windows = wm.getEnumerator("navigator:browser");
> while (windows.hasMoreElements()) {
>   let win = windows.getNext();
>   let tabbrowser = win.gBrowser;
>   // see if the tabbrowser knows about the inner window, and if so, return
> that window.
> }
> // Nope!
> 
> 
> 
> Does that help?

... or you could wait for bug 1131656 to be fixed (and/or apply that patch for now). :-\

(Only heard about this after I finished writing the previous comment!)

Comment 109

[Alex Verstak]

Attachment: test cases - click, middle-click, new-{tab,window}

Mostly works in e10s now.

1. The new-window e10s version spins at 100% CPU for a few minutes during shutdown, and then complains that it leaked 16 bytes in WeakReference<MessageListener>.  I'm getting a funny feeling that opening new windows is not well covered by e10s tests, and this is something totally unrelated to my changes...  Am inclined to disable new-window in e10s and file a followup bug; the other three tests run fine.

2. I'll make you more test cases once you're happy with the basics.  Don't want to iterate on the full enchilada - current set is 3min non-e10s + 7min e10s; full set would be 7X more cases - so, an hour.  Definitely too slow for iteration.  Actually, do you still want it at all?  Yes, I can shard it, but running locally would still take an hour...

Comment 110

[:Gijs (out for now; he/him)]

Comment on attachment 8575868 [details] [diff] [review]
test cases - click, middle-click, new-{tab,window}

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

Unfortunately the 'real' frontend patch has bitrotted (specifically the context menu bit), so now it's more difficult to test this. I'm looking into this. Some code-inspection-based feedback below.

::: browser/base/content/test/referrer/browser_referrer_middle_click.js
@@ +4,5 @@
> +function startTestCase(aTestNumber, aTestWindow) {
> +  info("browser_referrer_middle_click: " +
> +       getReferrerTestDescription(referrerTests[aTestNumber]));
> +  newTabLoaded(aTestWindow).then(function(aNewTab) {
> +    aTestWindow.gBrowser.selectedTab = aNewTab;

You probably don't need to select the tab, but OTOH it'll help with debugging on try if it fails there (the screenshot will show it if it did open and then timed out / stopped / whatever), so let's leave it in.

@@ +16,5 @@
> +function checkReferrerInNewTab(aTestNumber, aTestWindow, aNewTab) {
> +  referrerResultReady(aTestWindow).then(function(result) {
> +    let test = referrerTests[aTestNumber];
> +    let desc = getReferrerTestDescription(test);
> +    is(result, test.result, desc);

The block of the 3 lines here is the same all the time. Can you factor it into referrerResultReady(aTestWindow) ?

@@ +23,5 @@
> +    aTestWindow.gBrowser.removeTab(aNewTab);
> +    is(aTestWindow.gBrowser.tabs.length, 2, "two tabs open");
> +    aTestWindow.gBrowser.removeTab(aTestWindow.gBrowser.tabs[1]);
> +
> +    // Move on to the next test.

This block (from is(..., 2, "two tabs open") until the end of the function) is the same in every file (minus info("DONE DONE DONE")...).

Can you factor it out into head.js, too? nextTestOrFinish(aTestNumber, aTestWindow) or something? :-)

@@ +37,5 @@
> +    }
> +  });
> +}
> +
> +function test() {

In fact, so is this... which almost makes me wonder if you can factor this out into head.js, too. :-)

When you refactor this, can you please get rid of the param but add a gTestWindow global in head.js and stick the window on there? You can then also add a:

registerCleanupFunction(function() {
  gTestWindow && gTestWindow.close();
});

in head.js to ensure we always close the window even if the test fails halfway through or whatever.

::: browser/base/content/test/referrer/browser_referrer_open_link_in_window.js
@@ +39,5 @@
> +  });
> +}
> +
> +function test() {
> +  requestLongerTimeout(240);  // slowwww shutdown on e10s

err, this means 240 *times* the regular timeout. That's almost definitely wrong.

Do you have level 1 commit access so you can push to try?

::: browser/base/content/test/referrer/file_referrer_policyserver.sjs
@@ +25,5 @@
> +              ${metaReferrerTag}
> +              <title>Test referrer</title>
> +              </head>
> +              <body>
> +              <a id='testlink' href='${linkUrl}' ${rel ? ` rel='${rel}'` : ""}>

I'm amazed this works!

::: browser/base/content/test/referrer/head.js
@@ +111,5 @@
> +                                           handleResult);
> +      resolve(aMessage.data);
> +    });
> +    messageManager.loadFrameScript(
> +        REFERRER_FRAME_SCRIPT_URL + "referrer_frame_script.js", false);

This seems too trivial to need an extra file and all this setup. Instead of doing this, try using ContentTask:

return ContentTask.spawn(aWindow.gBrowser.selectedBrowser, {}, function() {
  return content.document.getElementById("testdiv").textContent;
});

should work, off the top of my head?

Comment 111

[:Gijs (out for now; he/him)]

Also, looking at the reasons for the bitrotting, are we intentionally not doing anything about "open frame in new tab" and friends?

Comment 112

[:Gijs (out for now; he/him)]

Attachment: Un-bitrotted browser patch (pass referrerPolicy from content process in e10s contextmenu case)

Comment 113

[:Gijs (out for now; he/him)]

Comment on attachment 8576322 [details] [diff] [review]
Un-bitrotted browser patch (pass referrerPolicy from content process in e10s contextmenu case)

Alex, can you doublecheck this? interdiff: 

https://bugzilla.mozilla.org/attachment.cgi?oldid=8574272&action=interdiff&newid=8576322&headers=1

I also changed doc.referrerPolicy to gContextMenuContentData.referrerPolicy in nsContextMenu.js (that's the only hunk for that file), but the interdiff tool is too dumb to figure that out because the context changed and so it can't apply both diffs.

Comment 114

[Alex Verstak]

Re test case coverage - this is the existing "channel test" I was referring to earlier:

http://lxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug704320.html?force=1

It covers the full cross product of {source-scheme, target-scheme, policy}.  But it triggers navigation from content, not from chrome.  So, it's more than just a unittest, it covers everything other than chrome event dispatch (which is this bug).

Given the existence of this other test, my 6-case / 10-minute strategy in the latest tests patch is to verify that (a) chrome gives the channel enough info to do its job, *not* that (b) the channel does its job for all possible inputs.  Either stance has reasonable philosophical arguments going for and against it.  It's probably not necessary to go there because I think we can resolve this on practical grounds; but I'd just like to point out that there's method and merit to my test strategy - it's the result of a different systematic approach, not of cutting corners.

The practical matter, of course, is how long these tests take to run, and how we expect this to grow over time.  Currently, a full cross-product would be 2 (source) * 2 (target) * 6 (policy|none) * 2 (rel=noreferrer) = 48 cases.  So, we're talking 10 minutes (mine) vs. 80 minutes (cross-product) of machine time.  Other cases that could reasonably be added in the future are the 6 CSP referrer values and the 3 referrer attribute values (I'm not sure of the implementation status, but both are in the spec, and bugs to implement them exist).  At that point, we're talking 18*48 = 864 cases = 15 hours.

Still think cross-product testing is a good idea?  If you do - fine, I'll fold my tent and implement the cross-product.  Just making sure you know what you're asking for, because you're going to get it. ;-)

(Re recent code review comments - thanks, will rebase and address them when I'm home later tonight.  Thanks for the blazingly fast reviews - this is really helpful.)

Comment 115

[Alex Verstak]

Comment on attachment 8576322 [details] [diff] [review]
Un-bitrotted browser patch (pass referrerPolicy from content process in e10s contextmenu case)

This doesn't quite work in e10s - open-in-new-tab needs another line in tabbrowser.xml to propagate the referrer policy; and open-in-new-window drops the referrer completely in all cases, AFAICT (by no fault of this patch - passing nsIURI to window.arguments is broken somehow, but a string works).  I'll send a counter-patch.

(BTW, my tests caught both of these bugs, and quickly.  Just saying... ;-)

Comment 116

[Alex Verstak]

Attachment: propagate referrer policy throughout the ui

Counter-patch - unbitrotted, plus a minor line in tabbrowser.xml, plus changed window.arguments to propagate a string instead of nsIURI - the latter gets lost somehow, but a string works.  Works in manual testing and passes automatic referrer tests.

Comment 117

[Alex Verstak]

Attachment: test cases - click, middle-click, new-{tab,window}

Addressed review feedback.  All pass both with and without e10s.

Pending issues with tests:

1. Coverage / strategy - see comment 114.

2. Leaks.  I get this intermittently with and w/o e10s, and am inclined to just ignore it:

TEST-INFO | leakcheck | default process: leaked 3 WeakReference<MessageListener> (48 bytes)
TEST-UNEXPECTED-FAIL | leakcheck | default process: 48 bytes leaked (WeakReference<MessageListener>)

3. Try run - sorry, I don't have committer access.

(The open-in-new-window test doesn't seem to be flaky anymore after the rebase - this was probably caused by the CPOW stuff that was fixed in bug 1133577.)

Comment 118

[:Gijs (out for now; he/him)]

(In reply to Alex Verstak from comment #115)
> Comment on attachment 8576322 [details] [diff] [review]
> Un-bitrotted browser patch (pass referrerPolicy from content process in e10s
> contextmenu case)
> 
> This doesn't quite work in e10s - open-in-new-tab needs another line in
> tabbrowser.xml to propagate the referrer policy; and open-in-new-window
> drops the referrer completely in all cases, AFAICT (by no fault of this
> patch - passing nsIURI to window.arguments is broken somehow, but a string
> works).  I'll send a counter-patch.
> 
> (BTW, my tests caught both of these bugs, and quickly.  Just saying... ;-)

FWIW, I did run the tests after I wrote the patch - but only in non-e10s mode (which was also broken before). D'oh. I'll look at the changes in more detail hopefully later today, but potentially it may have to be tomorrow as I'm a little overbooked this afternoon + evening.


(In reply to Alex Verstak from comment #114)
> Re test case coverage - this is the existing "channel test" I was referring
> to earlier:
> 
> http://lxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug704320.
> html?force=1
> 
> It covers the full cross product of {source-scheme, target-scheme, policy}. 
> But it triggers navigation from content, not from chrome.  So, it's more
> than just a unittest, it covers everything other than chrome event dispatch
> (which is this bug).

OK, great.

> Given the existence of this other test, my 6-case / 10-minute strategy in
> the latest tests patch is to verify that (a) chrome gives the channel enough
> info to do its job, *not* that (b) the channel does its job for all possible
> inputs.  Either stance has reasonable philosophical arguments going for and
> against it.  It's probably not necessary to go there because I think we can
> resolve this on practical grounds; but I'd just like to point out that
> there's method and merit to my test strategy - it's the result of a
> different systematic approach, not of cutting corners.

Right, sorry if I seemed to imply otherwise, that wasn't the intention.

> The practical matter, of course, is how long these tests take to run, and
> how we expect this to grow over time.  Currently, a full cross-product would
> be 2 (source) * 2 (target) * 6 (policy|none) * 2 (rel=noreferrer) = 48
> cases.  So, we're talking 10 minutes (mine) vs. 80 minutes (cross-product)
> of machine time.  Other cases that could reasonably be added in the future
> are the 6 CSP referrer values and the 3 referrer attribute values (I'm not
> sure of the implementation status, but both are in the spec, and bugs to
> implement them exist).  At that point, we're talking 18*48 = 864 cases = 15
> hours.

Right. That doesn't sound feasible.

> Still think cross-product testing is a good idea?  If you do - fine, I'll
> fold my tent and implement the cross-product.  Just making sure you know
> what you're asking for, because you're going to get it. ;-)
> 
> (Re recent code review comments - thanks, will rebase and address them when
> I'm home later tonight.  Thanks for the blazingly fast reviews - this is
> really helpful.)

Very welcome, and apologies in advance if this iteration will be slightly more tardy...


Do you have ideas about my note in comment 111 regarding "open frame in <whatever>" ? Followup bug? Do we even need to do something there?


(In reply to Alex Verstak from comment #117)
> Created attachment 8576569 [details] [diff] [review]
> test cases - click, middle-click, new-{tab,window}
> 
> Addressed review feedback.  All pass both with and without e10s.
> 
> Pending issues with tests:
> 
> 1. Coverage / strategy - see comment 114.
> 
> 2. Leaks.  I get this intermittently with and w/o e10s, and am inclined to
> just ignore it:
> 
> TEST-INFO | leakcheck | default process: leaked 3
> WeakReference<MessageListener> (48 bytes)
> TEST-UNEXPECTED-FAIL | leakcheck | default process: 48 bytes leaked
> (WeakReference<MessageListener>)

If it's intermittent it probably isn't anything new, but we can push this to try, which is always a new adventure...

> 3. Try run - sorry, I don't have committer access.

OK, then I shall push this for you in a bit.

Comment 119

[Alex Verstak]

(In reply to :Gijs Kruitbosch from comment #111)
> Also, looking at the reasons for the bitrotting, are we intentionally not
> doing anything about "open frame in new tab" and friends?

Yes. They reuse their own document.referrer, so there's no privacy / security issue.

(My goal is to help fix issues that reasonably block shipping of meta-referrer, so that I can get rid of plain http and silly redirectors on my website...)

Comment 120

[Alex Verstak]

(In reply to :Gijs Kruitbosch from comment #118)
> Do you have ideas about my note in comment 111 regarding "open frame in
> <whatever>" ? Followup bug? Do we even need to do something there?

I'm inclined to ignore this. The problem case is the following - an https source, with meta-referrer=origin, links to an http target. Then, the user does "open frame in" on the target. It would ignore the referrer policy of the source, i.e., use the default no-referrer-on-downgrade policy, so it would strip the referrer completely. But correct behavior would be to keep https://source.

I could file a followup bug, or I could just set the referrer policy here to unsafe-url w/o automatic tests, but somehow I just don't think this matters. It's reusing its own referrer, so it can only over-strip it, not under-strip it. Which means it's not a privacy / security issue, which means leaving it like this forever is probably just fine.

Thoughts?

Comment 121

[:Gijs (out for now; he/him)]

(In reply to Alex Verstak from comment #120)
> (In reply to :Gijs Kruitbosch from comment #118)
> > Do you have ideas about my note in comment 111 regarding "open frame in
> > <whatever>" ? Followup bug? Do we even need to do something there?
> 
> I'm inclined to ignore this. The problem case is the following - an https
> source, with meta-referrer=origin, links to an http target.

When you say "links to" you mean "has a frame with", right?

> Then, the user
> does "open frame in" on the target. It would ignore the referrer policy of
> the source, i.e., use the default no-referrer-on-downgrade policy, so it
> would strip the referrer completely. But correct behavior would be to keep
> https://source.
>
> I could file a followup bug, or I could just set the referrer policy here to
> unsafe-url w/o automatic tests, but somehow I just don't think this matters.
> It's reusing its own referrer, so it can only over-strip it, not under-strip
> it. Which means it's not a privacy / security issue, which means leaving it
> like this forever is probably just fine.

Ugh, human language. So confusing sometimes ("it's reusing its own referrer" - which? pre- or post-stripping?). AIUI, your argument is:

given page A containing a frame with page B:

i) page B was already opened with referrer of page A, subject to A's referrer policy, because it was opened in a frame of page A (right?)
ii) if we open page B in a new tab/window, the logical thing would be for the referrer to still be A, again subject to A's referrer policy
iii) for this case, we currently apply the default of no-referrer-on-downgrade, and the only case where that is problematic from a security/privacy perspective is when the referrer policy of page A allows passing (more of) A's URL to B then no-referrer-on-downgrade does.

You're saying that the opposite of (iii) (that is, "under-stripping" the referrer, so leaving too much of it where it really ought to be stripped down further to either origin or none) isn't possible. Is that really the case?

Let's say both A and B are https:, and A specifies referrer policy of "none". AIUI, B-in-a-frame gets no referrer info. However, right now, we pass all of A's URL to B when B is opened in a new tab/window, right? (AFAICT this is the case when A and B are both http:, too)

I'm assuming I'm missing something, but I'm not sure what. :-)

Even if I haven't missed anything, this can still be a followup because I think it's more of an edgecase than opening links - but *if* I'm right, we do need to do something.

Comment 122

[:Gijs (out for now; he/him)]

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

Comment 123

[:Gijs (out for now; he/him)]

Comment on attachment 8576558 [details] [diff] [review]
propagate referrer policy throughout the ui

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

::: browser/base/content/browser.js
@@ +1193,3 @@
>        else if (window.arguments.length >= 3) {
> +        let referrerURI = window.arguments[2];
> +        if (typeof(referrerURI) == 'string') {

Nit: double quotes

Comment 124

[:Gijs (out for now; he/him)]

Comment on attachment 8576569 [details] [diff] [review]
test cases - click, middle-click, new-{tab,window}

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

::: browser/base/content/test/referrer/browser_referrer_simple_click.js
@@ +5,5 @@
> +  info("browser_referrer_simple_click: " +
> +       getReferrerTestDescription(aTestNumber));
> +  BrowserTestUtils.browserLoaded(gTestWindow.gBrowser.selectedBrowser).then(
> +      function() {
> +    checkReferrerAndStartNextTest(aTestNumber, null, null,

Nit: is it possible to have less wonky indentation here? I've noticed that you stick to a conservative characters-per-line limit (not a criticism!). Personally, I'm not fussed until you go over something like 120, so I would just put the function() { on the previous line, but I appreciate that opinions may differ. :-)

If you would prefer not to, one other option would be passing:

checkReferrerAndStartNextTest.bind(null, aTestNumber, null, null, startSimpleClickTestCase)

as the callback, or using a temp var somewhere. My brain is just confused by a function body that is less indented than the start of the function. :-\

Comment 125

[Alex Verstak]

Comment on attachment 8573709 [details] [diff] [review]
dragdrop.patch

I don't think we need this - drag-n-drop hadn't been passing referrer before the meta-referrer changes, and it still doesn't. But feel free to file a followup if you wish.

Comment 126

[Alex Verstak]

Attachment: propagate referrer policy throughout the ui

Addressed style comment, and s/Ci/Components.interfaces/ in toolkit's browser.xml (per try run). Carrying r+ from Gijs.

Comment 127

[Alex Verstak]

Attachment: test cases - click, middle-click, new-{tab,window}

Addressed style comment. Carrying r+ from Gijs.

Could you please do another try run with the latest -browser and -tests patches?  The former contains a small fix that was causing failures all over.

Comment 128

[Alex Verstak]

Comment on attachment 8574270 [details] [diff] [review]
Document and nsIWebNavigation changes for referrer policy in UI code

@Sid - can you please review or reassign this?  sr=bz already but needs a regular review, I think.  Also see comment 63.

Comment 129

[Alex Verstak]

(In reply to Gijs Kruitbosch (unavailable until Friday March 13th) from comment #121)
> given page A containing a frame with page B:
> 
> i) page B was already opened with referrer of page A, subject to A's
> referrer policy, because it was opened in a frame of page A (right?)
> ii) if we open page B in a new tab/window, the logical thing would be for
> the referrer to still be A, again subject to A's referrer policy
> iii) for this case, we currently apply the default of
> no-referrer-on-downgrade, and the only case where that is problematic from a
> security/privacy perspective is when the referrer policy of page A allows
> passing (more of) A's URL to B then no-referrer-on-downgrade does.

Yep.

> You're saying that the opposite of (iii) (that is, "under-stripping" the
> referrer, so leaving too much of it where it really ought to be stripped
> down further to either origin or none) isn't possible. Is that really the
> case?

Yep.

> Let's say both A and B are https:, and A specifies referrer policy of
> "none". AIUI, B-in-a-frame gets no referrer info.

Yep.

> However, right now, we
> pass all of A's URL to B when B is opened in a new tab/window, right?

Nope.  We pass B.document.referrer, which is already empty; not A.document.documentURIObject, which is the full URL of A.

http://lxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#925
-- passes gContextMenuContentData.referrer.

http://lxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#161
-- sets gContextMenuContentData.referrer to event.target.ownerDocument.referrer (event is the contextmenu open event, so its target is in B, not in A - you need to right-click inside B to open B in a new frame).

> Even if I haven't missed anything, this can still be a followup because I
> think it's more of an edgecase than opening links - but *if* I'm right, we
> do need to do something.

Still want a followup?  Methinks not, but if you wish...

Comment 130

[:Gijs (out for now; he/him)]

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

Comment 131

[:Gijs (out for now; he/him)]

(In reply to Alex Verstak from comment #129)
> (In reply to Gijs Kruitbosch (unavailable until Friday March 13th) from
> comment #121)
> > Even if I haven't missed anything, this can still be a followup because I
> > think it's more of an edgecase than opening links - but *if* I'm right, we
> > do need to do something.
> 
> Still want a followup?  Methinks not, but if you wish...

Nope, thanks for clearing up my confusion!

Comment 132

[Alex Verstak]

(In reply to :Gijs Kruitbosch from comment #130)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0a991cfbd6e

All fine, except a race on linux opt e10s in the open-in-new-window test:

03:44:29 browser_referrer_open_link_in_window: policy=[undefined] rel=[undefined] http:// -> http://
03:44:30 must wait for load
03:44:30 must wait for focus
... logging from unrelated addons code ...
03:45:13 Longer timeout required, waiting longer...

It opened the window and loaded the target:

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/ec315ae00685fd5d73a946c9b18e6559f0ec290a5c833fd3c79dcdda043ddfd3dfbe7fba70317fbd90dbcc609597344369d0e282383970644088a55987783850

...but didn't call me back the way I expected.  Ah, life is like that.  One more version is coming right up...

Comment 133

[Alex Verstak]

Attachment: test cases - click, middle-click, new-{tab,window}

Exfoliate waitForFocus from the test - it appears to race with the new tab load in linux opt e10s.  Replace it with the "load" event when waiting for a new window to open.  Remove it from the other places too, we don't seem to need it.

Another try run?  (Sorry to keep bugging you...)

Comment 134

[:Gijs (out for now; he/him)]

Comment on attachment 8577368 [details] [diff] [review]
test cases - click, middle-click, new-{tab,window}

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

::: browser/base/content/test/referrer/head.js
@@ +145,5 @@
> +  return new Promise(function(resolve) {
> +    Services.obs.addObserver(function observer(aSubject, aTopic) {
> +      if (aWindow == aSubject) {
> +        Services.obs.removeObserver(observer, aTopic);
> +        executeSoon(resolve);

Why executeSoon? Why not just resolve immediately?

@@ +183,5 @@
> +        Services.wm.removeListener(this);
> +        var newWindow = aXULWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                  .getInterface(Ci.nsIDOMWindow);
> +        newWindow.addEventListener("load", function onLoad() {
> +          newWindow.removeEventListener("load", onLoad, false);

I am skeptical about your change to remove the focus handling here. I also just realized you could use promiseFocus() (or perhaps it needs to be SimpleTest.promiseFocus; not sure) here in the previous version. In any case, I'll push to try in a sec.

Also, instead of this load handler, perhaps use:

delayedStartupFinished(newWindow).then(resolve);

?

Comment 135

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #134)
> ::: browser/base/content/test/referrer/head.js
> @@ +183,5 @@
> > +        Services.wm.removeListener(this);
> > +        var newWindow = aXULWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                  .getInterface(Ci.nsIDOMWindow);
> > +        newWindow.addEventListener("load", function onLoad() {
> > +          newWindow.removeEventListener("load", onLoad, false);
> 
> I am skeptical about your change to remove the focus handling here.

Err, scratch that. We don't actually need focus on this window anyway. :-\

> I also
> just realized you could use promiseFocus() (or perhaps it needs to be
> SimpleTest.promiseFocus; not sure) here in the previous version. In any
> case, I'll push to try in a sec.

Well, I would have, but we're in the middle of a treeclosing window. Will try to do it afterwards (should be finished in 1 hour or so, if my timezone math is right), or tomorrow. If there isn't a link here monday, please yell at (needinfo) me.

Comment 136

[:Gijs (out for now; he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=762951d39c9c

(and sorry that this took a little longer than the previous reviews, yesterday and today were a bit manic)

Comment 137

[Alex Verstak]

Attachment: test cases - click, middle-click, new-{tab,window}

Addressed review comments - removed executeSoon, changed load to browser-delayed-startup-finished.  Snuck in one more test - open in new private window.  Fixed outdated comments and removed leftover debug logging.  Carrying r+ from Gijs.

Last try-run looked good - none of the orange was from the referrer tests.  Ship it?  Or another try run?  Your call.

Comment 138

[:Gijs (out for now; he/him)]

(In reply to Alex Verstak from comment #137)
> Created attachment 8577711 [details] [diff] [review]
> test cases - click, middle-click, new-{tab,window}
> 
> Addressed review comments - removed executeSoon, changed load to
> browser-delayed-startup-finished.  Snuck in one more test - open in new
> private window.  Fixed outdated comments and removed leftover debug logging.
> Carrying r+ from Gijs.
> 
> Last try-run looked good - none of the orange was from the referrer tests. 
> Ship it?  Or another try run?  Your call.

I'm happy to do another try run, but in any case, you still need review on the dom/docshell/embedding/netwerk changes before this can land, I think?

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=346747818e3e

Comment 139

[Alex Verstak]

(In reply to :Gijs Kruitbosch from comment #138)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=346747818e3e

Looks good, thanks!

> ...but in any case, you still need review on
> the dom/docshell/embedding/netwerk changes before this can land, I think?

Yes.  Now if only the person who asked me for help with this bug in the first place would kindly do this review that has been in his queue for over a week... :-/

Comment 140

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8574270 [details] [diff] [review]
Document and nsIWebNavigation changes for referrer policy in UI code

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

Ack!  I'm so sorry for the long delay, Alex.  :(  bz sr+'ed the interface bits, and that's most of what this patch does: update the interface call-sites.  The rest looks good to me, but unfortunately I'm not a DOM or DocShell peer so I can't technically r+ it.

jst: I would r+ this if I could.  Can you help?

::: toolkit/components/viewsource/content/viewPartialSource.js
@@ +177,5 @@
> +                                         "view-source:data:text/html;charset=utf-8," :
> +                                         "view-source:data:application/xml;charset=utf-8,")
> +                                        + encodeURIComponent(tmpNode.innerHTML),
> +                                        loadFlags,
> +                                        null, 0,  // referrer, referrerPolicy

Minor nit: can you use a more meaningful constant instead of 0 so it's clearer?  I'm thinking: Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT like in RemoteWebNavigation.jsm.

Comment 141

[Sid Stamm [:geekboy or :sstamm]]

Alex: please needinfo me again (and perhaps pester me via email too) if jst doesn't get back to us quickly and I'll try to find another peer to review.  Your code looks good, but want to make sure I didn't miss something module specific.

Comment 142

[Alex Verstak]

Attachment: Document and nsIWebNavigation changes for referrer policy in UI code

Addressed Sid's style comment; did a trivial rebase.  Carrying over r=jst sr=bz.

Comment 143

[Alex Verstak]

Attachment: propagate referrer policy throughout the ui

Trivial rebase.  Carrying over r+ from Gijs.

Comment 144

[Alex Verstak]

Attachment: test cases - click, middle-click, new-{tab,window}

Trivial rebase.  Carrying over r+ from Gijs.

Comment 145

[Alex Verstak]

Looks like all the ducks are in a row - time to land this on M/C.

Last try run:

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

(I'm not sure what the process is for landing this on 38 too; assuming this is a separate step, and that someone knowledgeable will chime in.  That would probably also require the patches from bug 1133577.)

Comment 146

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6a170ee249b2
https://hg.mozilla.org/integration/fx-team/rev/3019cfbbfd0d
https://hg.mozilla.org/integration/fx-team/rev/4b5850a205d0

Comment 147

[:Gavin Sharp [email: [e-mail]]]

(In reply to Alex Verstak from comment #145)
> (I'm not sure what the process is for landing this on 38 too; assuming this
> is a separate step, and that someone knowledgeable will chime in.  That
> would probably also require the patches from bug 1133577.)

Once the patch is merged to mozilla-central safely, the next step is to request approval‑mozilla‑aurora from the attachment details page (you can do it for just one of the three patches here). That will auto-fill in a little questionnaire to be filled out explaining the rationale, https://wiki.mozilla.org/Release_Management/Uplift_rules#Guidelines_on_approval_comments has some more context on that. Once that's done the process is pretty automatic from there. Gijs can help with the overall process as needed.

Comment 148

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a170ee249b2
https://hg.mozilla.org/mozilla-central/rev/3019cfbbfd0d
https://hg.mozilla.org/mozilla-central/rev/4b5850a205d0

Comment 149

[Sylvestre Ledru [:Sylvestre]]

Alex, could you fill the uplift request to aurora (38)? thanks

Comment 150

[Alex Verstak]

Comment on attachment 8579150 [details] [diff] [review]
propagate referrer policy throughout the ui

Approval Request Comment

[Feature/regressing bug #]:

1113431 (primary 3 patches)
1133577 (likely needed for the above to apply)
1144816 (focus patch fixes race condition in the test)

I'd also disable referrer tests on e10s - they're flaky on several platforms, for several different reasons (unrelated to these patches, AFAICT).

[User impact if declined]:

Meta referrer will get delayed yet again:

https://groups.google.com/forum/#!topic/mozilla.dev.platform/0elnyR7jZe8

This feature enables several privacy & security improvements.  Facebook, Yahoo, and Google want this, and have been using a similar feature in Chrome & Safari for years.  For Google Scholar specifically (I cannot speak for others), lack of meta referrer support stops me from moving Firefox to HTTPS / HSTS, which has obvious privacy and security implications.

[Describe test coverage new/current, TreeHerder]:

Primary bug includes an integration test.  TreeHerder has been happy for several days, except on e10s - see bug 1144816 and bug 1145199, both of which are test flakiness on e10s only, not bugs in the feature AFAICT.

[Risks and why]:

Medium - there's a fair amount of code, but it's all straightforward parameter propagation, and it's well covered by the automatic tests.  The underlying meta referrer implementation had been on MC and Aurora / Beta since last fall.  This set of patches merely propagates the referrer policy in a few more places in the UI.

These changes obviously touch click and context menu handling code in the UI.  But if I broke _that_ completely, these patches would be backed out by now. :-)

The real risk here is the privacy / security risk of propagating overly detailed referrer headers due to unknown implementation deficiencies.  I'm not aware of any logic errors in this set of patches, but, of course, I'm merely human.

[String/UUID change made/needed]:

One of the patches changes UUID for nsIWebNavigation, and renames one of the methods in it.  I updated all the calls on MC, and it was not in use by any of the addons.

Per comment 74, this breaks source compatibility with one of the addons, due to the addon's use of a private function in browser.js.  The author had been copied on the bug for some time.  No UUID change is involved.

Comment 151

[Ryan VanderMeulen [:RyanVM]]

Part 2 needs rebasing for Aurora.

Comment 152

[Alex Verstak]

Sure, will do in the evening (have a day job, sorry).

Comment 153

[Sylvestre Ledru [:Sylvestre]]

(In reply to Alex Verstak from comment #152)
> Sure, will do in the evening (have a day job, sorry).
No worries! Thanks for doing it.

Comment 154

[Alex Verstak]

Attachment: aurora-01-1133577-openLinkAndFramevFinal.patch

Aurora patch 1 of 9; applies cleanly.

Comment 155

[Alex Verstak]

Attachment: aurora-02-1133577-fixTestBrowser_bug734076v2.2.patch

Aurora patch 2 of 9; applies cleanly.

Comment 156

[Alex Verstak]

Attachment: aurora-03-1113431-api.patch

Aurora patch 3 of 9; applies cleanly.

Comment 157

[Alex Verstak]

Attachment: aurora-04-1113431-browser.patch

Aurora patch 4 of 9; minor rebase against an earlier rel=noreferrer patch, covered by the test in the upcoming part 5.

Comment 158

[Alex Verstak]

Attachment: aurora-05-1113431-tests.patch

Aurora patch 5 of 9; applies cleanly.

Comment 159

[Alex Verstak]

Attachment: aurora-06-1144816-timeout.patch

Aurora patch 6 of 9; applies cleanly.

Comment 160

[Alex Verstak]

Attachment: aurora-07-1144816-focus.patch

Aurora patch 7 of 9; applies cleanly.

Comment 161

[Alex Verstak]

Attachment: aurora-08-1144816-longer.patch

Aurora patch 8 of 9; applies cleanly, not checked in to MC yet but safe (just increases timeouts) and r=gijs already.

Comment 162

[Alex Verstak]

Attachment: aurora-09-1113431-skip-e10s.patch

Aurora patch 9 of 9.  Skip flaky e10s tests on win & mac due to bug 1145199 and bug 1144816.  The mac one could well be fixed by part 8, but we don't know this yet.  The win one is still open.  Both are e10s races unrelated to meta-referrer code, hence silencing them on Aurora.

Comment 163

[Alex Verstak]

This should do it for Aurora, though it obviously needs a try run (I don't have committer access to submit one).  Part 9 should be reviewed - flagging Gijs, though it's quite trivial.  All else is patches from MC with at worst a simple rebase.

Comment 164

[Alex Verstak]

Comment on attachment 8582269 [details] [diff] [review]
aurora-03-1113431-api.patch

(Obviously reviewed, just forgot the flag.)

Comment 165

[:Gijs (out for now; he/him)]

Comment on attachment 8582278 [details] [diff] [review]
aurora-09-1113431-skip-e10s.patch

We don't run e10s tests on aurora:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora

so this shouldn't be necessary.

Comment 166

[Alex Verstak]

Aurora patches are ready for checkin.  8 total; part 9 isn't needed, per comment 165.

Comment 167

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/de0c010c81a4
https://hg.mozilla.org/releases/mozilla-aurora/rev/5aaef73ac73e
https://hg.mozilla.org/releases/mozilla-aurora/rev/7670a26203b6

Comment 168

[Jean-Yves Perrier [:teoli]]

Morphing the ni/ in a dev-doc-needed

Comment 169

[Jean-Yves Perrier [:teoli]]

Added a note
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta#Browser_compatibility
and updated
https://developer.mozilla.org/en-US/Firefox/Releases/38#HTML