Bug 13933 – REGRESSION: fast/history/clicked-link-is-visited is failing

• Component: History
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

fast/history/clicked-link-is-visited is failing on the buildbot.

[Adam Roben (:aroben)]

<rdar://problem/5237078>

[Beth Dakin]

This does not seem to reproduce outside of DumpRenderTree.

[Beth Dakin]

Looks like this was caused by http://trac.webkit.org/projects/webkit/changeset/21649

[Beth Dakin]

Working on this.

[Beth Dakin]

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.

[Beth Dakin]

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.

[Adele Peterson]

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.

[Beth Dakin]

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.

[Beth Dakin]

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.

[Beth Dakin]

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.

[Adele Peterson]

Comment on attachment 14926 [details]
Fix Adele's test to reload through DRT

dear beth,

you rock.

from, adele

[Beth Dakin]

Dear Adele, 

YOU TOOOO!

From, Beth

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