326107 - Send referrer to GlobalHistory even when it isn't being sent over the network

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Brett Wilson]

For example, when you log into Gmail, you get a redirect page (www.google.com/accounts/CheckCookie?...) and then the real mail reading page mail.google.com/mail/?auth=... This final page has an empty referrer for some reason, making it marked as a new session.

Comment 1

[Brett Wilson]

The referrer isn't being sent to GlobalHistory for https->http transitions. This information is stored in a property "docshell.internalReferrer" on the channel. There aren't any security considerations since this is your local history, and it will enable session tracking in the new Places history implmenetation to work correctly.

Comment 2

[Brett Wilson]

Attachment: Patch

This tries to get the internal referrer from the channel's property bag before calling AddToGlobalHistory.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 210920 [details] [diff] [review]
Patch

>Index: docshell/base/nsDocShell.cpp
>+                // see if there's a true referrer stored as a property
>+                nsCOMPtr<nsIWritablePropertyBag2> props(

Why writable?  You should only need nsIPropertyBag2 here.

>+                    props->GetPropertyAsInterface(NS_LITERAL_STRING(INTERNAL_REFERRER_PROPERTY),

This compiled?  It sure didn't use to, and given how NS_LITERAL_STRING is defined, I wouldn't expect it to, since NS_LITERAL_STRING(s) expands to L##s, and the ## should be processed before attempted further macro expansion, no?

I'm pretty sure that even if this happened to work with one version of one C preprocessor it doesn't work in general...

>@@ -7337,8 +7353,19 @@

Could I prevail on you to fix the existing code duplication by adding an aChannel argument to AddToGlobalHistory?  Only these two callsites would be affected, and they do exactly the same thing...

Note that you do want to keep passing in the URI, since the two callers don't _quite_ treat the URI identically... or at least not obviously.

Also, if you can add the "-p -8" options to whatever your diff setup is, that would be wonderful.  :)

sr=bzbarsky with those issues fixed.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Why do this only if GetReferrer fails? It seems like you could even remove the nsIHttpChannel thing entirely and make this work for other protocols as well.

Comment 5

[Brett Wilson]

Attachment: New patch

This fixes boris' comments. Sorry about the -p8, I usually remember...

This also addressed Christian's comments. If there is no referrer from the httpchannel or no httpchannel, I always check the interalReferrer.

Comment 6

[Darin Fisher]

Brett, I think you can get away with removing the nsIHttpChannel::GetReferrer call altogether.  If the channel has a "referrer" then it will definitely have a "docshell.internalReferrer", so there is no need to bother calling GetReferrer.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

yes, that's what I was trying to say in comment 6.

Comment 8

[Brett Wilson]

Attachment: I get it now

Comment 9

[Darin Fisher]

Comment on attachment 210944 [details] [diff] [review]
I get it now

>Index: docshell/base/nsDocShell.cpp

>+    // Get referrer from the channel. We have to check for a property on a
>+    // property bag because the referrer is empty for security reasons (for
>+    // example, when loading a http page with a https referrer).

nit: s/is empty/may be empty/

r+a=darin

Comment 10

[Brett Wilson]

On 1.8 branch and trunk.

Comment 11

[Ian Macfarlane]

Does this also work when network.http.sendRefererHeader is 0?