Bug 8079 – REGRESSION: Redraw from page cache does not show visited links

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[David Kilzer (:ddkilzer)]

Summary:

When clicking a link on a page, then hitting the browser "Back" button, the link that was clicked is not redrawn as a visited link.

Steps to reproduce:

1. Load a page with an unvisited link.  I used:

http://www.opendarwin.org/pipermail/webkit-unassigned/2006-March/date.html

2. Click on an unvisited (blue) link.

3. Click the browser "Back" button.

Expected results:

The link clicked in Step 2 should now look visited (purple).

Actual results:

The link clicked in Step 2 still looks unvisited (blue).

Regression:

Works for me on Mac OS X 10.4.5 (8H14) with Safari 2.0.3 (417.9.2).

Fails for me on Mac OS X 10.4.5 (8H14) with Safari 2.0.3 (417.9.2) and locally-built WebKit r13576.

[David Kilzer (:ddkilzer)]

Another quirk of this bug:  If the same link is clicked a second time, then the browser "Back" button is hit a second time, the link will finally be drawn as visited (purple).

I did a binary search through the nightlies and found that the bug occurred between these builds:

Works: WebKit-SVN-r13431.dmg
Fails: WebKit-SVN-r13442.dmg

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Works: WebKit-SVN-r13431.dmg
> Fails: WebKit-SVN-r13442.dmg

I suspect r13441 checked in by "tomernic" (see brief changelog entry below), but I haven't had a chance to build r13440 yet.

        Part of <rdar://problem/4351664> REGRESSION (420+): extra URL in b/f list - navigating back to previous page fai
ls at apple.com/retail/)
        This also fixes <rdar://problem/4477821> REGRESSION (10.4.5-TOT): meta tag specifying refresh is being added to 
history.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> I suspect r13441 checked in by "tomernic" (see brief changelog entry below),
> but I haven't had a chance to build r13440 yet.

I just built r13440 and it still exhibits this behavior, so it's something else.

[David Kilzer (:ddkilzer)]

This regression was caused by the patch for Bug 7907 (Replace more uses of DeprecatedString with String), committed as r13440.

Works: r13439
Fails: r13440

Note that I had to apply the build-fix patch (r13442) in order to build r13439 and r13440 to test.

[Darin Adler]

(In reply to comment #4)
> This regression was caused by the patch for Bug 7907 (Replace more uses of
> DeprecatedString with String), committed as r13440.

It's really hard to see which of the changes in r13440 caused this! None of them seem likely to do so, but if we knew which change it was we could probably figure it out.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> It's really hard to see which of the changes in r13440 caused this! None of
> them seem likely to do so, but if we knew which change it was we could probably
> figure it out.

As it turns out, the historyContains() method in HistoryMac.mm is never called after r13440 is applied when the browser "Back" button is hit, so my guess is that some string comparison is no longer causing the links to be re-rendered on the cached page.  I'm not very familiar with the rendering code in Safari, but below is the call stack from Safari+WebKit-r13439 when it hits the historyContains() method.  I will continue looking into this, but I'll be happy if someone else beats me to it.  :)

#0	0x01ae0938 in WebCore::historyContains at HistoryMac.mm:37
#1	0x019b08a8 in WebCore::checkPseudoState at cssstyleselector.cpp:579
#2	0x019b45b4 in WebCore::CSSStyleSelector::getColorFromPrimitiveValue at cssstyleselector.cpp:4257
#3	0x019ba094 in WebCore::CSSStyleSelector::applyProperty at cssstyleselector.cpp:2346
#4	0x019c1c04 in WebCore::CSSStyleSelector::applyDeclarations at cssstyleselector.cpp:1741
#5	0x019c2fb0 in WebCore::CSSStyleSelector::styleForElement at cssstyleselector.cpp:788
#6	0x0180af74 in WebCore::Element::recalcStyle at dom_elementimpl.cpp:562
#7	0x0180b2c8 in WebCore::Element::recalcStyle at dom_elementimpl.cpp:599
#8	0x0180b2c8 in WebCore::Element::recalcStyle at dom_elementimpl.cpp:599
#9	0x019279c8 in WebCore::Document::recalcStyle at Document.cpp:836
#10	0x0192d950 in WebCore::Document::updateStyleSelector at Document.cpp:1756
#11	0x018f9b3c in WebCore::Frame::reparseConfiguration at Frame.cpp:1660
#12	0x01946f54 in -[WebCoreFrameBridge reapplyStylesForDeviceType:] at WebCoreFrameBridge.mm:868
#13	0x0037ae0c in -[WebHTMLView reapplyStyles] at WebHTMLView.m:2275
#14	0x0037ae80 in -[WebHTMLView layoutToMinimumPageWidth:maximumPageWidth:adjustingViewSize:] at WebHTMLView.m:2290
#15	0x0037b228 in -[WebHTMLView layout] at WebHTMLView.m:2334
#16	0x00361818 in -[WebFrame(WebPrivate) _opened] at WebFrame.m:1028
#17	0x00360690 in -[WebFrame(WebPrivate) _commitProvisionalLoad:] at WebFrame.m:867
#18	0x003547c4 in -[WebDataSource(WebPrivate) _loadFromPageCache:] at WebDataSource.m:335
#19	0x0036883c in -[WebFrame(WebPrivate) _continueLoadRequestAfterNavigationPolicy:formState:] at WebFrame.m:2293
#20	0x003651e8 in -[WebFrame(WebPrivate) _checkNavigationPolicyForRequest:dataSource:formState:andCall:withSelector:] at WebFrame.m:1675
#21	0x00368c1c in -[WebFrame(WebPrivate) _loadDataSource:withLoadType:formState:] at WebFrame.m:2330
#22	0x0036353c in -[WebFrame(WebPrivate) _loadItem:withLoadType:] at WebFrame.m:1350
#23	0x00363e9c in -[WebFrame(WebPrivate) _recursiveGoToItem:fromItem:withLoadType:] at WebFrame.m:1469
#24	0x00364074 in -[WebFrame(WebPrivate) _goToItem:withLoadType:] at WebFrame.m:1489
#25	0x003ab900 in -[WebView(WebPrivate) _goToItem:withLoadType:] at WebView.m:753
#26	0x003b29b8 in -[WebView goBack] at WebView.m:1873
#27	0x003b613c in -[WebView(WebIBActions) goBack:] at WebView.m:2425
[...]

[David Kilzer (:ddkilzer)]

Created attachment 7476 [details]
Patch v1 (no test)

Okay, I found the problem using the debugger.  One line of code (a one-line 'if' statement that really should have been formatted as two lines) was removed from the end of the reparseConfiguration() method in Frame.cpp from the patch for Bug 7907:

-  if (d->m_doc) d->m_doc->updateStyleSelector();

Adding this back fixes this bug.  I do not have a test yet.  I think a test will require an http test with a PNG so that a unique link is rendered as visited.  I'll need more time to write a test if that is required for this fix (unless someone else wants to do it).

Also, I did not see a reason for removing this line of code in Bug 7907, so I'm not sure if adding this back is the appropriate fix.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Adding this back fixes this bug.  I do not have a test yet.  I think a test
> will require an http test with a PNG so that a unique link is rendered as
> visited.  I'll need more time to write a test if that is required for this fix
> (unless someone else wants to do it).

A test will also require the ability to "go back", either via a browser "Back" button or perhaps through a JavaScript "history.go(-1)" call.  Is this possible with DumpRenderTree?

[Maciej Stachowiak]

Regression tests do have the ability to go back (via window.history.back()) and also to save state across back/forward navigations via evalAfterBackForwardNavigation (Geoff and Trey have also discussed some extensions to this feature).

The patch looks great but I'll r- it for the lack of test case. Please put it back in r? if it turns out that making a test case is too hard.

[Darin Adler]

(In reply to comment #9)
> The patch looks great but I'll r- it for the lack of test case. Please put it
> back in r? if it turns out that making a test case is too hard.

I landed this patch because it was in the list to be committed, still marked r+. I guess you forgot to mark it r-. A test case is still a good idea.