Bug 50331 - ASSERT failing restoring scroll position from HistoryItem after reload
Summary: ASSERT failing restoring scroll position from HistoryItem after reload
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 57906
  Show dependency treegraph
 
Reported: 2010-12-01 09:55 PST by Mario Sanchez Prada
Modified: 2012-02-21 08:56 PST (History)
10 users (show)

See Also:


Attachments
Patch proposal (2.61 KB, patch)
2010-12-01 10:22 PST, Mario Sanchez Prada
eric: review-
Details | Formatted Diff | Diff
Patch proposal + unit test (6.98 KB, patch)
2010-12-06 09:20 PST, Mario Sanchez Prada
darin: review-
Details | Formatted Diff | Diff
Patch proposal + unit test (7.50 KB, patch)
2011-01-03 09:33 PST, Mario Sanchez Prada
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2010-12-01 09:55:16 PST
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.
Comment 1 Mario Sanchez Prada 2010-12-01 10:22:01 PST
Created attachment 75287 [details]
Patch proposal

Attaching patch based on the comments pointed out in the description
Comment 2 Mario Sanchez Prada 2010-12-01 10:24:59 PST
Adding to CC people found through svn blame
Comment 3 Eric Seidel (no email) 2010-12-03 10:29:29 PST
Btw, webkit-patch upload --suggest-reviewers will do the svn blame step for you automatically and then offer to CC relevant reviewers.  It's certainly not perfect, but it's often good enough.
Comment 4 Eric Seidel (no email) 2010-12-03 10:30:27 PST
Comment on attachment 75287 [details]
Patch proposal

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

This needs a test.

> WebCore/loader/FrameLoader.cpp:2408
> +            // If the user had a scroll point, scroll to it,
> +            // overriding the anchor point if any. Also, make sure
> +            // there's a valid current item in the history before
> +            // trying to scroll because it could happen there was none
> +            // (e.g. loading from a string instead of an URI).
> +            if (m_frame->page() && history()->currentItem()) {

Seems rather overzealous line wrapping. :)
Comment 5 Mario Sanchez Prada 2010-12-06 04:45:45 PST
Thanks for the review Eric, but I have some doubts that are currently blocking me from providing a better patch. See below...

(In reply to comment #4)
> (From update of attachment 75287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75287&action=review
> 
> This needs a test.

Not sure whether you mean a Layout Test (this is my guessing, though) or an unit test.

The problem with writing a Layout Test for this is that, as I said, the bug happens when  using webkit_web_view_load_string() in an unit test for the GTK port, which basically loads an HTML content from a (const gchar*) instead of loading the content from an URI. And that is (not having an URI) precisely the problem causing this bug, since history->currentItem() will be 0 at the point we're trying to restore the scroll position, right after finishing the reload process.

Hence, I don't know how to test this with a layout test, since (1) a layout test has always an URI (the path to the test), so it wouldn't be testing the problem I try to fix, and (2) since the problem was detected from a function of the GTK port.


In case you're talking about an unit test (or perhaps you were not, but my paragraphs above has finally convinced you :-)), I do not see any problem on writing an small unit test in the GTK port for that, which would be a ultra-reduced version of testWebkitAtkDocumentReloadEvents() (provided with patch for bug 25831), in case that was fine.

So, as you can see, I need some extra input here to continue working on this :-)

> > WebCore/loader/FrameLoader.cpp:2408
> > +            // If the user had a scroll point, scroll to it,
> > +            // overriding the anchor point if any. Also, make sure
> > +            // there's a valid current item in the history before
> > +            // trying to scroll because it could happen there was none
> > +            // (e.g. loading from a string instead of an URI).
> > +            if (m_frame->page() && history()->currentItem()) {
> 
> Seems rather overzealous line wrapping. :)

I don't think I deserve credit for this: emacs was the author of such a beautiful piece of art!
Comment 6 Mario Sanchez Prada 2010-12-06 09:20:28 PST
Created attachment 75704 [details]
Patch proposal + unit test

Attaching a new patch with a new unit test to check everything works as expected when reloading a webView loaded through a gchar*. The unit test is GTK specific, so I guess it's not the perfect solution, but the best one I could find so far since I wasn't able to write a layout test to test this stuff.
Comment 7 Eric Seidel (no email) 2010-12-12 02:27:51 PST
CC'd folks who know more than I do about history.
Comment 8 Mario Sanchez Prada 2010-12-13 01:41:39 PST
(In reply to comment #7)
> CC'd folks who know more than I do about history.

Thanks!
Comment 9 Martin Robinson 2010-12-27 14:44:33 PST
This test changes platform-independent WebCore code. Is it possible to write a layout test for all platforms?
Comment 10 Darin Adler 2010-12-29 18:01:04 PST
Comment on attachment 75704 [details]
Patch proposal + unit test

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

Thanks for tackling this.

> WebCore/loader/FrameLoader.cpp:2410
> +            // If the user had a scroll point, scroll to it, overriding the anchor
> +            // point if any. Also, make sure there's a valid current item in the
> +            // history before trying to scroll because it could happen there was
> +            // none (e.g. loading from a string instead of an URI).
> +            if (m_frame->page() && history()->currentItem()) {
>                  if (isBackForwardLoadType(m_loadType) || m_loadType == FrameLoadTypeReload || m_loadType == FrameLoadTypeReloadFromOrigin)
>                      history()->restoreScrollPositionAndViewState();
>              }

It’s pretty clear looking at this code that the currentItem check belongs inside the restoreScrollPositionAndViewState function, not here at the call site.
Comment 11 Mario Sanchez Prada 2011-01-03 02:10:22 PST
(In reply to comment #9)
> This test changes platform-independent WebCore code. Is it possible to write a layout test for all platforms?

A I said in comment #6, I wouldn't know how to write a layout test to test this stuff, that's why I'm proposing a GTK-specific unit test instead. 

On the bright side, it seems this problem affects only to the GTK port, so perhaps it's not a terrible solution either, but you'll have the final word, of course :-)
Comment 12 Mario Sanchez Prada 2011-01-03 07:27:45 PST
Oops! forgot to reply this other comment...

(In reply to comment #10)
> (From update of attachment 75704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75704&action=review
> 
> Thanks for tackling this.
> 
> > WebCore/loader/FrameLoader.cpp:2410
> > +            // If the user had a scroll point, scroll to it, overriding the anchor
> > +            // point if any. Also, make sure there's a valid current item in the
> > +            // history before trying to scroll because it could happen there was
> > +            // none (e.g. loading from a string instead of an URI).
> > +            if (m_frame->page() && history()->currentItem()) {
> >                  if (isBackForwardLoadType(m_loadType) || m_loadType == FrameLoadTypeReload || m_loadType == FrameLoadTypeReloadFromOrigin)
> >                      history()->restoreScrollPositionAndViewState();
> >              }
> 
> It’s pretty clear looking at this code that the currentItem check belongs inside the 
> restoreScrollPositionAndViewState function, not here at the call site.

As I said in the description, I was not sure about that being the right option, although I also considered as a possibility:

" [...] 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 [...]" 


So, I initially proposed adding the extra check in the call site in order not to remove the ASSERT in HistoryController::restoreScrollPositionAndViewState(), which I thought perhaps could be seen as the wrong thing to do. But now you commented on this I'm more for adding the protection inside that function, replacing the ASSERT right there with just a null check to avoid doing anything if currentItem is null.

So, expect a new patch in some minutes...

Thanks for the feedback!
Comment 13 Mario Sanchez Prada 2011-01-03 09:33:35 PST
Created attachment 77812 [details]
Patch proposal + unit test

Attaching new patch to be reviewed, this time moving the null check to HistoryController::restoreScrollPositionAndViewState(), that is, removing the ASSERT and explaining the change in a comment :P

Also, provided the unit test as in the previous patch
Comment 14 Joanmarie Diggs (irc: joanie) 2011-01-24 15:37:41 PST
So the good news is that Orca+Epiphany are shaping up nicely.

The bad news is when I follow links and new pages are loaded Orca has no clue and thus presents the no-longer displayed content. :-( If this is the only thing standing in the way of committing the fix for bug 25831, please let me know what bribes are needed to get Mario's patch reviewed and they will be in the mail. :-P

(Really sorry to be a noodge. And thanks in advance guys!!)
Comment 15 Mario Sanchez Prada 2011-01-25 02:01:57 PST
(In reply to comment #14)
> So the good news is that Orca+Epiphany are shaping up nicely.

Great!

> The bad news is when I follow links and new pages are loaded Orca has no clue 
> and thus presents the no-longer displayed content. :-( If this is the only 
> thing standing in the way of committing the fix for bug 25831,

Yes, actually this is the only pending bit blocking the (already reviewed+) patch for 25831 to be applied.

> please let me 
> know what bribes are needed to get Mario's patch reviewed and they will be in 
> the mail. :-P

:-)
Comment 16 Darin Fisher (:fishd, Google) 2011-01-25 02:53:43 PST
Comment on attachment 77812 [details]
Patch proposal + unit test

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

> WebCore/loader/HistoryController.cpp:-104
> -    

this seems like the wrong direction to go.  you have identified a case
where we can get a null m_currentItem.  perhaps we should instead try
to fix that case to not have a null m_currentItem?

i think it needlessly complicates HistoryController to have to support
a null m_currentItem as we sort of do today.
Comment 17 Mario Sanchez Prada 2011-01-25 03:57:48 PST
(In reply to comment #16)
> (From update of attachment 77812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77812&action=review
> 
> > WebCore/loader/HistoryController.cpp:-104
> > -    
> 
> this seems like the wrong direction to go.  you have identified a case
> where we can get a null m_currentItem.  perhaps we should instead try
> to fix that case to not have a null m_currentItem?
> 
> i think it needlessly complicates HistoryController to have to support
> a null m_currentItem as we sort of do today.

That was my first approach (to avoid getting a null m_currentItem when calling that function), but in comment #10 Darin (well, "the other Darin") pointed out that it would be better to make the change right inside restoreScrollPositionAndViewState(), so I did.

Not sure what to do now, perhaps I'm missing something and there's a third way to fix this, but I can't clearly see it at this moment.

Any hint?

Thanks for the feedback, btw.
Comment 18 Darin Fisher (:fishd, Google) 2011-01-25 09:14:41 PST
(In reply to comment #17)
> > this seems like the wrong direction to go.  you have identified a case
> > where we can get a null m_currentItem.  perhaps we should instead try
> > to fix that case to not have a null m_currentItem?
> > 
> > i think it needlessly complicates HistoryController to have to support
> > a null m_currentItem as we sort of do today.
> 
> That was my first approach (to avoid getting a null m_currentItem when calling that function), but in comment #10 Darin (well, "the other Darin") pointed out that it would be better to make the change right inside restoreScrollPositionAndViewState(), so I did.
> 
> Not sure what to do now, perhaps I'm missing something and there's a third way to fix this, but I can't clearly see it at this moment.

Yes, I was referring to a different approach.  Instead of null checking m_currentItem, I think it would be better to ensure that webkit_web_view_load_string actually creates a HistoryItem.  Why doesn't it create one?

webkit_web_view_load_string looks a lot like WebKit::WebFrame::loadData() in the Chromium WebKit API.  Perhaps you can compare your implementation of webkit_web_view_load_string to that to see how they compare.
Comment 19 Mario Sanchez Prada 2011-01-26 06:12:15 PST
(In reply to comment #18)
> [...] 
> Yes, I was referring to a different approach.  Instead of null checking
> m_currentItem, I think it would be better to ensure that 
> webkit_web_view_load_string actually creates a HistoryItem. 
> Why doesn't it create one?

As far as I can see from the code and while debugging, it seems the problem is that such a function ends up calling webkit_web_frame_load_data () with zero in the unreachableURL param, meaning that no history item will be created later on in HistoryController::updateForStandardLoad(), as historyURL.isEmpty() will return true there.

I could probably use webkit_web_frame_load_alternate_string() from the unit test to workaround this problem (passing a dummy unreachableURL), but I don't think it's a good approach... still think that if that was the case the last proposed patch would be a better option, since it would be gracefully treating the (very strange) case of calling to restoreScrollPosition() with m_currentItem == 0.

But take this with a grain of salt, of course. I know can be wrong again

> webkit_web_view_load_string looks a lot like WebKit::WebFrame::loadData()
> in the Chromium WebKit API.  Perhaps you can compare your implementation
> of webkit_web_view_load_string to that to see how they compare.

I've spent all the morning debugging the code and deeply analyzing the code behind that function (and comparing it to webkit_web_view_load_string() in the GTK port), and I can't properly understand why that function in Chromium wouldn't be affected by the same problem... actually, hope you don't mind me explicitly asking this but: are you sure the same problem doesn't happen in Chromium? To be sure, it would be wonderful if you could try to reproduce it by:

  1. Compile a debug build, so ASSERTS will make WK crash if false.
  2. Create a new WebKit view in your test code.
  3. Load an static string with HTML content on it through WebKit::WebFrame::loadData(), as you said.
  4. Try to reload now.

If I'm right, you should be getting a crash as the result of the ASSERT failing in restoreScrollPositionAndViewState().

I of course understand that you could have no time to test this, but I'd appreciate at least some insight on these thoughts, they could be really helpful to me.

Thanks!
Comment 20 Mario Sanchez Prada 2011-02-09 01:18:09 PST
Ping?
Comment 21 Charles Reis 2011-04-05 17:05:33 PDT
I've discovered a way to write a layout test for this bug:

w = window.open();
w.document.write(1);
w.location.reload();

This code is actually broken in another way (in that it loads the wrong page during reload()), but it does hit the same assert being discussed here.  I've filed bug 57906 for the reload bug, and I've uploaded a sample layout test there.
Comment 22 Adam Barth 2011-04-26 15:35:53 PDT
Comment on attachment 77812 [details]
Patch proposal + unit test

There seems to be agreement in the comments that this ASSERT is correct and other code should be changed so that m_currentItem is always non-NULL at this program point.
Comment 23 Mario Sanchez Prada 2011-04-26 16:17:24 PDT
(In reply to comment #22)
> (From update of attachment 77812 [details])
> There seems to be agreement in the comments that this ASSERT is correct and 
> other code should be changed so that m_currentItem is always non-NULL at this 
> program point.

Then, unless I'm missing something else, the right approach would be the one proposed in the second patch, right?

https://bugs.webkit.org/attachment.cgi?id=75704&action=prettypatch
Comment 24 Mario Sanchez Prada 2011-12-04 08:19:24 PST
(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 77812 [details] [details])
> > There seems to be agreement in the comments that this ASSERT is correct and 
> > other code should be changed so that m_currentItem is always non-NULL at this 
> > program point.
> 
> Then, unless I'm missing something else, the right approach would be the one proposed in the second patch, right?
> 
> https://bugs.webkit.org/attachment.cgi?id=75704&action=prettypatch

Sorry about getting back to this again, but I found myself today in the very same situation while writing a new test, and it would be awesome if we could fix this (supposing it's a bug, which is not 100% clear to me at this point).

So... ping? :-)
Comment 25 Darin Adler 2011-12-05 12:02:57 PST
Comment on attachment 77812 [details]
Patch proposal + unit test

I still don’t understand why there is no currentItem when we are loading from a string. Fixing that seems like the way to go.

If we do agree that instead we want a null pointer check, then we have a battle of the Darins here. The question is what restoreScrollPositionAndViewState should do when there is no history. Should it assert (and then the caller has to check a pointer for null), or check and quietly do nothing when there is nothing to restore. The reason I suggest the latter is that it seems non-obvious that what the caller should null check is currentItem. Having the check inside the function makes more sense to me because it’s in there that it’s clear what to check.
Comment 26 Darin Adler 2011-12-05 12:04:21 PST
The reason these patches keep getting review- is that there is no explanation of *why* current item is 0 in the “load from string” case. Once we understand why it’s 0 we can figure out if it’s a bug or not. Nobody wants to just add code to handle 0 without knowing if the 0 is legitimate or not.

The debate about whether to put the null check inside the function or outside it is a relatively unimportant sideshow to that question.
Comment 27 Jack 2012-02-21 08:56:35 PST
I'm getting the same error - but in real use, not testing.  I'm using webkit-gtk with balsa (email app) under Gentoo.  I just did a debug compile of webkit-gtk 1.6.3 to track down a different problem.  I don't know if it's balsa or webkit, but when I first display an HTML email, it does not load remote images.  As soon as I click on the button to load remote images, I get this crash.

Oddly, run under gdb, it is not finding symbols for libwebktigtk, but that may be a Gentoo related issue.  I'm mentioning it only in that I won't be able to provide a useful backtrace until I resolve that issue.