Closed Bug 326107 Opened 15 years ago Closed 15 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.8.1

People

(Reporter: brettw, Assigned: brettw)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

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.
Priority: -- → P4
Target Milestone: --- → Firefox 2 beta1
Depends on: 325665
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.
Component: Places → Embedding: Docshell
Priority: P4 → P3
Product: Firefox → Core
Summary: Session tracking is broken for some types of redirects → Send referrer to GlobalHistory even when it isn't being sent over the network
Target Milestone: Firefox 2 beta1 → mozilla1.8.1
Attached patch Patch (obsolete) — Splinter Review
This tries to get the internal referrer from the channel's property bag before calling AddToGlobalHistory.
Attachment #210920 - Flags: superreview?(bzbarsky)
Attachment #210920 - Flags: branch-1.8.1?(darin)
Blocks: 325665
No longer depends on: 325665
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.
Attachment #210920 - Flags: superreview?(bzbarsky) → superreview+
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.
Attached patch New patchSplinter Review
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.
Attachment #210920 - Attachment is obsolete: true
Attachment #210936 - Flags: branch-1.8.1?(darin)
Attachment #210920 - Flags: branch-1.8.1?(darin)
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.
yes, that's what I was trying to say in comment 6.
Attached patch I get it nowSplinter Review
Attachment #210944 - Flags: branch-1.8.1?(darin)
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
Attachment #210944 - Flags: review+
Attachment #210944 - Flags: branch-1.8.1?(darin)
Attachment #210944 - Flags: branch-1.8.1+
Attachment #210936 - Flags: branch-1.8.1?(darin)
On 1.8 branch and trunk.
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Does this also work when network.http.sendRefererHeader is 0?
You need to log in before you can comment on or make changes to this bug.