Bug 13693

Summary: REGRESSION (r13615): Acid2 Test Eyes render improperly after a page refresh
Product: WebKit Reporter: Elliott Sprehn <esprehn>
Component: Page LoadingAssignee: Cameron Zwarich (cpst) <zwarich>
Severity: Normal CC: ap, beidson, ddkilzer, gavin.sharp, ian, oliver, trey, webkit, zwarich
Priority: P1 Keywords: HasReduction, InRadar, Regression, ReviewedForRadar
Version: 523.x (Safari 3)   
Hardware: Macintosh   
OS: OS X 10.4   
URL: http://www.webstandards.org/files/acid2/test.html
Bug Depends on:    
Bug Blocks: 4911    
Description Flags
Further reduction
Proposed patch
Revised proposed patch
oliver: review+
Revised proposed patch
Revised proposed patch oliver: review+

Description Elliott Sprehn 2007-05-12 00:28:13 PDT
Steps to reproduce:
1. Load the acid 2 test
2. Click "Take the Test"
3. Press Apple+R
4. Click "Take the Test" again.

The checked background behind the eyes should now be visible, and the eyes shouldn't be rendered.

Reproducible in the latest nightly. (Fri May 11 22:54:47 GMT 2007,
Comment 1 Alexey Proskuryakov 2007-07-06 06:04:15 PDT
Wow. Confirmed with r23922. Please note that one doesn't get to step 4 with 10.4 WebKit - the page automatically reloads to its test view.
Comment 2 David Kilzer (:ddkilzer) 2007-07-07 13:14:33 PDT
Comment 3 Cameron Zwarich (cpst) 2007-08-06 03:11:28 PDT
The bug first occurs between the r13594 and r13626 nightlies. The most obvious candidate for the introduction of the bug is r13615:

Comment 4 Cameron Zwarich (cpst) 2007-08-06 14:43:59 PDT
As expected, it is indeed r13615 that introduces this bug.
Comment 5 Luca Bruno 2007-12-14 03:10:18 PST
I see eyes correctly using WebKit/Gtk+ after refreshing the page, using r28711.
Comment 6 Alexey Proskuryakov 2007-12-14 05:30:09 PST
Still an issue on Mac OS X (r28711).
Comment 7 Richard Secor 2008-03-03 16:41:23 PST
This appears to have nothing to do with Safari.
There is something wrong with the stylesheet information on this page.
It doesn't even come up right the first time in Firefox.
Comment 8 Cameron Zwarich (cpst) 2008-03-16 03:05:24 PDT
Created attachment 19791 [details]

Here is a reduction of the bug. The problem seems to be occurring because of the navigation to #top. It reloads fine without that.
Comment 9 Oliver Hunt 2008-03-16 03:14:44 PDT
Much craziness: once you've gotit to the point that the eyes are gone doing "View source" on the page produces the source for about:blank
Comment 10 Cameron Zwarich (cpst) 2008-03-16 11:20:29 PDT
Created attachment 19801 [details]
Further reduction

The fallback content doesn't need to be a data URL for this to work, it can even be text.
Comment 11 Cameron Zwarich (cpst) 2008-03-16 11:26:25 PDT
(In reply to comment #9)
> Much craziness: once you've gotit to the point that the eyes are gone doing
> "View source" on the page produces the source for about:blank

Viewing the source works for me with Safari 3.0.4, with both the shipping WebKit and ToT.
Comment 12 Cameron Zwarich (cpst) 2008-03-17 11:49:51 PDT
When the loading of the fallback content should occur in MainResourceLoader::continueAfterContentPolicy(), the URL of the object resource is blanked out along with the fact that it is an HTTP resource.
Comment 13 Cameron Zwarich (cpst) 2008-03-18 00:06:15 PDT
Something triggered by the call to the FrameLoadDelegate in WebFrameLoaderClient::dispatchDidChangeLocationWithinPage() is causing this problem to occur. Commenting out the call makes it go away.
Comment 14 Cameron Zwarich (cpst) 2008-03-18 02:29:21 PDT
The URL gets blanked out by the line

URL = [NSURL _web_URLWithDataAsString:childItem->originalURLString()];

in the middle of loadURL in WebFrame.mm. The backtrace on the call to HistoryItem::setOriginalURLString() that sets it to "about:blank" is

#0  WebCore::HistoryItem::setOriginalURLString (this=0x117d3960, urlString=@0xbfffe110) at /Users/Cameron/WebKit/WebCore/history/HistoryItem.cpp:224
#1  0x01596e4c in WebCore::FrameLoader::createHistoryItem (this=0x31b4000, useOriginal=true) at /Users/Cameron/WebKit/WebCore/loader/FrameLoader.cpp:3849
#2  0x01596f80 in WebCore::FrameLoader::createHistoryItemTree (this=0x31b4000, targetFrame=0x2cf18f0, clipAtTarget=false) at /Users/Cameron/WebKit/WebCore/loader/FrameLoader.cpp:3891
#3  0x01597042 in WebCore::FrameLoader::createHistoryItemTree (this=0x30a0400, targetFrame=0x2cf18f0, clipAtTarget=false) at /Users/Cameron/WebKit/WebCore/loader/FrameLoader.cpp:3898
#4  0x01597225 in WebCore::FrameLoader::addBackForwardItemClippedAtTarget (this=0x30a0400, doClip=false) at /Users/Cameron/WebKit/WebCore/loader/FrameLoader.cpp:3884
#5  0x015981a5 in WebCore::FrameLoader::addHistoryItemForFragmentScroll (this=0x30a0400) at /Users/Cameron/WebKit/WebCore/loader/FrameLoader.cpp:3785
#6  0x0159828c in WebCore::FrameLoader::continueFragmentScrollAfterNavigationPolicy (this=0x30a0400, request=@0xbfffe4a4, shouldContinue=true) at /Users/Cameron/WebKit/WebCore/loader/FrameLoader.cpp:3438
#7  0x01598332 in WebCore::FrameLoader::callContinueFragmentScrollAfterNavigationPolicy (argument=0x30a0400, request=@0xbfffe4a4, shouldContinue=true) at /Users/Cameron/WebKit/WebCore/loader/FrameLoader.cpp:3408
#8  0x01589768 in WebCore::PolicyCheck::call (this=0xbfffe4a4, shouldContinue=true) at /Users/Cameron/WebKit/WebCore/loader/FrameLoader.cpp:4563
#9  0x0158df5f in WebCore::FrameLoader::continueAfterNavigationPolicy (this=0x30a0400, policy=WebCore::PolicyUse) at /Users/Cameron/WebKit/WebCore/loader/FrameLoader.cpp:3581
warning: internal error: no C/C++ fundamental type 1
#10 0x001b6792 in WebFrameLoaderClient::receivedPolicyDecison (this=0x2cf1a70, action=WebCore::PolicyUse) at /Users/Cameron/WebKit/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1068
Comment 15 Cameron Zwarich (cpst) 2008-03-18 02:52:23 PDT
The problem is that FrameLoader::createHistoryItem() is being called to create the history items generated by the internal navigation, one of which is used to reload the <object>. There are checks in createHistoryItem() which cause something that never loaded to be given an "about:blank" URL in the history, but this is clearly wrong in this case.

How do we change the behaviour of createHistoryItem() so it is correct in this case but doesn't cause any issues with histories? The only real constraint seems to be that HistoryItem URLs can't be null.
Comment 16 Cameron Zwarich (cpst) 2008-03-18 11:42:40 PDT
It seems that none of the URLs that are considered in FrameLoader::createHistoryItem() are the correct one. What's the best way to get the correct URL from the <object> element to place it in the history?
Comment 17 Cameron Zwarich (cpst) 2008-03-21 19:00:16 PDT
I think the bug here is that there is even a HistoryItem being created in the first place. If the <object> data is set to a page that actually loads, then we have the following sequence:

On load:

- HistoryItem created for the main document
- HistoryItem created for the <object> data

On internal anchor navigation:

- HistoryItem created for new URL with anchor
- HIstoryItem created for <object> data, even though it hasn't changed

If the <object> data 404s, and we go to fallback content, then we have the following sequence:

On load:

- HistoryItem created for the main document
- HistoryItem *not* created for the <object> data, which never loaded

On internal anchor navigation:

- HistoryItem created for new URL with anchor
- HIstoryItem created for <object> data, even though it never loaded, which gets sent to about:blank because the DocumentLoader doesn't have any information (is this a bug in itself?)

The HistoryItem for the <object> confuses the reload into thinking it should use that information. I think the way to fix this is to change the code so that a HistoryItem doesn't get created for an <object> inside of a document on an internal navigation if the data never loaded.

I don't think this is the same as bug 3580, although one could probably find other bugs related to bug 3580 and fallback content, e.g. if content loaded properly I don't think reloading would actually reload it and check for a 404.
Comment 18 Cameron Zwarich (cpst) 2008-03-21 19:52:27 PDT
Created attachment 19948 [details]
Proposed patch

Here is a patch that fixes the bug. There are some test failures, but they all occur in my tree with or without this patch.

I didn't ask for review because I don't have any layout tests. I will add one and upload a new patch.
Comment 19 Cameron Zwarich (cpst) 2008-03-21 21:13:09 PDT
Created attachment 19952 [details]
Revised proposed patch

Here is the patch with a test added. The test fails with no output under ToT. Oliver suggested that I make local variables for the things that appear in the if statement to make it a bit clearer, so I did.
Comment 20 Cameron Zwarich (cpst) 2008-03-21 23:30:23 PDT
Created attachment 19957 [details]
Revised proposed patch

Weinig noticed a typo in the comment. Thanks!
Comment 21 Oliver Hunt 2008-03-21 23:38:47 PDT
Comment on attachment 19957 [details]
Revised proposed patch

Looking at their implementations i think it might be better to change this to:
if (!(!hasChildLoaded && childLoader->isHostedByObjectElement()))

as isHostedByObjectElement doesn't look to be zero cost, and appears to be the more expensive of those calls...
Comment 22 Cameron Zwarich (cpst) 2008-03-21 23:55:47 PDT
Created attachment 19959 [details]
Revised proposed patch

Here are the changes you requested, Oliver.
Comment 23 Oliver Hunt 2008-03-21 23:57:51 PDT
Comment on attachment 19959 [details]
Revised proposed patch

Comment 24 Oliver Hunt 2008-03-22 00:20:34 PDT
Landed r31228