Bug 135129 - Assertion failure in WebPage::reload (!m_pendingNavigationID) when reloading after a same-document back navigation
Summary: Assertion failure in WebPage::reload (!m_pendingNavigationID) when reloading ...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: mitz
Keywords: InRadar
Depends on:
Reported: 2014-07-21 13:33 PDT by mitz
Modified: 2017-03-20 15:20 PDT (History)
2 users (show)

See Also:

Don't create new navigations for same-document back-forward navigations (8.78 KB, patch)
2014-07-21 13:49 PDT, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2014-07-21 13:33:22 PDT

To reproduce: navigate to data:text/html,<a%20href="#">link</a> then tap the link, go back, and reload.
Comment 1 mitz 2014-07-21 13:49:34 PDT
Created attachment 235245 [details]
Don't create new navigations for same-document back-forward navigations
Comment 2 Darin Adler 2014-07-21 16:58:50 PDT
Comment on attachment 235245 [details]
Don't create new navigations for same-document back-forward navigations

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

> Source/WebKit2/Shared/WebBackForwardListItem.cpp:74
> +    for (const auto& child : a.children) {
> +        const FrameState* otherChild = childItemWithDocumentSequenceNumber(b, child.documentSequenceNumber);
> +        if (!otherChild || !documentTreesAreEqual(child, *otherChild))
> +            return false;
> +    }

Too bad this is n^2 the number of children at each level of the tree, and recursive.

It also unnecessarily compares every document sequence number during the recursion even though it finds each child by sequence number.

I could imagine a more version that copied and sorted the vectors of children to avoid the n^2.

And we might be able to cleanly avoid the recursion by using a Deque to keep track of children to compare.

But I’m not sure either of these changes would actually be an improvement.

> Source/WebKit2/Shared/WebBackForwardListItem.cpp:84
> +    // The following logic must be kept in sync with WebCore::HistoryItem::shouldDoSameDocumentNavigationTo.

I find the logic below utterly mysterious. Maybe the “why” comments are in shouldDoSameDocumentNavigationTo?

> Source/WebKit2/Shared/WebBackForwardListItem.cpp:95
> +    WebCore::URL url = WebCore::URL(WebCore::ParsedURLString, mainFrameState.urlString);
> +    WebCore::URL otherURL = WebCore::URL(WebCore::ParsedURLString, otherMainFrameState.urlString);
> +
> +    if ((url.hasFragmentIdentifier() || otherURL.hasFragmentIdentifier()) && equalIgnoringFragmentIdentifier(url, otherURL))

Seems a real shame we have to reconstitute the URL objects just to find the "#" characters in them. I am pretty sure this can be done more efficiently without fully re-parsing the URLs, but it might make the code harder to maintain so I guess we can leave it like this.
Comment 3 mitz 2014-07-24 18:33:12 PDT
Fixed in <http://trac.webkit.org/r171574>.