Bug 13693 - REGRESSION (r13615): Acid2 Test Eyes render improperly after a page refresh
: REGRESSION (r13615): Acid2 Test Eyes render improperly after a page refresh
Status: RESOLVED FIXED
: WebKit
Page Loading
: 523.x (Safari 3)
: Macintosh Mac OS X 10.4
: P1 Normal
Assigned To:
: http://www.webstandards.org/files/aci...
: HasReduction, InRadar, Regression, Re...
:
: 4911
  Show dependency treegraph
 
Reported: 2007-05-12 00:28 PST by
Modified: 2008-03-22 00:20 PST (History)


Attachments
Reduction (2.77 KB, text/html)
2008-03-16 03:05 PST, Cameron Zwarich (cpst)
no flags Details
Further reduction (273 bytes, text/html)
2008-03-16 11:20 PST, Cameron Zwarich (cpst)
no flags Details
Proposed patch (2.07 KB, patch)
2008-03-21 19:52 PST, Cameron Zwarich (cpst)
no flags Review Patch | Details | Formatted Diff | Diff
Revised proposed patch (3.86 KB, patch)
2008-03-21 21:13 PST, Cameron Zwarich (cpst)
oliver: review+
Review Patch | Details | Formatted Diff | Diff
Revised proposed patch (3.86 KB, patch)
2008-03-21 23:30 PST, Cameron Zwarich (cpst)
no flags Review Patch | Details | Formatted Diff | Diff
Revised proposed patch (3.81 KB, patch)
2008-03-21 23:55 PST, Cameron Zwarich (cpst)
oliver: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2007-05-12 00:28:13 PST
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).
------- Comment #1 From 2007-07-06 06:04:15 PST -------
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 From 2007-07-07 13:14:33 PST -------
<rdar://problem/5319523>
------- Comment #3 From 2007-08-06 03:11:28 PST -------
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>
------- Comment #4 From 2007-08-06 14:43:59 PST -------
As expected, it is indeed r13615 that introduces this bug.
------- Comment #5 From 2007-12-14 03:10:18 PST -------
I see eyes correctly using WebKit/Gtk+ after refreshing the page, using r28711.
------- Comment #6 From 2007-12-14 05:30:09 PST -------
Still an issue on Mac OS X (r28711).
------- Comment #7 From 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 From 2008-03-16 03:05:24 PST -------
Created an attachment (id=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.
------- Comment #9 From 2008-03-16 03:14:44 PST -------
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 From 2008-03-16 11:20:29 PST -------
Created an attachment (id=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 From 2008-03-16 11:26:25 PST -------
(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 From 2008-03-17 11:49:51 PST -------
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 From 2008-03-18 00:06:15 PST -------
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 From 2008-03-18 02:29:21 PST -------
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 From 2008-03-18 02:52:23 PST -------
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 From 2008-03-18 11:42:40 PST -------
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 From 2008-03-21 19:00:16 PST -------
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 From 2008-03-21 19:52:27 PST -------
Created an attachment (id=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 From 2008-03-21 21:13:09 PST -------
Created an attachment (id=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 From 2008-03-21 23:30:23 PST -------
Created an attachment (id=19957) [details]
Revised proposed patch

Weinig noticed a typo in the comment. Thanks!
------- Comment #21 From 2008-03-21 23:38:47 PST -------
(From update of attachment 19957 [details])
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 From 2008-03-21 23:55:47 PST -------
Created an attachment (id=19959) [details]
Revised proposed patch

Here are the changes you requested, Oliver.
------- Comment #23 From 2008-03-21 23:57:51 PST -------
(From update of attachment 19959 [details])
r=me!!!
------- Comment #24 From 2008-03-22 00:20:34 PST -------
Landed r31228