Closed
Bug 326107
Opened 19 years ago
Closed 19 years ago
Send referrer to GlobalHistory even when it isn't being sent over the network
Categories
(Core :: DOM: Navigation, defect, P3)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla1.8.1
People
(Reporter: brettw, Assigned: brettw)
References
Details
(Keywords: fixed1.8.1)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
darin.moz
:
review+
darin.moz
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•19 years ago
|
Priority: -- → P4
Target Milestone: --- → Firefox 2 beta1
Assignee | ||
Comment 1•19 years ago
|
||
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
Assignee | ||
Comment 2•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Comment 3•19 years ago
|
||
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+
Comment 4•19 years ago
|
||
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.
Assignee | ||
Comment 5•19 years ago
|
||
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)
Comment 6•19 years ago
|
||
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.
Assignee | ||
Comment 8•19 years ago
|
||
Attachment #210944 -
Flags: branch-1.8.1?(darin)
Comment 9•19 years ago
|
||
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+
Updated•19 years ago
|
Attachment #210936 -
Flags: branch-1.8.1?(darin)
Assignee | ||
Comment 10•19 years ago
|
||
On 1.8 branch and trunk.
Comment 11•19 years ago
|
||
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.
Description
•