Open Bug 968273 Opened 7 years ago Updated 7 months ago

Entered URL in firefox, Firefox remember old redirect and i get the wrong page

Categories

(Core :: Networking: Cache, defect, P3)

26 Branch
x86_64
Windows 7
defect

Tracking

()

REOPENED

People

(Reporter: zero_enna_cfg, Assigned: dragana)

References

Details

(Whiteboard: [bugday-20140212][necko-active])

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

1. Use XBMC default Web interface
2. Change to XWMM Web Interface (access to http://XBMC is redirected to http://XBMC/movies/index.html)
3. Change back to XBMC default

Addendum:
The same result with Xibo, i get a redirect to the installer - the Xibo server run fine


Actual results:

FF still redirect me to the non existing 
http://XBMC/movies/index.html
IE Don't do this, FF Portable ditto

When i enter 
XIBO/index.html
i get the right page


Expected results:

When i Enter
XIBO (hostname)
in my browser, FF read the index and redirect when the there is the a redirect. When there is no redirect -> no redirect

I see no reason for a permanent redirect storing
[bugday-20140210]
The Sites http://XBMC and http://XBMC/movies/index.html are no longer available.
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0

I confirm that I have had the same problem for some time, for several different addresses and sites, also in earlier versions of FF.

I think that FF is caching redirect HTTP (30x) responses and the problem is there is no way to refresh this cache (Shift+F5 does not work as the original URL is immediately changed to the redirected one and you refresh it).

When this happens to me Live HTTP Headers plugin does not show any HTTP requests being sent and received so it suggests thats it is the cache.

Another plugin, CacheViewer, does in fact show, that this particular HTTP redirect response (301) has been cached:

(Sorry for the Polish fragments below, it is my default locale)

--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8

Klucz: http://gd.supermemo.net/
Rozmiar: 11438 bytes
Liczba pobrań: 24
Ostatnio zmodyfikowany: 10 lutego 2014 16:44:43
Ostatnio pobrany: 12 lutego 2014 10:41:57
Data wygaśnięcia: Brak daty
Plik na dysku: C:\Users\Grzegorz\AppData\Local\Mozilla\Firefox\Profiles\od2012.default\Cache\2\51\A08CFd01

request-method: GET
response-head: HTTP/1.1 301 Moved Permanently
Date: Mon, 10 Feb 2014 15:44:43 GMT
Server: Apache/2.2.22 (Win64) PHP/5.3.13
Location: http://gd.supermemo.net/cake/
Content-Length: 237
Content-Type: text/html; charset=iso-8859-1

charset: UTF-8

--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8
I meant Ctrl+F5 above, of course.
Component: Untriaged → Networking: Cache
Product: Firefox → Core
Whiteboard: [bugday-20140212]
Clearing the web cache is a help. But i still miss a real solution. 


This is a new behavior in FF. My last set up (same XIBO Server Files) some months ago don't get this result.
You may try with turning on the warning before redirect, in the Prefs → Advanced → General → Accessibility. But this pref does not work in all cases.
this is a dup of something I can't find..
Whiteboard: [bugday-20140212] → [bugday-20140212][necko-active]
Assignee: nobody → dd.mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch bug_968273_v1.patch (obsolete) — Splinter Review
Attachment #8755305 - Flags: review?(honzab.moz)
I figured out how the global history work, but I am not sure we should do anything there.

Redirects are stored in the history but they are hidden. We could unhide them (option 1).

I would not do the same think as with the session history: if we do the same as with the session history for example for a url made with tinyurl.com only tinyurl.com/**** url will be shown in the history but not the redirected one. i think the other is more useful. We could show them both (option 1)

So i am for not doing anything or doing option 1. Patrick, Honza?
Flags: needinfo?(mcmanus)
Flags: needinfo?(honzab.moz)
(In reply to Dragana Damjanovic [:dragana] from comment #9)
> I would not do the same think as with the session history: if we do the same
> as with the session history for example for a url made with tinyurl.com only
> tinyurl.com/**** url will be shown in the history but not the redirected
> one. i think the other is more useful. We could show them both (option 1)

Will the back button work if we show them both? Back button would try to back to tinyurl.com/****, then it will redirect us again.
Also, window.history will expose the session history to content. Is this web-compatible?
(In reply to Masatoshi Kimura [:emk] from comment #10)
> (In reply to Dragana Damjanovic [:dragana] from comment #9)
> > I would not do the same think as with the session history: if we do the same
> > as with the session history for example for a url made with tinyurl.com only
> > tinyurl.com/**** url will be shown in the history but not the redirected
> > one. i think the other is more useful. We could show them both (option 1)
> 
> Will the back button work if we show them both? Back button would try to
> back to tinyurl.com/****, then it will redirect us again.

Back button work as it worked till now. Back button load pages from bfcache and is not going to network or our main cache. If you hit reload on a page it will reload starting from tinyurl.com/*** and do redirect again - this is a part that I have changed in this patch.

I have not change global history and if we change the global history behavior it will not have effect on back button it will only list 2 entries instead of one (tinyurl.com/*** and the redirected one). Although, I have not thoroughly tested that one and I have not changed it in this patch.
I don't know that I'm going to be of much help here - I'm a punter at docshell too.

Are you doubting that we want a functionality change? I still strongly believe in the goal of this bug.
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #13)
> I don't know that I'm going to be of much help here - I'm a punter at
> docshell too.
> 
> Are you doubting that we want a functionality change? I still strongly
> believe in the goal of this bug.

No, I do not doubt. The attached patch fix this for session history. if you go to a url and that url redirects somewhere else and if you hit reload it is going to start from url you entered and do redirect again. The behavior is kept if you go back and forward and also if you close a tab or browser and restore the tab from session history.

The redirects are not kept on the history list and I think we should not fix that one. In example above tinyurl.com/*** will never show in history  list only the url it redirects to and loading that page will not start from tinyurl.com/*** only from url it redirected to. Probably there are some example where it would be better to show the original one or list only redirected one but on load start from the original one and do redirect again (maybe temporal redirects) but I am not sure about that.

I will needinfo Boris what he thinks about keeping redirect information in the global history and if a page is loaded from history it starts the load from the original url and not redirected one.
Flags: needinfo?(bzbarsky)
I'm a bit lost as to what problem we're trying to solve.  The original bug description just comes about because 3xx responses are cacheable, right?  Are we trying to change that, or to make "reload" on a redirect result reload the origin (pre-redirect) URI in a "skip the cache" mode, or something else?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #15)
> I'm a bit lost as to what problem we're trying to solve.  The original bug
> description just comes about because 3xx responses are cacheable, right? 
> Are we trying to change that, or to make "reload" on a redirect result
> reload the origin (pre-redirect) URI in a "skip the cache" mode, or
> something else?

make "reload" on a redirect result reload the origin (pre-redirect) URI in a "skip the cache" mode

We have a problem when captive portal redirect e.g. from cnn.com to its page we can never go back to cnn.com. Reload will always reload captive portal page.
OK, that seems reasonable.

For global history.... I'm not sure.  It seems weird in cases like "foo.com" => "www.foo.com" redirects to remember the "foo.com" and keep putting people through the redirect.
(In reply to Boris Zbarsky [:bz] from comment #17)
> OK, that seems reasonable.
> 
> For global history.... I'm not sure.  It seems weird in cases like "foo.com"
> => "www.foo.com" redirects to remember the "foo.com" and keep putting people
> through the redirect.

That was my thinking as well, therefore I have not change anything in global history.
(In reply to Dragana Damjanovic [:dragana] from comment #9)
> I figured out how the global history work, but I am not sure we should do
> anything there.
> 
> Redirects are stored in the history but they are hidden. We could unhide
> them (option 1).
> 
> I would not do the same think as with the session history: if we do the same
> as with the session history for example for a url made with tinyurl.com only
> tinyurl.com/**** url will be shown in the history but not the redirected
> one. i think the other is more useful. We could show them both (option 1)
> 
> So i am for not doing anything or doing option 1. Patrick, Honza?

Do you still need any info from me?  I think Boris has answered with more insight here than me.  I personally would not change global history.  The thing we want to do here is to be able to reload the original page that has redirected, nothing else.

I'll look at the patch.
Flags: needinfo?(honzab.moz)
Can you please describe what the patch does?
Flags: needinfo?(dd.mozilla)
(In reply to Honza Bambas (:mayhemer) from comment #20)
> Can you please describe what the patch does?

only thing that it does is: it will load originalURI and not currentURI. so if there was no redirect it will load currentURI if there was a redirect it will load original uri which will probably redirect again. This is what we want e.g. if we go to cnn.com and captive portal redirects us to its page on reload we want to reload starting from cnn.com. This i relevant only for reloads. 

i have also removed loadReplace because it is not needed anymore.
Flags: needinfo?(dd.mozilla)
Comment on attachment 8755305 [details] [diff] [review]
bug_968273_v1.patch

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

I'm personally not sure we should rush on removing loadReplace here, this is something that bz has to r+ too (not accepting review now, tho).

Make sure you don't break this changeset functionality: https://hg.mozilla.org/mozilla-central/diff/7829b99f0d59/docshell/base/nsDocShell.cpp

LGTM otherwise.

::: docshell/base/nsDocShell.cpp
@@ +10952,3 @@
>          nsCOMPtr<nsILoadInfo> loadInfo = httpChannel->GetLoadInfo();
>          if (loadInfo) {
>            loadInfo->SetVerifySignedContent(true);

please let :fkiefer overlook this change.

::: docshell/test/test_bug968273.html
@@ +1,2 @@
> +<!DOCTYPE HTML>
> +<html>

please add some comment on what exactly the test does so that people don't have to study the code
Attachment #8755305 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #22)
> Comment on attachment 8755305 [details] [diff] [review]
> bug_968273_v1.patch
> 
> Review of attachment 8755305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm personally not sure we should rush on removing loadReplace here, this is
> something that bz has to r+ too (not accepting review now, tho).
> 

I have added loadReplace when I was adding OriginalURI it was need, I think we do not need it for now.

> Make sure you don't break this changeset functionality:
> https://hg.mozilla.org/mozilla-central/diff/7829b99f0d59/docshell/base/
> nsDocShell.cpp
> 
> LGTM otherwise.
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +10952,3 @@
> >          nsCOMPtr<nsILoadInfo> loadInfo = httpChannel->GetLoadInfo();
> >          if (loadInfo) {
> >            loadInfo->SetVerifySignedContent(true);
> 
> please let :fkiefer overlook this change.
> 
> ::: docshell/test/test_bug968273.html
> @@ +1,2 @@
> > +<!DOCTYPE HTML>
> > +<html>
> 
> please add some comment on what exactly the test does so that people don't
> have to study the code
Comment on attachment 8755305 [details] [diff] [review]
bug_968273_v1.patch

Can you please look at aboutNewTab change. Thanks.
Attachment #8755305 - Flags: review?(franziskuskiefer)
bz, you are not accepting reviews now, bug I will needinfo you to look through this when you have time. Sorry, there are not so many people that know this code.
Flags: needinfo?(bzbarsky)
>+   *                          up  actually loading date from network or cache,

Nix the extra space, and s/date/data/.  Looks reasonable.
Flags: needinfo?(bzbarsky)
Comment on attachment 8755305 [details] [diff] [review]
bug_968273_v1.patch

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

r+ for the newtab related changes in DocShell.

::: docshell/base/nsDocShell.cpp
@@ +10947,5 @@
>        httpChannel->SetReferrerWithPolicy(aReferrerURI, aReferrerPolicy);
>      }
>      // set Content-Signature enforcing bit if aOriginalURI == about:newtab
> +    if (httpChannel) {
> +      if (IsAboutNewtab(aURI)) {

If aURI is always the originalURI (which should be the case here), that's fine with me.
Attachment #8755305 - Flags: review?(franziskuskiefer) → review+
Attached patch bug_968273_v1.patch (obsolete) — Splinter Review
Attachment #8755305 - Attachment is obsolete: true
Attachment #8758192 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f50f2c7e813
On reload load from the original uri, so that all redirects are reloader.r=mayhemer
Keywords: checkin-needed
The problem was the test not what it was testing. I have change test to be in stile of other tests in this directory.

I used performance.navigation.redirectCount  to test that redirect happened on reload, but all performance.navigation tests are disabled on andoid, so I disabled this one too. Funny on a reload test succeeded but on original load test failed (I have not change any behavior on original load). I think this is a timing issue with performance.navigation.
Attachment #8758192 - Attachment is obsolete: true
Attachment #8760165 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00312b136937
On reload load from the original uri, so that all redirects are reloader.r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/00312b136937
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1279050
This change has caused a regression when visiting some sites like proxies (Bug 1279955). It won't allow you to go back one page but takes you to the first page of the proxy. In order to reproduce the bug "browser.sessionhistory.max_total_viewers" must be set to 0.
Depends on: 1279955
Depends on: 1282353
So what do other browsers do in case of history.go(0) or location.reload()?
Note, any change to history API handling tends to be super regression risky.
Based on initial testing looks like we end up doing now something else what blink for example does, and what gecko has done for ages when location.reload() is used.
Why do we think the change is safe, web compatible and why it is a good change? This makes location.reload() to be slower in redirect case (assuming my testing is right).
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #43)
> Note, any change to history API handling tends to be super regression risky.

I already warned it in comment #11 *before* landing, but Dragana didn't take it seriously.
Moreover, bug 1282353 does not even has a fix yet.
I strongly recommend to back out this immediately.
(In reply to Masatoshi Kimura [:emk] from comment #45)
> (In reply to Olli Pettay [:smaug] (high review load, please consider other
> reviewers) from comment #43)
> > Note, any change to history API handling tends to be super regression risky.
> 
> I already warned it in comment #11 *before* landing, but Dragana didn't take
> it seriously.
> Moreover, bug 1282353 does not even has a fix yet.
> I strongly recommend to back out this immediately.

Comment 11 was on my question, but I have never made that change. 

window.history is not exposing the session history to content.
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #44)
> Based on initial testing looks like we end up doing now something else what
> blink for example does, and what gecko has done for ages when
> location.reload() is used.
> Why do we think the change is safe, web compatible and why it is a good
> change? This makes location.reload() to be slower in redirect case (assuming
> my testing is right).

We have a problem with for example captive portals, they are really annoying. A captive portal is going to redirect you to its page and there is no way to get to the original page again. Other browsers have a solution for this. 

I am going to back out this change.
(In reply to Dragana Damjanovic [:dragana] from comment #47)> 
> We have a problem with for example captive portals, they are really
> annoying. A captive portal is going to redirect you to its page and there is
> no way to get to the original page again. Other browsers have a solution for
> this. 
Sure, but shouldn't the solution be a UI thing, not change to the web phasing API?
We can do whatever we like with back/forward handling in UI, but changing how window.history or window.location works is rather risky.
Or are you saying other browsers have different window.history behavior?
(it is well known issue that what HTML spec defines for .history API is largely bogus,
see for example https://github.com/whatwg/html/issues/1454)
(In reply to Dragana Damjanovic [:dragana] from comment #47)
> We have a problem with for example captive portals, they are really
> annoying. A captive portal is going to redirect you to its page and there is
> no way to get to the original page again. Other browsers have a solution for
> this. 

Are captive portals using permanent redirects? Even if so, we should handle captive portals without mucking around necko.

https://tools.ietf.org/html/rfc7231#section-6.4.2
>   The 301 (Moved Permanently) status code indicates that the target
>   resource has been assigned a new permanent URI and any future
>   references to this resource ought to use one of the enclosed URIs.

This change is doing the exact opposite and defeats the purpose of permanent redirects. Did you consider the case where the first request is, for example, a credit-card purchase transaction?
(In reply to Masatoshi Kimura [:emk] from comment #49)
> (In reply to Dragana Damjanovic [:dragana] from comment #47)
> > We have a problem with for example captive portals, they are really
> > annoying. A captive portal is going to redirect you to its page and there is
> > no way to get to the original page again. Other browsers have a solution for
> > this. 
> 
> Are captive portals using permanent redirects? Even if so, we should handle
> captive portals without mucking around necko.
> 


They use anything and everything, really. They do not have any rules
(In reply to Carsten Book [:Tomcat] from comment #41)
> https://hg.mozilla.org/mozilla-central/rev/00312b136937

May I ask you to back out this change. Thank you!
Flags: needinfo?(cbook)
(In reply to Dragana Damjanovic [:dragana] from comment #51)
> (In reply to Carsten Book [:Tomcat] from comment #41)
> > https://hg.mozilla.org/mozilla-central/rev/00312b136937
> 
> May I ask you to back out this change. Thank you!

sure anytime, backed out in https://hg.mozilla.org/mozilla-central/rev/d87b76177b2f
Status: RESOLVED → REOPENED
Flags: needinfo?(cbook)
Resolution: FIXED → ---
Target Milestone: mozilla50 → ---
Duplicate of this bug: 1274691
I don't understand the backed out patch enough to comment on whether or not its the right approach - we might indeed need a completely different one. I'm going to try and clarify the use case instead:

1] user navigates to http://a and receives a redirect to http://b.. http://b is rendered and perceived as "not right"
2] some time passes.. perhaps the location has changed
3] user clicks reload and receives http://b - they are captured.

or

1] user navigates to http://a and receives a redirect that goes into the cache to http://b - b is rendered and perceived as "not right"
2] some time passes as user finds a usable network
3] user restarts firefox and loads http://a from a safe network. The are redirected to http:// b from the cache - b is rendered
4] reload. Still http://b

This is essentially a security bug at this point - unauthenticated http allowed the cache poisoning but the usual way to fix corruption is via reload and that's not working even when you are in a safe environment. That kind of capture is a problem and I don't think expecting people to find the clear cache button is a reasonable expectation - when people are clicking reload its a reasonable inference to think corruption is a possible trigger for that.. in terms of efficiency, that certainly adds a round trip, but as we're going to revalidate every subresource on the page as well, its not especially significant in the context of that particular action.
I have check chrome again. and it is not refreshing all redirects, just the last one, the same as we.

Olli, can you look at comment 54 and suggest a solution if this patch (with the patch from bug 1279050) is not the right direction. Maybe better question is should we fix this? Maybe do reload from the original uri only for hard refresh?
Flags: needinfo?(bugs)
(Trying to page this issue into mind... I may say something silly.)

So window.history handling needs to stay the same. Doesn't the patch make window.history.length to increase when doing window.location.reload() or window.history.go(0)?

If we want to prevent the cache usage, shouldn't we add some flag to channel, or possibly to loadinfo that in case of redirects we skip loading from cache?


(In reply to Dragana Damjanovic [:dragana] from comment #12)
> > Will the back button work if we show them both? Back button would try to
> > back to tinyurl.com/****, then it will redirect us again.
> 
> Back button work as it worked till now. Back button load pages from bfcache
> and is not going to network or our main cache.
Only if previous page went into bfcache. But lots of pages don't go there, like any page
with unload event listener.
Flags: needinfo?(bugs)
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Moving to p3 because no activity for at least 24 weeks.
Priority: P1 → P3
Duplicate of this bug: 1405307

Let's have a conversation in Q2 about next steps for fixing this. Probably related to network-id ideas.

Could we simply not cache redirects? I wonder if that would be perceived as a perf regression.

As I understand, we land at a 40X/5XX response? We could throw away the whole redirect chain from the cache when that happens, so on next navigation to the first (bookmarked) URI in the chain we re-request it. For error pages this may be a valid thing to do.

This is always hard... if the server persists the redirect (301 Moved Permanently) then we respect that. If it changes, it goes against the 'permanency' of the proclaimed redirect.

:mayhemer, in theory a "permanent" redirect should be permanent, and its permanency should be respected. But we also have the issue of domain squatting; if a squatter sets up a 301 to redirect to their mega spam site, and then a legitimate user of the domain gets control over the domain, any Firefox browser which had attempted to access the domain in the past will not "see" the legitimate site. Having the cached 301 verify the IP address of the A record was my first thought, but in the case of load balancers, reverse proxies, virtual hosts, and content delivery networks, this would break.

It is probably useful to still respect 301 as permanent for the rare use case where it is really meant as a long-term, this-url-will-stop-working-down-the-road kind of thing, but the ability to

  • selectively remove permanent redirects from the cache (so users don't have to experience slow load times for all the sites which get flushed from cache, or lose stored permanent redirects for other sites which may no longer be accessible via their pre-redirect URLs),
  • have a built-in browser security tool which would say something like "Firefox has a cached redirect for this site to a different URL, but a new site may be available at the old (non-redirected) URL; would you like to Remove the cached redirect, Keep the cached redirect, Update any bookmarks which use the old URL and then remove the cached redirect, or Compare the two sites side-by-side before making a decision?",
  • and have an intuitive force-reload behavior (if any network request between the beginning of URL navigation and rendering of the completed page hits the cache, it gets logged in a "cache loading history," and a force-reload will override the cache for everything in the cache loading history before reverting the URL bar to the URL which began the URL navigation, and then restart URL navigation from the address in the URL bar

I also experienced something odd; I had a HTTP/301 set up for domain name of mine, which I had never expected to need to change... and then one day I needed to. The fact that it was using a cached HTTP/301 was not obvious (it's easy enough to find by hitting F12, but do regular users know that?), and the inability to clear it in isolation (I wish CacheViewer was still around!) is unfortunate.

From a scientific perspective, the inability to clear a single cache entry is not ideal, because it means I am potentially changing multiple variables at the same time between observations: e.g., what if the page has a dependency which also has a cached HTTP/301? If I can only remove the top-level HTTP/301 by clearing my entire cache, I might miss the fact that some dependency also has a cached HTTP/301 -- this is something which I would like to be aware of, as it could affect users of my site.

Oops, I said I experienced something odd without saying what the odd thing was (I don't see a way to edit my comment). I had notice some inconsistent behavior with favicons when accessing /test.php. / on my site used to have a HTTP/301 to another site entirely, with its own favicon, and it seemed like Firefox just simply stopped showing any favicon entirely for a while, but then started showing my site's real favicon when accessing /test.php eventually. But that's just a favicon, not a usability concern... :-)

You need to log in before you can comment on or make changes to this bug.