[Webkit-unassigned] [Bug 50331] New: ASSERT failing restoring scroll position from HistoryItem after reload

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50331

           Summary: ASSERT failing restoring scroll position from
                    HistoryItem after reload
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: Page Loading
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: msanchez at igalia.com


As unveiled by a new unit test in patch for bug 25831 (currently rolled out), there's a situation where the following ASSERT in HistoryController is failing, causing crashes in GTK's debug builds:

    void HistoryController::restoreScrollPositionAndViewState()
    {
        if (!m_frame->loader()->stateMachine()->committedFirstRealDocumentLoad())
            return;

        ASSERT(m_currentItem);
        [...]
    }

After some investigation it seems that, in the specific scenario caused by bug 25831's patch, the HistoryController is never, ever, updated with a valid 'currentItem', causing this failure when trying to call this function, while finishing the reload of the webview. So, the question is "how could it be possible that we reached this situation?", and I think I found the answer...

The situation that triggers this problem is the following code in WebKit/gtk/tests/testatk.c (where 'contents' is a const char* with a hardcoded html code):

    static void testWebkitAtkDocumentReloadEvents()
    {
        WebKitWebView* webView = WEBKIT_WEB_VIEW(webkit_web_view_new());
        [...]
        webkit_web_view_load_string(webView, contents, 0, 0, 0);
        [...]
    }

As you can see, that code just loads the HTML in the 'contents' variable in the webview. Then, when trying to test the actual issue fixed with that patch, we do:

       webkit_web_view_reload(webView);

... and that's what's causing the problem, resulting in the following error output:

   ASSERTION FAILED: m_currentItem
   (../../WebCore/loader/HistoryController.cpp:103 void WebCore::HistoryController::restoreScrollPositionAndViewState())


After investigating the issue for a long time, diving through a long backtrace, I found the problem seems to be that it's ok not to have a valid m_currentItem in the HistoryController for this case, since the webView is being loaded from a raw char* with the HTML code, instead of fetching the HTML content from a URI (as it would be done with webkit_web_view_load_uri(), in WebKitGTK), hence there's no valid HistoryItem to be stored (no URI!), resulting in the problem explained above.

Hence I think the problem could be solved by either removing that ASSERT from HistoryController::restoreScrollPositionAndViewState(), accepting that there are still situations (like this one) where m_currentItem could be 0, or just to try to protect the calls to that function from the callers when needed. So far, I found that the only place that would require such that extra check would be the following one:

    void FrameLoader::checkLoadCompleteForThisFrame()
    {
        [...]
        switch (m_state) {
                [...]
            case FrameStateCommittedPage: {
                [...]
                // If the user had a scroll point, scroll to it, overriding the anchor point if any.
                if (m_frame->page()) {
                    if (isBackForwardLoadType(m_loadType) || m_loadType == FrameLoadTypeReload || m_loadType == FrameLoadTypeReloadFromOrigin)
                        history()->restoreScrollPositionAndViewState();
                }
                [...]
        }
        [...]
    }

I think we could protect this scenario by just doing something like this:

   // If the user had a scroll point, scroll to it, overriding the anchor point if any.
   if (m_frame->page() && history()->currentItem()) {
       if (isBackForwardLoadType(m_loadType) || m_loadType == FrameLoadTypeReload || m_loadType == FrameLoadTypeReloadFromOrigin)
           history()->restoreScrollPositionAndViewState();
   }


I tested this solution and avoids the crash in my debug build of WebKitGTK, while still passing all the tests, but still not sure whether that's the right solution, if it would be better just to remove the ASSERT in HistoryController::restoreScrollPositionAndViewState() or if I'm missing something else, in which case I'd appreaciate any kind of feedback :-)

Sorry for the long report, hope at least you'll find this info valuable.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.