Bug 68278 – http/tests/history/back-with-fragment-change.py fails

• Component: Tools / Tests
• Status: ASSIGNED

[Mihai Parparita]

Test landed in http://trac.webkit.org/changeset/95259. See bug 64556 for history of this issue.

[Mihai Parparita]

Test was skipped with http://trac.webkit.org/changeset/95322.

[Mihai Parparita]

Created attachment 107739 [details]
Patch

[Adam Barth]

Comment on attachment 107739 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2301
> +// In cases where a load is not committed, undoes the speculative updating of
> +// the current history item that's done by HistoryController::goToItem.
> +void FrameLoader::undoSpeculativeBackForwardListUpdate()

Shouldn't this be a method of HistoryController since it interacts almost exclusively with HistoryController state?

[Mihai Parparita]

(In reply to comment #3)
> Shouldn't this be a method of HistoryController since it interacts almost exclusively with HistoryController state?

Makes sense. Updated patch coming up.

[Mihai Parparita]

Created attachment 107747 [details]
Patch

[Adam Barth]

This history stuff is somewhat mysterious to me.  Probably fishd should review as our history expert.

[WebKit Review Bot]

Comment on attachment 107747 [details]
Patch

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

New failing tests:
http/tests/navigation/forward-and-cancel.html

[David Kilzer (:ddkilzer)]

<rdar://problem/10490675>

[Eric Seidel (no email)]

I think you borked cr-linux?  This is clearly an Adam/Nate review.

[Darin Adler]

Comment on attachment 107747 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        navigated-to history item was cloberred by the same-document navigation.

typo: cloberred should be clobbered

> Source/WebCore/loader/HistoryController.cpp:549
> +void HistoryController::updateForProvisionalLoadStopped()

Should this function also set m_provisionalItem to 0? If so, you could remove that call from the one call site that does it separately and explicitly.

> Source/WebCore/loader/HistoryController.cpp:555
> +    bool isTargetItem = m_provisionalItem ? m_provisionalItem->isTargetItem() : false;

You just moved this line of code, but I suggest writing it with && instead of the ternary operator.

    bool isTargetItem = m_provisionalItem && m_provisionalItem->isTargetItem();

> Source/WebCore/loader/HistoryController.cpp:558
> +        if (HistoryItem* resetItem = mainFrame->loader()->history()->currentItem()) {

I find the variable name “reset item” to be a bit confusing. It sounds like a verb phrase to me.

> Source/WebCore/loader/HistoryController.h:73
> +    void updateForProvisionalLoadStopped();

A better name for this would be updateForStoppedProvisionalLoad. For the record, I also think the function above should be renamed updateForCompletedFrameLoad. I think I understand why someone gave that a different name before, but it’s unnecessarily non-grammatical naming.

[Simon Fraser (smfr)]

The test still fails.

[Ádám Kallai]

This test fails on Qt too. Is Anybody working on this bug?

[Stephen Chenney]

There is nobody from Chromium working on this.

[Ryosuke Niwa]

What happened to this patch?

[Darin Adler]

This test is marked as failing, not flaky, in TestExpectations but it’s not failing for me.

[Alexey Proskuryakov]

There are a lot of failures (both flaky and consistent) on https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fhistory%2Fback-with-fragment-change.php

[Darin Adler]

So we should mark this as flaky, not as known failure.

[Ryan Haddad]

Marked test as failing for mac-wk1, flaky for mac-wk2 in http://trac.webkit.org/r266758.

[Dawn Morningstar]

Created attachment 452118 [details]
expectations

[Robert Jenner]

Comment on attachment 452118 [details]
expectations

Clearing flags on attachment: 452118

Committed r289871 (?): <https://commits.webkit.org/r289871>

[Dawn Morningstar]

Re-added test expectations because they had been inadvertently removed.

[Fujii Hironori]

http/tests/history/back-with-fragment-change.py
https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fhistory%2Fback-with-fragment-change.py

Mac WK1 is constantly failing, GTK and WPE WK2 is randomly failing.

https://build.webkit.org/results/GTK-Linux-64-bit-Debug-Tests/270007@main%20(11606)/http/tests/history/back-with-fragment-change-diff.txt

--- /home/buildbot/worker/GTK-Linux-64-bit-Debug-Tests/build/layout-test-results/http/tests/history/back-with-fragment-change-expected.txt
+++ /home/buildbot/worker/GTK-Linux-64-bit-Debug-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.py  **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**
 ===============================================

[Przemyslaw Gorszkowski]

I think this test case is not compliant with the specification: https://html.spec.whatwg.org/multipage/browsing-the-web.html#centralized-modifications-of-session-historyn (the first example describes exactly the same case as is in this testcase). In my opinion (but please correct me if I am wrong) that the result of this test case should be:

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

Chrome and FireFox work in such way (at the end the back-with-fragment-change.py is displayed and due to deleting the sessionStorage.didNavigate; in function "done" of the back-with-fragment-change-target.html, the test case restarts).