Bug 64556 – Fragment navigations should interrupt a provisional load of a different document

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Charles Reis]

If a provisional navigation to a different document is in progress but a fragment scroll in the current page commits, we should cancel the provisional navigation.

Letting the provisional navigation continue to load and commit after the fragment scroll is a bit strange in the normal case, but dangerous to the session history if it's a back/forward navigation.  Suppose the user is going forward but interrupts it with a fragment navigation.  When the fragment navigation commits, it clears the forward history, even though the forward navigation is still in progress.

I have a fairly straightforward fix prepared, which calls StopAllLoaders if there's a provisional loader for a different document in FrameLoader::continueFragmentScrollAfterNavigationPolicy.

[Charles Reis]

Created attachment 100853 [details]
Patch

[WebKit Review Bot]

Comment on attachment 100853 [details]
Patch

Attachment 100853 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9059319

New failing tests:
fast/loader/page-dismissal-modal-dialogs.html

[WebKit Review Bot]

Created attachment 100854 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexey Proskuryakov]

The bug seems valid to me, but stopping all loaders doesn't sound right. The current document may have its own loaders, which don't need to be stopped for fragment navigation.

[Charles Reis]

(In reply to comment #4)
> The bug seems valid to me, but stopping all loaders doesn't sound right. The current document may have its own loaders, which don't need to be stopped for fragment navigation.

Yes, and stopAllLoaders also seems to have led to a crash in page-dismissal-modal-dialogs.html (which is happening when we interact with m_documentLoader in checkLoadCompleteForFrame).

Is there a better way to stop just the provisional load?  Presumably we should do it for the whole frame tree (much like stopAllLoaders), but not stop m_documentLoader.  There's clearProvisionalLoad(), but that doesn't deal with the rest of the frame tree.  (Maybe that's ok?)

[Mihai Parparita]

Created attachment 107415 [details]
Patch

[Mihai Parparita]

I've updated Charlie's patch to only stop provisional loaders (via a recursive FrameLoader::stopAllProvisionalLoaders method).

That made fast/loader/page-dismissal-modal-dialogs.html not crash, but it would still fail. This was because the order of events was:
1. main frame "load" fires
2. main frame sets location to resources/pass-and-notify-done.html (the provisional load)
3. main frame "beforeunload" fires
4. Event handler simulates a click on the <a href="#"> anchor node
5. Beyond invoking the showMessages function, since that doesn't preventDefault the event, this triggers a same-document navigation, thus canceling the provisional load.

Since it seemed like having the anchor node be a real link wasn't what was being tested, I've just changed it to a <span>, so that the event handler still gets triggered (which shows an alert during page unload, which is what the test is testing for), but we don't cause a same-document navigation anymore.

+Sreeram in case I'm misunderstanding the purpose of the test and the <a href="#"> part is necessary

[Mihai Parparita]

Cc-ing more people who know FrameLoader.

[Alexey Proskuryakov]

Can a regression test be made for this?

Stopping provisional loaders in subframes when a top frame navigates to an anchor is surprising, too.

[Mihai Parparita]

(In reply to comment #9)
> Can a regression test be made for this?

http/tests/navigation/navigation-interrupted-by-fragment.html tests this, sorry for not mentioning it in the ChangeLog.

> Stopping provisional loaders in subframes when a top frame navigates to an anchor is surprising, too.

Hmm, now that I think about it, yes, I don't see any reason for stopping provisional loads in subframes.  Changed to just stop the current frame's provisional loader.

[Mihai Parparita]

Created attachment 107419 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 107419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107419&action=review

Thanks, I have no further comment. Others CC'ed on the bug would make better reviewers for actual code change. Considering whether the urls being compared are original or final ones in case of redirect seems important.

> Source/WebCore/loader/FrameLoader.cpp:2708
> +    // If we have a provisional request for a different document, a fragment scroll should cancel it.

It's not entirely obvious why a provisional request for the same document should not be canceled.

[Adam Barth]

Comment on attachment 107419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107419&action=review

> Source/WebCore/loader/FrameLoader.cpp:2712
> +    // If we have a provisional request for a different document, a fragment scroll should cancel it.
> +    if (m_provisionalDocumentLoader && !equalIgnoringFragmentIdentifier(m_provisionalDocumentLoader->request().url(), request.url())) {
> +        m_provisionalDocumentLoader->stopLoading();
> +        setProvisionalDocumentLoader(0);
> +    }

This part makes sense to me.

> Source/WebCore/loader/HistoryController.cpp:531
> +    // The provisional item may represent a different pending navigation.
> +    // Don't commit it if it isn't a same document navigation.
> +    if (m_currentItem && !m_currentItem->shouldDoSameDocumentNavigationTo(m_provisionalItem.get()))
> +        return;

This part I'm less confident in, mostly because I don't understand HistoryController very well.

[Mihai Parparita]

(In reply to comment #13)
> > Source/WebCore/loader/HistoryController.cpp:531
> > +    // The provisional item may represent a different pending navigation.
> > +    // Don't commit it if it isn't a same document navigation.
> > +    if (m_currentItem && !m_currentItem->shouldDoSameDocumentNavigationTo(m_provisionalItem.get()))
> > +        return;
> 
> This part I'm less confident in, mostly because I don't understand HistoryController very well.

This was in Charlie's original patch, I didn't look at it too closely originally. This happens when there's a provisional history item set (triggered via history.back() or anything else that ends up in FrameLoader::loadDifferentDocumentItem) but then we abort that provisional load because of a fragment change. This didn't have a test originally, I've added http/tests/history/back-with-fragment-change.php to the patch to exercise this behavior. If we do overwrite the current item with the different document provisional one (that belongs to the aborted load), then the back-forward list would end up being:

        http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**
        http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**
curr->  http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**

Instead of the expected:

        http://127.0.0.1:8000/history/back-with-fragment-change.php  **nav target**
        http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html  **nav target**
curr->  http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**

[Mihai Parparita]

Created attachment 107432 [details]
Patch

[Sreeram Ramachandran]

(In reply to comment #7)
> Since it seemed like having the anchor node be a real link wasn't what was being tested, I've just changed it to a <span>, so that the event handler still gets triggered (which shows an alert during page unload, which is what the test is testing for), but we don't cause a same-document navigation anymore.
> 
> +Sreeram in case I'm misunderstanding the purpose of the test and the <a href="#"> part is necessary

LGTM. The anchor isn't important. Any method that executes JS in the context of the other frame works. The simplest way I found was to dispatch a click event to an element in the other frame.. A span works just as well as an anchor for this purpose. Thanks for taking care of the test!

[WebKit Review Bot]

Comment on attachment 107432 [details]
Patch

Attachment 107432 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9685040

New failing tests:
http/tests/history/back-with-fragment-change.php

[Mihai Parparita]

(In reply to comment #17)
> New failing tests:
> http/tests/history/back-with-fragment-change.php

I thought the EWS would attach results from failing runs to the bug. Does it no longer do that?

[Eric Seidel (no email)]

(In reply to comment #18)
> (In reply to comment #17)
> > New failing tests:
> > http/tests/history/back-with-fragment-change.php
> 
> I thought the EWS would attach results from failing runs to the bug. Does it no longer do that?

I'm told it was disabled due to space concerns.

[Adam Barth]

Comment on attachment 107432 [details]
Patch

Ok.  I'm not 100% confident in the HistoryController part of the change, but it looks fine as far as I can tell.

[Mihai Parparita]

It looks like http/tests/history/back-with-fragment-change.php fails with release builds (which is what EWS uses. I assume there's some a timing bug in the test, where the load starts already by the time the fragment change is issued, so it's not considered provisional. I'll see if I can tweak it to make it more reliable before landing.

[Mihai Parparita]

Created attachment 107576 [details]
Patch for landing

[Mihai Parparita]

Actually, the problem with the test was that back-with-fragment-change.php was getting cached, so the load history.back() was called was not taking 2 seconds as expected (it was only a matter of luck that it passed with debug builds). Adding the appropriate HTTP headers to disable caching made it pass in both build types.

[WebKit Review Bot]

Comment on attachment 107576 [details]
Patch for landing

Clearing flags on attachment: 107576

Committed r95259: <http://trac.webkit.org/changeset/95259>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

This test still fails on SL, on GTK and on Qt ...

--- /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/http/tests/history/back-with-fragment-change-expected.txt 
+++ /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/http/tests/history/back-with-fragment-change-actual.txt 
@@ -4,6 +4,5 @@
 
 ============== Back Forward List ==============
         http://127.0.0.1:8000/history/back-with-fragment-change.php  **nav target**
-        http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html  **nav target**
 curr->  http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**
 ===============================================

[Mihai Parparita]

Looking into those failures. Windows fails too, presumably because of HTTP server differences: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r95289%20(16342)/http/tests/history/back-with-fragment-change-diffs.txt

[Mihai Parparita]

Leaving this as closed, bug 68278 is open for the failures on other platforms.

[David Kilzer (:ddkilzer)]

<rdar://problem/10327710>

[Brady Eidson]

This caused a regression in Safari choosing the different comics at the top of http://www.uclick.com/client/nyt/db/

Covered in <rdar://problem/11019576>

[Mihai Parparita]

(In reply to comment #30)
> This caused a regression in Safari choosing the different comics at the top of http://www.uclick.com/client/nyt/db/

The comic switching UI boils down to: <a href="#" onclick="window.location = '...'">...</a>. So the regression is expected given the change that was made.

Charlie's original concerns when he filed the bug were about fragment scrolls during back/forward navigation. Charlie, was it http://crbug.com/86758 that prompted this (which was an address bar spoofing security bug triggered by back/forward navigation)?

Maybe it makes sense to revise the fix such that we only cancel provisional navigation during back/forward navigation only.

[Charles Reis]

(In reply to comment #31)
> (In reply to comment #30)
> > This caused a regression in Safari choosing the different comics at the top of http://www.uclick.com/client/nyt/db/
> 
> The comic switching UI boils down to: <a href="#" onclick="window.location = '...'">...</a>. So the regression is expected given the change that was made.
> 
> Charlie's original concerns when he filed the bug were about fragment scrolls during back/forward navigation. Charlie, was it http://crbug.com/86758 that prompted this (which was an address bar spoofing security bug triggered by back/forward navigation)?
> 
> Maybe it makes sense to revise the fix such that we only cancel provisional navigation during back/forward navigation only.

Yes, that was the original bug, which was about using back and forward to cause a URL spoof.  I take it the "#" on the link click is preempting the window.location change?  I'm not 100% sure it's safe to ignore new navigations and only have this apply to back/forward.  It depends on whether the correct URL is displayed if we allow the provisional new navigation to commit.