Bug 17186 – Fragment navigation within a page permanently cancels meta refresh

• Component: Page Loading
• Status: RESOLVED CONFIGURATION CHANGED

[Tim S]

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.

[Tim S]

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.

[Mark Rowe (bdash)]

<rdar://problem/5726537>

[Tim S]

Created attachment 19071 [details]
Fix is to reload the page on fragment nav if redirect notification has been sent.

[Darin Adler]

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!

[Tim S]

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.

[Darin Adler]

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

[Darin Adler]

I got (2) to work and I have a test now.

[Darin Adler]

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.

[Darin Adler]

Created attachment 19339 [details]
patch

[Anders Carlsson]

Comment on attachment 19339 [details]
patch

r=me

[Darin Adler]

Committed revision 30571.

[Alexey Proskuryakov]

The fix was rolled out in r30738, because it caused bug 17569.

[Alexey Proskuryakov]

Comment on attachment 19339 [details]
patch

Clearing review flag.

[Ahmad Saleem]

It seems to work fine in Safari 17.4.1 for me and tab is refreshed every 10 seconds and continues to do so similar to Chrome Canary 125.

@Karl - can you double check and then we can mark this as 'RESOLVED CONFIGURATION CHANGED'?

[Karl Dubost]

This is working, but it also surfaces something not very practical in the Web Inspector.