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

REOPENED
Assigned to

Status

()

defect
P3
normal
REOPENED
5 years ago
12 days ago

People

(Reporter: zero_enna_cfg, Assigned: dragana)

Tracking

26 Branch
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

Comment 1

5 years ago
[bugday-20140210]
The Sites http://XBMC and http://XBMC/movies/index.html are no longer available.

Comment 2

5 years ago
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

Comment 3

5 years ago
I meant Ctrl+F5 above, of course.

Updated

5 years ago
Component: Untriaged → Networking: Cache
Product: Firefox → Core
Whiteboard: [bugday-20140212]
(Reporter)

Comment 4

5 years ago
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
(Assignee)

Updated

3 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 7

3 years ago
Posted patch bug_968273_v1.patch (obsolete) — Splinter Review
Attachment #8755305 - Flags: review?(honzab.moz)
(Assignee)

Comment 9

3 years ago
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?
(Assignee)

Comment 12

3 years ago
(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)
(Assignee)

Comment 14

3 years ago
(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)
(Assignee)

Comment 16

3 years ago
(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.
(Assignee)

Comment 18

3 years ago
(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)
(Assignee)

Comment 21

3 years ago
(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+
(Assignee)

Comment 23

3 years ago
(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
(Assignee)

Comment 24

3 years ago
Comment on attachment 8755305 [details] [diff] [review]
bug_968273_v1.patch

Can you please look at aboutNewTab change. Thanks.
Attachment #8755305 - Flags: review?(franziskuskiefer)
(Assignee)

Comment 25

3 years ago
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+
(Assignee)

Comment 28

3 years ago
Posted patch bug_968273_v1.patch (obsolete) — Splinter Review
Attachment #8755305 - Attachment is obsolete: true
Attachment #8758192 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 29

3 years ago
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
(In reply to Wes Kocher (:KWierso) from comment #30)
> The test this patch added is failing on android:
> https://treeherder.mozilla.org/logviewer.html#?job_id=29108780&repo=mozilla-
> inbound
> 
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/73dc6bee6cf1

Also failed on linux: https://treeherder.mozilla.org/logviewer.html#?job_id=29111054&repo=mozilla-inbound
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 39

3 years ago
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+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 40

3 years ago
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

Comment 41

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/00312b136937
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

3 years ago
Depends on: 1279050

Comment 42

3 years ago
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: 1282162
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.
(Assignee)

Comment 46

3 years ago
(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.
(Assignee)

Comment 47

3 years ago
(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?
(Assignee)

Comment 50

3 years ago
(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
(Assignee)

Comment 51

3 years ago
(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.
(Assignee)

Comment 55

3 years ago
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

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

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