Relative links with "://" in them are never colored as visited after being clicked

• Component: History
• Status: NEW ---

[Brett Wilson (Google)]

On Google, there are some relative links of the form:
  /url?sa=p&pref=ig&pval=3&q=http://www.google.com/ig%BlahBlahBlah

checkPseudoState() in cssstyleselector.cpp does a hardcoded search for "://" before checking to see if the URL begins with a slash. The result is that the un-resolved URL given above is passed to historyContains(), which then fails.

This function looks suspiciously hardcoded in other ways as well.

[David Kilzer (:ddkilzer)]

Created attachment 14592 [details]
Test case

Clicking the link never causes the link to be painted as "visited".

Works in Firefox 2.0.0.3 and Opera 9.10.

[David Kilzer (:ddkilzer)]

BTW, this is not a regression since it doesn't work in shipping Safari 2.0.4 (419.3) either.

[Eric Seidel (no email)]

Line 695 is the code in question:

    if (containsColonSlashSlash(characters, length)) {
        // FIXME: Strange to not clean the path just beacause it has "://" in it.
        pseudoState = historyContains(characters, length) ? PseudoVisited : PseudoLink;
        return;
    }

I'm not entirely sure what that code is even for.  This will require further investigation and test cases.

http://bugs.webkit.org/show_bug.cgi?id=15981 is the bug which introduced this if statement.  r28536 is when the change landed.

[Eric Seidel (no email)]

(I didn't mean to imply that bug 15981 broke this case, since as ddkilzer points out, this was already busted in Safari 2.0)

[Brett Wilson (Google)]

I don't understand this resolving code. Why isn't KURL used to do the resolution, it has the code and does this correctly. It's weird that this file has it's own relative URL resolver.

Would a patch to use KURL's resolving be OK? I would actually prefer this since the WebCore::historyContains function on my plactorm could be more efficiently written if I had a KURL rather than a string (since we do lookups based on URL objects). I doubt this would have a significant performance impact, since KURL's code is not much more complicated (this would have to be tested, of course).

I can easily put this patch together if you are willing to try this.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> I can easily put this patch together if you are willing to try this.

This sounds very reasonable.

Do you have layout tests (DumpRenderTree) running on your platform?  This would definitely need to be tested against the layout tests before landing, but I'm willing to do that on Mac.

[Darin Adler]

(In reply to comment #5)
> I don't understand this resolving code. Why isn't KURL used to do the
> resolution, it has the code and does this correctly. It's weird that this file
> has it's own relative URL resolver.

It's very old, from pre-WebKit KHTML.

The code is very performance sensitive. Even small changes to it affect our page loading test benchmarks.

[Brett Wilson (Google)]

I will run some perf tests I have before submitting a patch. I am just as worried about performance as you. Unfortunately, I can't run your own perf tests. I will make sure to run all the tests on my Mac.

[Brett Wilson (Google)]

I did a quick and dirty patch to test performance on (haven't run the layout tests or anything yet). This basically runs different pages through as fast as possible and measures total time. I used two sets of pages, simple ones and more complicated ones including things like Chinese. The test runs the set of pages 5 times. I ran it once to prime the cache, and then twice to get the timings.

Before the change (old code):
  simple set run 1: 7352ms
  simple set run 2: 7462ms
  complex set run 1: 34779ms
  complex set run 2: 37128ms

After the change (new code):
  simple set run 1: 7445ms
  simple set run 2: 7347ms
  complex set run 1: 36186ms
  complex set run 2: 34446ms

This is *not* using the code on my platform for looking up given a KURL. In other words, I convert the KURL to the character pointer given to historyContains, which then converts it back to a KURL. This is more realistic since it is more like what Apple will be running. I will change historyContains to take a KURL, so it will be up to each port how to look up URLs.

[Eric Seidel (no email)]

It's pretty common to run into this bug inside google, since most URLs get piped through google.com to block referrer sniffing.  This makes basically all non-google urls viewed on internal google pages via Safari marked as "non visited"... always.

Sounds like Marvin had a patch.  Would be nice to see said patch.

[Alex Baumgertner]

Still have this bug in Safari desktop Version 8.0.8 (10600.8.9) and mobile iOS7-8

test page
http://jsfiddle.net/wvnwa6en/

[Ahmad Saleem]

I am able to reproduce this bug in Safari Technology Preview 152 using attached test case and upon right clicking and opening new Tab from the link, refreshing and doing anything on the test case page does not change the link color to "visited" while other browsers (Chrome Canary 107 and Firefox Nightly 106) do change it to "purplish" to show that it is visited link.

NOTE - I tried it in non-PRIVATE window. Thanks!