Bug 17186 - Fragment navigation within a page permanently cancels meta refresh
Summary: Fragment navigation within a page permanently cancels meta refresh
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-02-05 14:43 PST by Tim S
Modified: 2010-06-10 15:33 PDT (History)
1 user (show)

See Also:


Attachments
testcase (187 bytes, text/html)
2008-02-05 14:48 PST, Tim S
no flags Details
Fix is to reload the page on fragment nav if redirect notification has been sent. (1.48 KB, patch)
2008-02-11 11:32 PST, Tim S
no flags Details | Formatted Diff | Diff
patch with regression test (6.50 KB, patch)
2008-02-24 21:01 PST, Darin Adler
darin: review-
Details | Formatted Diff | Diff
patch (9.35 KB, patch)
2008-02-24 22:03 PST, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim S 2008-02-05 14:43:46 PST
1. Page containing a meta-refresh is loaded.
2. Click on a link to a fragment in the page.
3. When the timer for the refresh fires, FrameLoader::shouldReload returns false because it is an in-page navigation and so the meta tag is not re-parsed and the refresh will never be rescheduled.

FF/IE do continue firing refreshes.

Additionally in this case, while scrollToAnchor does reschedule _ONE_ instance of the refresh and dispatch a WillPerformClientRedirect, it does so without dispatching DidCancelClientRedirect first, so clients can't distinguish between the rescheduling and the _actual_ redirect navigation. It should probably first call stopRedirectionTimer.
Comment 1 Tim S 2008-02-05 14:48:24 PST
Created attachment 18941 [details]
testcase 

Webkit/Safari: Clicking on the href will result in the page getting refreshed once and only once after. 

FF/IE: It will continue to be refreshed every 10 seconds as per the meta-refresh.
Comment 2 Mark Rowe (bdash) 2008-02-05 16:50:41 PST
<rdar://problem/5726537>
Comment 3 Tim S 2008-02-11 11:32:15 PST
Created attachment 19071 [details]
Fix is to reload the page on fragment nav if redirect notification has been sent.
Comment 4 Darin Adler 2008-02-13 05:03:07 PST
If you can't make an automated test for this, can you at least add the test case to the patch in the WebCore/manual-tests directory?

Also, I don't see why you can't make an automated test. It seems entirely possible to do so!
Comment 5 Tim S 2008-02-13 11:09:27 PST
Re: the test - I've asked around several times on #webkit for help and have spoken 1/1 with some webkit developers, and I spent a day or two trying to write a good test. But it is absolutely possible you know something I don't and I'd be willing to give it another go if you give me a pointer. The main problem: I don't see a way to use the layout test controller to maintain state (exposed to the page itself) across loads of the page, and I need to do this so that I know after I fake a link click on the anchor, how many times the page (with identical url) has been loaded. AFAIK, I can't add URL params or modify the urls involved to point at a "server side" script because that will cause FrameLoader::shouldReload to behave differently and I wont end up testing exactly this situation.

I'm all ears.
Comment 6 Darin Adler 2008-02-24 20:09:31 PST
(In reply to comment #5)
> I don't see a way to use the layout test controller to maintain state
> (exposed to the page itself) across loads of the page, and I need to do this so
> that I know after I fake a link click on the anchor, how many times the page
> (with identical url) has been loaded.

A few ideas come to mind:

    1) Since DumpRenderTree supports multiple windows, you can do a window.open and store the persistent state in a separate window.

    2) If the test can run in a subframe, then you can store the persistent state in the top frame. I don't see any reason this can't work in a subframe, but maybe I'm overlooking something.

    3) If the test can't run in a subframe, we can add something to the test controller to let you load and store a persistent string. It's very simple to add that.

I'm sure one of these will work.
Comment 7 Darin Adler 2008-02-24 20:50:31 PST
I got (2) to work and I have a test now.
Comment 8 Darin Adler 2008-02-24 21:01:14 PST
Created attachment 19334 [details]
patch with regression test

Here's a patch that includes a regression test. However, I'm almost certain this fix is incorrect. There are two things wrong with it:

    1) The m_sentRedirectNotification flag is not the right thing to look at. That flag has a specific purpose relating to sending notifications to the FrameLoaderClient, and this is not an appropriate use of it.

    2) Forcing a reload is not the correct behavior. We don't want to stop the meta refresh, but we also don't want to do a reload of the page for the same reason we don't reload other times.

I'll investigate other fixes.
Comment 9 Darin Adler 2008-02-24 22:03:53 PST
Created attachment 19339 [details]
patch
Comment 10 Anders Carlsson 2008-02-25 09:03:04 PST
Comment on attachment 19339 [details]
patch

r=me
Comment 11 Darin Adler 2008-02-25 09:35:12 PST
Committed revision 30571.
Comment 12 Alexey Proskuryakov 2008-03-04 01:13:38 PST
The fix was rolled out in r30738, because it caused bug 17569.
Comment 13 Alexey Proskuryakov 2008-03-04 01:13:56 PST
Comment on attachment 19339 [details]
patch

Clearing review flag.