REGRESSION (r13615): Acid2 Test Eyes render improperly after a page refresh

• Component: Page Loading
• Status: RESOLVED FIXED

[Elliott Sprehn]

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,
r21420).

[Alexey Proskuryakov]

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.

[David Kilzer (:ddkilzer)]

<rdar://problem/5319523>

[Cameron Zwarich (cpst)]

The bug first occurs between the r13594 and r13626 nightlies. The most obvious candidate for the introduction of the bug is r13615:

<http://trac.webkit.org/projects/webkit/changeset/13615>

[Cameron Zwarich (cpst)]

As expected, it is indeed r13615 that introduces this bug.

[Luca Bruno]

I see eyes correctly using WebKit/Gtk+ after refreshing the page, using r28711.

[Alexey Proskuryakov]

Still an issue on Mac OS X (r28711).

[Richard Secor]

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.

[Cameron Zwarich (cpst)]

Created attachment 19791 [details]
Reduction

Here is a reduction of the bug. The problem seems to be occurring because of the navigation to #top. It reloads fine without that.

[Oliver Hunt]

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

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

(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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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?

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

Created attachment 19957 [details]
Revised proposed patch

Weinig noticed a typo in the comment. Thanks!

[Oliver Hunt]

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...

[Cameron Zwarich (cpst)]

Created attachment 19959 [details]
Revised proposed patch

Here are the changes you requested, Oliver.

[Oliver Hunt]

Comment on attachment 19959 [details]
Revised proposed patch

r=me!!!

[Oliver Hunt]

Landed r31228