Bug 13933 - REGRESSION: fast/history/clicked-link-is-visited is failing
Summary: REGRESSION: fast/history/clicked-link-is-visited is failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Beth Dakin
URL: http://build.webkit.org/results/post-...
Keywords: InRadar, LayoutTestFailure, Regression
Depends on:
Blocks:
 
Reported: 2007-05-30 11:00 PDT by Adam Roben (:aroben)
Modified: 2007-06-10 15:30 PDT (History)
2 users (show)

See Also:


Attachments
Fix Adele's test to reload through DRT (1.38 KB, patch)
2007-06-10 15:06 PDT, Beth Dakin
adele: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2007-05-30 11:00:23 PDT
fast/history/clicked-link-is-visited is failing on the buildbot.
Comment 1 Adam Roben (:aroben) 2007-05-30 11:02:11 PDT
<rdar://problem/5237078>
Comment 2 Beth Dakin 2007-06-08 15:15:09 PDT
This does not seem to reproduce outside of DumpRenderTree.
Comment 3 Beth Dakin 2007-06-08 16:17:52 PDT
Looks like this was caused by http://trac.webkit.org/projects/webkit/changeset/21649
Comment 4 Beth Dakin 2007-06-08 16:54:27 PDT
Working on this.
Comment 5 Beth Dakin 2007-06-08 19:09:37 PDT
This is difficult to debug because it only happens within DRT and only when you run the entire fast directory, but I have re-arranged the code a bit to get some clues, and I think that this is failing because of the reliance on having a page in the  EventHandler::focusDocumentView(), which does seems a bit hard to believe, but it seems like that is the problem.
Comment 6 Beth Dakin 2007-06-08 20:33:59 PDT
Actually, that last assessment was incorrect, but what is actually going on is even crazier. This failure has nothing to do with Adele's code change. Instead, it has to do with the test that she added! Deleting that test makes this test pass every time! One I discovered this, I tried to run the two tests back-to-back to see if the failure would reproduce, and it would not. After a lot of testing and reducing, I cam up with a reduced case, which is to run the following three tests in DRT in order:

fast/frames/empty.html
fast/frames/frame-display-none-focus.html (This is the test Adele added)
fast/history/clicked-link-is-visited.html

fast/frames/empty.html is not a real test. And it really is empty. For some reason, though, some test, any test!, needs to run before Adele's test to see the bug. 

So, with THIS reduction, I discovered that the problematic part of Adele's test is:

                if (parent.location.href.indexOf('?') == -1) {
                    parent.location.href= parent.location.href + "?";
                }

Removing that part of the test, also makes this other test succeed.

I have NO idea how adding a "?" to the parent href would end up affecting history in a later test. I have not figured this out yet at all. 

But I am also curious about why that is needed for the test anyway. I may test if removing those lines will still make the test a valid layout test for the original issue, but I doubt that Adele would have left it in if it were not necessary.
Comment 7 Adele Peterson 2007-06-08 21:14:25 PDT
The whole '?' thing is to avoid getting in an infinite loop of reloading the page.  You need to change the href to trigger a reload, but you only want to do it once.
Comment 8 Beth Dakin 2007-06-10 12:33:46 PDT
I re-wrote the test to reload in a different way, and it still caused the other test to fail. So there is something about reloading this page that causes the history/link-coloring test to fail.
Comment 9 Beth Dakin 2007-06-10 15:06:52 PDT
Created attachment 14926 [details]
Fix Adele's test to reload through DRT

I have a fix! Having the test reload through DRT's queueReload mechanism fixes the problem. I confirmed that this is still a valid test for the bug it originally fixed by rolling Adele's patch out of my tree. And sure enough, with Adele's patch gone, this new test also crashes. (Meaning it's valid!) My totally-pulled-out-of-my-ass threory is that reloading through JS got DRT's history all messed up. Maybe off-by-one? So DRT did not color the link the visited color because it did not make it into the history list yet. Or something.
Comment 10 Beth Dakin 2007-06-10 15:08:13 PDT
By the way, we should probably fix this problem in DRT. But right now, it is more important to have a green bot. And this is the fastest path to a green bot.
Comment 11 Adele Peterson 2007-06-10 15:20:55 PDT
Comment on attachment 14926 [details]
Fix Adele's test to reload through DRT

dear beth,

you rock.

from, adele
Comment 12 Beth Dakin 2007-06-10 15:30:14 PDT
Dear Adele, 

YOU TOOOO!

From, Beth

p.s. Fixed with r22087
p.p.s. adeleandbethrock.com