41973 - Redirected links are not marked Visited

Status: VERIFIED FIXED

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

Description

[Pierre Saslawsky]

- Go to Yahoo
- Click almost any link
- Go back
==> The link is not marked as visited

I guess it is related to the fact that these links are redirected. For instance, 
if you click "Business & Economy", it goes to http://www.yahoo.com/r/bu which is 
redirected to http://dir.yahoo.com/Business_and_Economy/

Comment 1

[Pierre Saslawsky]

Marking beta2/top100

Comment 2

[Radha on family leave (not reading bugmail)]

waterson, Do you have any clue on this one? Thanks,

Comment 3

[Chris Waterson]

it's probably not being added to the global history. check the flow of control 
in the docshell stuff.

Comment 4

[leger]

Putting on [nsbeta2-] radar. Not critical to beta2.  Adding nsbeta3 keyword.

Comment 5

[Pierre Saslawsky]

Moving some of my comments from bug 12493...
----
Bug 41973 comes from the assumption that a page that causes a redirection isn't 
added to the History before causing the redirection. But as I'm writing this, I'm 
realizing that Marc and I could have been wrong because the URLs on Yahoo are not 
canonized either. We need to write a testcase with canonized URLs that cause a 
redirection. If it works, then bug 41973 is in fact a dup of bug 12493.

Comment 6

[johng]

nav triage team:
if this also effects Back / Fwd, we will definitely nsbeta3+ it.

By the way, this effects almost all portals like Yahoo, Netscape.com, etc.

Comment 7

[johng]

nav triage team:
Claudius says it does not effect Back / Forward button - doesn't appear to.

However, this does mean that links on about every portal will nto change colors 
to show that you have visited the page.  That is bad, marking nsbeta3+ 
(borderline, so if it is hard to fix, we might revisit this).

Comment 8

[Radha on family leave (not reading bugmail)]

The fix for it needs change in docshell. Not in Session History. Changing 
component.

Comment 9

[Judson Valeski]

Radha, do we change the color of links based on history entries? If so, I don't
think the docshell will ever know about a redirect as they happen transparently
(right gagan?) in necko. This would imply that http would need to set the
history :-/.

Comment 10

[Radha on family leave (not reading bugmail)]

visited link color change is done by global history owned by rjc. But I think 
the code that lets global history know about such links exists in docshell. 
There is a inherent problem with redirected links though. Redirected links are 
passed to docshell with loadtype "loadNormal" which gives rise to a bunch of 
sesion History related bugs. Gagan has a bug on this.  May be this problem is 
related to the bug that gagan has. 

Comment 11

[Gagan]

I don't see how the loadNormal bug is related to this bug. The essential problem
as I see it is that history is not updated for visited links (thru redirects)
and the main reason is becuz webshell/docsheel needs to update that just as they
would update the location bar on a redirect. cc'ing mscott.

Comment 12

[Scott MacGregor]

Okay so on a whim I implemented nsIHTTPEventSink tonight 'cause we seem to have
a set of bugs that we think would be fixed by such an implementation.

Looking at this bug after implementing http event sink, I'm not sure how that's
supposed to fix this. The docshell gets told of the location change. It looks
like the new url is added correctly to history.

The problem is really that we don't add urls to history until after we load
them. i.e. until after the redirection happens....so we only end up adding the
final url. 

Comment 13

[Judson Valeski]

so who's impl'ing nsIHTTPEnventSink? I suspect whoever's doing that should add 
the original url (the one that caused the redirect) to history when the 
HTTPEventSink gets redirect notification (maybe this is in the form of checking 
the HTTP status code?). Scott, from your comments I can't tell if this is 
already happening or not?

Comment 14

[Radha on family leave (not reading bugmail)]

*** Bug 44560 has been marked as a duplicate of this bug. ***

Comment 15

[clayton]

If this is truly only a lack of visual indication, it can wait for the next 
release cycle.

Comment 16

[Phil Peterson]

Not holding PR3 for this; marking nsbeta3-.

Comment 17

[Andreas Franke (gone)]

*** Bug 59304 has been marked as a duplicate of this bug. ***

Comment 18

[Michael Ward]

*** Bug 58642 has been marked as a duplicate of this bug. ***

Comment 19

[Asa Dotzler [:asa]]

*** Bug 60211 has been marked as a duplicate of this bug. ***

Comment 20

[Radha on family leave (not reading bugmail)]

Looking thro' the comments, I see that the problem described here occurs for all 
server side redirects. Necko handles the redirection and docshell has absolutely 
no info on it. So the original url  never gets added to global history, which 
means no color change for the original url. nsDocLoader implements 
nsIHTTPEventSink and it sends out a nsIWebProgressListener::onStateChange() 
notification when there is a redirect on the main document channel.  DocShell 
implements nsIWebProgressListener. Little debugging should provide info on 
whether this notification comes on time to update the history. 

Re-assigning to self.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 61223 has been marked as a duplicate of this bug. ***

Comment 22

[Radha on family leave (not reading bugmail)]

Attachment: patch to docshell

Comment 23

[rpotts (gone)]

(r=rpotts)

hey Radha,

This patch looks fine...  You're correct that when the document URI gets
redirected, OnStateChange(...) is called with STATE_REDIRECTING |
STATE_IS_DOCUMENT flags set...

-- rick

Comment 24

[Chris Waterson]

sr=waterson; looks good to me.

Comment 25

[Scott MacGregor]

Rick's right, you are using the correct status flags. sr=mscott

Comment 26

[Brendan Eich [:brendan]]

Not raising any flag or demanding any change, but trying to be consistent: I
recently persuaded/nagged adam lock into optimising (flags & F1) && (flags & F2)
into (~flags & (F1|F2)) == 0.  Same applies here:

+  else if ((aStateFlags & STATE_IS_DOCUMENT) && (aStateFlags &
STATE_REDIRECTING)) {

could be, maybe should be:

+  else if ((~aStateFlags & (STATE_IS_DOCUMENT | STATE_REDIRECTING)) == 0) {

saves a branch and second mask test, compares to zero for faster test on many
architectures.  jst used this pattern recently, too.

/be

Comment 27

[Radha on family leave (not reading bugmail)]

This was fixed last week

Comment 28

[Claudius Gayle]

VERIFIED Fixed 20010207 builds