1409248 - Stop sending referrer details on text links

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[mdew]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171014215713

Steps to reproduce:

Original bug posted here: #1133201  It seems to be broken again (in Firefox nightly). Where Firefox will send referrer links on text url's.




Actual results:

1: Visit https://pastebin.com/QD6QHKn3
2: Select link, and right click "Open link in new tab"
3: Displays referrer details in new tab. (which it should not).


Expected results:

Doesn't send details to the container tabs, but it shouldn't send to regular tabs either.

(Example)
https://www.youtube.com/watch?v=E94LzgtwogA

Comment 1

[mdew]

https://bugzilla.mozilla.org/show_bug.cgi?id=1133201

Comment 2

[Mike Taylor [:miketaylr]]

Attachment: tc.html

mdew, using the attached test case I get EXPECTED RESULTS (no referer) in 57 and 58.

But trying to bisect with mozregression, I get a referer between 39 (when the fix supposedly landed) and 58 from the pastebin link.

Can you try to find a regression range? 

http://mozilla.github.io/mozregression/

Comment 3

[Mike Taylor [:miketaylr]]

(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #2)
> mdew, using the attached test case I get EXPECTED RESULTS (no referer) in 57
> and 58.

Uh, that was at least true when it was a local file. Being served from BMO, I see a referer. If you could help find a regression range that would be helpful. Otherwise, this maybe was never fixed.

Comment 4

[Mike Taylor [:miketaylr]]

Nathan, can you help verify your original fix worked? I'm confused if this is a real regression.

Comment 5

[Nathan Froyd [:froydnj]]

[Tracking Requested - why for this release]: privacy leak and regression.

(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #4)
> Nathan, can you help verify your original fix worked? I'm confused if this
> is a real regression.

The initial report is confusing; here's what's actually happening:

1. Bug 1133201 was reported: we were sending Referer headers when it was not appropriate to do so.
2. Said bug was fixed: we no longer sent Referer headers for plain text links.
3. Something *waves hands* broke the behavior, i.e. we send Referer headers for plain text links now.
4. ...leading to this bug report.

So yes, it would be a regression, but bug 1133201 is not responsible.

This would be tracking for 57, I think, but there's no way we would get it in now.  Maybe as a ride-along on a point release?

Comment 6

[Marcia Knous [:marcia]]

Tracking 58+ based on Comment 5.

Comment 7

[Mike Taylor [:miketaylr]]

Thanks Nathan. I'll see if I can get a regression range this time.

Comment 8

[Mike Taylor [:miketaylr]]

OK, not sure what I was doing wrong with mozregression before, but here we go:

2017-11-13T16:42:06: INFO : Narrowed nightly regression window from [2015-03-04, 2015-03-07] (3 days) to [2015-03-06, 2015-03-07] (1 days) (~0 steps left)


https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c5b90c003be8&tochange=43fb1f92e8d4

That's still a big window, but given the regression happened 2.5 years ago (and just a few weeks after the fix landed!), it's not worth tracking as a regression IMO. 

Dao, assuming this is the right component, could you give this a priority?

Comment 9

[Mike Taylor [:miketaylr]]

Marcia, since this is such an old regression, I don't think we should track 58 -- thoughts?

Comment 10

[Dão Gottwald [:dao]]

(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #8)
> OK, not sure what I was doing wrong with mozregression before, but here we
> go:
> 
> 2017-11-13T16:42:06: INFO : Narrowed nightly regression window from
> [2015-03-04, 2015-03-07] (3 days) to [2015-03-06, 2015-03-07] (1 days) (~0
> steps left)
> 
> 
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=c5b90c003be8&tochange=43fb1f92e8d4
> 
> That's still a big window,

Probably a regression from bug 1133577.

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

I believe Dão is correct.  In https://hg.mozilla.org/mozilla-central/rev/5532969bd5df I moved the BrowserUtils.linkHasNoReferrer() check from within _openLinkInParameters() to within setTarget(), but unfortunately also within a "if (elem.nodeType == Node.ELEMENT_NODE)" branch, where it still resides today, after some moving around:
https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/browser/modules/ContextMenu.jsm#956
though now also within a _setContextForNodesWithChildren() function.

Since all the plain text link logic is happening in the parent process in nsContextMenu.js anyway
https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/browser/base/content/nsContextMenu.js#330-339
I think the simplest and also best fix is to either explicitly set this.linkHasNoReferrer to true at the same time as this.onPlainTextLink, or make params.noReferrer in _openLinkInParameters() "this.linkHasNoReferrer || this.onPlainTextLink"
https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/browser/base/content/nsContextMenu.js#759

Dão do you have a preference? (I guess the second option is more "honest")

Comment 12

[Marcia Knous [:marcia]]

Updating the 58 tracking flag based on won't fix for 58.

Comment 13

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1409248 - Don't send a referrer if right-click opening a selected plain-text link.

Review commit: https://reviewboard.mozilla.org/r/199836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199836/

Comment 14

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8928604 [details]
Bug 1409248 - Don't send a referrer if right-click opening a selected plain-text link.

https://reviewboard.mozilla.org/r/199836/#review204934

Uploading this approach so it can be reviewed immediately if wanted, rather than going through a second flag cycle.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8928604 [details]
Bug 1409248 - Don't send a referrer if right-click opening a selected plain-text link.

https://reviewboard.mozilla.org/r/199836/#review207374

Comment 16

[Dão Gottwald [:dao]]

(In reply to Ian Moody [:Kwan] from comment #11)
> Since all the plain text link logic is happening in the parent process in
> nsContextMenu.js anyway
> https://searchfox.org/mozilla-central/rev/
> bab833ebeef6b2202e71f81d225b968283521fd6/browser/base/content/nsContextMenu.
> js#330-339
> I think the simplest and also best fix is to either explicitly set
> this.linkHasNoReferrer to true at the same time as this.onPlainTextLink, or
> make params.noReferrer in _openLinkInParameters() "this.linkHasNoReferrer ||
> this.onPlainTextLink"
> https://searchfox.org/mozilla-central/rev/
> bab833ebeef6b2202e71f81d225b968283521fd6/browser/base/content/nsContextMenu.
> js#759
> 
> Dão do you have a preference? (I guess the second option is more "honest")

Either way works for me. The patch looks fine as-is. Thanks!

Comment 17

[Ian Moody [:Kwan] (UTC+1)]

Thanks Dão!

This got a (mostly) ride-along try, which is (shockingly) 100% green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1323a57b8bbf84763ee1141893abf52bbf5a5d45

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02ac37bf54ba
Don't send a referrer if right-click opening a selected plain-text link. r=dao

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/02ac37bf54ba