Bug 68278 - http/tests/history/back-with-fragment-change.py fails
Summary: http/tests/history/back-with-fragment-change.py fails
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihai Parparita
Keywords: InRadar
Depends on:
Reported: 2011-09-16 13:31 PDT by Mihai Parparita
Modified: 2023-10-31 23:09 PDT (History)
18 users (show)

See Also:

Patch (10.22 KB, patch)
2011-09-16 15:56 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (10.22 KB, patch)
2011-09-16 16:56 PDT, Mihai Parparita
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
expectations (1.45 KB, patch)
2022-02-15 17:50 PST, Dawn Morningstar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2011-09-16 13:31:44 PDT
Test landed in http://trac.webkit.org/changeset/95259. See bug 64556 for history of this issue.
Comment 1 Mihai Parparita 2011-09-16 13:44:08 PDT
Test was skipped with http://trac.webkit.org/changeset/95322.
Comment 2 Mihai Parparita 2011-09-16 15:56:08 PDT
Created attachment 107739 [details]
Comment 3 Adam Barth 2011-09-16 16:05:53 PDT
Comment on attachment 107739 [details]

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?
Comment 4 Mihai Parparita 2011-09-16 16:15:45 PDT
(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.
Comment 5 Mihai Parparita 2011-09-16 16:56:25 PDT
Created attachment 107747 [details]
Comment 6 Adam Barth 2011-09-16 17:02:14 PDT
This history stuff is somewhat mysterious to me.  Probably fishd should review as our history expert.
Comment 7 WebKit Review Bot 2011-09-17 17:56:17 PDT
Comment on attachment 107747 [details]

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

New failing tests:
Comment 8 David Kilzer (:ddkilzer) 2011-11-28 08:00:59 PST
Comment 9 Eric Seidel (no email) 2012-05-21 15:10:37 PDT
I think you borked cr-linux?  This is clearly an Adam/Nate review.
Comment 10 Darin Adler 2012-08-14 16:17:24 PDT
Comment on attachment 107747 [details]

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.
Comment 11 Simon Fraser (smfr) 2012-09-19 13:55:26 PDT
The test still fails.
Comment 12 Ádám Kallai 2013-02-14 06:38:49 PST
This test fails on Qt too. Is Anybody working on this bug?
Comment 13 Stephen Chenney 2013-04-09 05:43:54 PDT
There is nobody from Chromium working on this.
Comment 14 Ryosuke Niwa 2013-05-22 15:44:07 PDT
What happened to this patch?
Comment 15 Darin Adler 2020-09-04 15:19:10 PDT
This test is marked as failing, not flaky, in TestExpectations but it’s not failing for me.
Comment 16 Alexey Proskuryakov 2020-09-04 16:37:56 PDT
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
Comment 17 Darin Adler 2020-09-04 16:58:58 PDT
So we should mark this as flaky, not as known failure.
Comment 18 Ryan Haddad 2020-09-08 16:33:03 PDT
Marked test as failing for mac-wk1, flaky for mac-wk2 in http://trac.webkit.org/r266758.
Comment 19 Dawn Morningstar 2022-02-15 17:50:21 PST
Created attachment 452118 [details]
Comment 20 Robert Jenner 2022-02-15 17:56:06 PST
Comment on attachment 452118 [details]

Clearing flags on attachment: 452118

Committed r289871 (?): <https://commits.webkit.org/r289871>
Comment 21 Dawn Morningstar 2022-02-15 17:57:28 PST
Re-added test expectations because they had been inadvertently removed.
Comment 22 Fujii Hironori 2023-10-31 23:09:36 PDT

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


--- /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 ==============  **nav target**
-  **nav target**
 curr->  **nav target**