Full Text Bug Listing

Mario Sanchez Prada

Reported 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.

Mario Sanchez Prada

Comment 1 2010-12-01 10:22:01 PST

Created attachment 75287 [details] Patch proposal Attaching patch based on the comments pointed out in the description

Mario Sanchez Prada

Comment 2 2010-12-01 10:24:59 PST

Adding to CC people found through svn blame

Eric Seidel (no email)

Comment 3 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.

Eric Seidel (no email)

Comment 4 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. :)

Mario Sanchez Prada

Comment 5 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!

Mario Sanchez Prada

Comment 6 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.

Eric Seidel (no email)

Comment 7 2010-12-12 02:27:51 PST

CC'd folks who know more than I do about history.

Mario Sanchez Prada

Comment 8 2010-12-13 01:41:39 PST

(In reply to comment #7) > CC'd folks who know more than I do about history. Thanks!

Martin Robinson

Comment 9 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?

Darin Adler

Comment 10 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.

Mario Sanchez Prada

Comment 11 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 :-)

Mario Sanchez Prada

Comment 12 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!

Mario Sanchez Prada

Comment 13 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

Joanmarie Diggs

Comment 14 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!!)

Mario Sanchez Prada

Comment 15 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 :-)

Darin Fisher (:fishd, Google)

Comment 16 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.

Mario Sanchez Prada

Comment 17 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.

Darin Fisher (:fishd, Google)

Comment 18 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.

Mario Sanchez Prada

Comment 19 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!

Mario Sanchez Prada

Comment 20 2011-02-09 01:18:09 PST

Ping?

Charles Reis

Comment 21 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.

Adam Barth

Comment 22 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.

Mario Sanchez Prada

Comment 23 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

Mario Sanchez Prada

Comment 24 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? :-)

Darin Adler

Comment 25 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.

Darin Adler

Comment 26 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.

Jack

Comment 27 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.

Ahmad Saleem

Comment 28 2023-10-03 10:50:37 PDT

*** This bug has been marked as a duplicate of bug 252944 ***

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Bug 50331