Bug 18064

Summary: assert-fail in WebCore::FrameLoader::saveDocumentState (document is null)
Product: WebKit Reporter: Jasper Bryant-Greene <jasper>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: jchaffraix, jmalonzo, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
URL: http://acid3.acidtests.org/reference.html
Bug Depends on: 18411    
Bug Blocks:    
Attachments:
Description Flags
remove bogus assertion none

Jasper Bryant-Greene
Reported 2008-03-25 03:34:29 PDT
This is only an issue in debug builds because in release builds there is a null check for document immediately after the assert. A probable fix is to just remove the ASSERT() although it would be interesting to find out why document is null only on GTK. Backtrace: #0 0x00007fc0ce043ed1 in WebCore::FrameLoader::saveDocumentState (this=0xb2f930) at WebCore/loader/FrameLoader.cpp:4001 #1 0x00007fc0ce05f029 in WebCore::FrameLoader::closeURL (this=0x7fffd7019640) at WebCore/loader/FrameLoader.cpp:641 #2 0x00007fc0ce05f07d in WebCore::FrameLoader::detachFromParent (this=0xb2f930) at WebCore/loader/FrameLoader.cpp:3194 #3 0x00007fc0cdfaf105 in WebCore::HTMLFrameOwnerElement::willRemove (this=0xa068c0) at WebCore/html/HTMLFrameOwnerElement.cpp:46 #4 0x00007fc0cddd679e in WebCore::ContainerNode::willRemove (this=0xb07860) at WebCore/dom/ContainerNode.cpp:347 #5 0x00007fc0cddd679e in WebCore::ContainerNode::willRemove (this=0xaf6e30) at WebCore/dom/ContainerNode.cpp:347 #6 0x00007fc0cddd679e in WebCore::ContainerNode::willRemove (this=0x82a580) at WebCore/dom/ContainerNode.cpp:347 #7 0x00007fc0cddd679e in WebCore::ContainerNode::willRemove (this=0x851100) at WebCore/dom/ContainerNode.cpp:347 #8 0x00007fc0ce04ef0f in WebCore::FrameLoader::clear (this=0x810fe0, clearWindowProperties=true, clearScriptObjects=true) at WebCore/loader/FrameLoader.cpp:803 #9 0x00007fc0ce057eef in WebCore::FrameLoader::begin (this=0x810fe0, url=@0x811190, dispatch=false, origin=0x0) at WebCore/loader/FrameLoader.cpp:898 #10 0x00007fc0ce05bda2 in WebCore::FrameLoader::receivedFirstData (this=0x7fffd7019640) at WebCore/loader/FrameLoader.cpp:848 #11 0x00007fc0ce05c7c8 in WebCore::FrameLoader::setEncoding (this=0x810fe0, name=@0x80e048, userChosen=false) at WebCore/loader/FrameLoader.cpp:1749 #12 0x00007fc0cdb4f335 in WebKit::FrameLoaderClient::committedLoad (this=<value optimized out>, loader=<value optimized out>, data=0xfded60 "<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.0//EN\">\n<html>\n <title>The Acid3 Test (Reference Rendering)</title>\n <style type=\"text/css\">\n html { margin: 0; padding: 0; }\n body { background: #c0c0c0 ur"..., length=1256) at WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:165 #13 0x00007fc0ce03c683 in WebCore::DocumentLoader::commitLoad (this=0xfcfd80, data=0xfded60 "<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.0//EN\">\n<html>\n <title>The Acid3 Test (Reference Rendering)</title>\n <style type=\"text/css\">\n html { margin: 0; padding: 0; }\n body { background: #c0c0c0 ur"..., length=1256) at WebCore/loader/DocumentLoader.cpp:325 #14 0x00007fc0ce089ee2 in WebCore::ResourceLoader::didReceiveData (this=0xfcec10, data=0xfded60 "<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.0//EN\">\n<html>\n <title>The Acid3 Test (Reference Rendering)</title>\n <style type=\"text/css\">\n html { margin: 0; padding: 0; }\n body { background: #c0c0c0 ur"..., length=1256, lengthReceived=0, allAtOnce=false) at WebCore/loader/ResourceLoader.cpp:234 #15 0x00007fc0ce07f4e3 in WebCore::MainResourceLoader::didReceiveData (this=0xfcec10, data=0xfded60 "<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.0//EN\">\n<html>\n <title>The Acid3 Test (Reference Rendering)</title>\n <style type=\"text/css\">\n html { margin: 0; padding: 0; }\n body { background: #c0c0c0 ur"..., length=1256, lengthReceived=0, allAtOnce=160) at WebCore/loader/MainResourceLoader.cpp:296 #16 0x00007fc0ce271057 in writeCallback (ptr=0xfded60, size=<value optimized out>, nmemb=<value optimized out>, data=<value optimized out>) at WebCore/platform/network/curl/ResourceHandleManager.cpp:126 #17 0x00007fc0c844b838 in ?? () from /usr/lib/libcurl-gnutls.so.4 #18 0x00007fc0c8464b79 in ?? () from /usr/lib/libcurl-gnutls.so.4 #19 0x00007fc0c8464d2f in ?? () from /usr/lib/libcurl-gnutls.so.4 #20 0x00007fc0c845e16a in ?? () from /usr/lib/libcurl-gnutls.so.4 #21 0x00007fc0c8462e9c in ?? () from /usr/lib/libcurl-gnutls.so.4 #22 0x00007fc0c846380b in curl_multi_perform () from /usr/lib/libcurl-gnutls.so.4 #23 0x00007fc0ce2759bc in WebCore::ResourceHandleManager::downloadTimerCallback (this=0x897320, timer=<value optimized out>) at WebCore/platform/network/curl/ResourceHandleManager.cpp:308 #24 0x00007fc0ce15e350 in WebCore::TimerBase::fireTimers (fireTime=1206441151.5012569, firingTimers=@0x7fffd701a880) at WebCore/platform/Timer.cpp:347 #25 0x00007fc0ce15e870 in WebCore::TimerBase::sharedTimerFired () at WebCore/platform/Timer.cpp:368 #26 0x00007fc0cdb86492 in timeout_cb () at WebCore/platform/gtk/SharedTimerGtk.cpp:48 #27 0x00007fc0cccd294b in ?? () from /usr/lib/libglib-2.0.so.0 #28 0x00007fc0cccd2222 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0 #29 0x00007fc0cccd54d6 in ?? () from /usr/lib/libglib-2.0.so.0 #30 0x00007fc0cccd5797 in g_main_loop_run () from /usr/lib/libglib-2.0.so.0 #31 0x00007fc0cd2e3ee3 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0 #32 0x00000000004040b8 in main (argc=1, argv=0x7fffd701abc8) at juniper.c:46
Attachments
remove bogus assertion (1.21 KB, patch)
2008-03-26 03:35 PDT, Jasper Bryant-Greene 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Jasper Bryant-Greene
Comment 1 2008-03-26 03:35:59 PDT
Created attachment 20080 [details] remove bogus assertion I'm pretty sure the ASSERT is bogus, because a frame can contain no document under certain circumstances, and because there's an if() test immediately after that protects against the case that the ASSERT is checking for. Also because although the ASSERT trips on the debug build (for example when clicking the reference rendering link on http://acid3.acidtests.org/), the release build works fine in the same circumstances. Thus this patch removes the ASSERT.
Julien Chaffraix
Comment 2 2008-03-27 08:07:18 PDT
(In reply to comment #1) > Created an attachment (id=20080) [edit] > remove bogus assertion > > I'm pretty sure the ASSERT is bogus, because a frame can contain no document > under certain circumstances, and because there's an if() test immediately after > that protects against the case that the ASSERT is checking for. I would be more cautious about removing the ASSERT because first it is never triggered on the Mac port only on Qt/Gtk (as mentionned in http://bugs.webkit.org/show_bug.cgi?id=17984). You should also be carefull as ASSERTs are not enforced on release build so the null check is a necessity for those cases. Finally in bug17984, I showed another way of resolving the bug that does not involve removing the ASSERT (but may lead to memory leaks). > > Also because although the ASSERT trips on the debug build (for example when > clicking the reference rendering link on http://acid3.acidtests.org/), the > release build works fine in the same circumstances. > > Thus this patch removes the ASSERT. > Your patch lacks a test case (it is pretty straightforward as the assertion can be triggered by a page with an iframe and just reloading it).
Julien Chaffraix
Comment 3 2008-03-29 19:26:08 PDT
*** Bug 17984 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 4 2008-04-02 02:33:59 PDT
Comment on attachment 20080 [details] remove bogus assertion r=me
Darin Adler
Comment 5 2008-04-02 09:18:17 PDT
Comment on attachment 20080 [details] remove bogus assertion Clearing review flag because I think Julien is right. The assertion is correct and in fact it's the null check after the assertion that's wrong We should fix the bug instead.
Julien Chaffraix
Comment 6 2008-04-02 11:22:30 PDT
Jasper, the saveDocumentState does try to save frame's information but it is pointless to save frame's information when they are not connected to a Document. The core of the issue is how the frames are released. That's why the gtk port is the only one concerned with this bug. I have said that the Qt port would also fail but it is not longer the case. I could not find exactly when it was fixed. If you want to attack the real issue, I would advise you to have a look at http://trac.webkit.org/projects/webkit/changeset/31521. This is the changeset where Zecke has moved the portion of the code I had hold responsible for the assertion failure (the code in FrameLoaderClient::detachedFromParent4()). The other Qt commits from the same day should give an answer to how and when the frame should be released to avoid both the assertion and a memory leak.
Julien Chaffraix
Comment 7 2008-04-06 13:31:26 PDT
*** Bug 18334 has been marked as a duplicate of this bug. ***
Brady Eidson
Comment 8 2008-04-06 13:42:36 PDT
I thought Frames were always created with "Empty Documents"? Maciej went through significant pains to accomplish that for us, and we spent time cleaning up all the loose ends, but are better off for it. Maybe that empty document creation has pieces reaching into WebKit code and thats why Win/Mac are fine but not Qt/GTK?
Jan Alonzo
Comment 9 2008-04-14 06:31:22 PDT
The patch at bug #18411 fixes this issue so i'll mark this as dependent on that bug. Feel free to remove the dependency if you think otherwise.
Jan Alonzo
Comment 10 2008-04-19 01:55:13 PDT
(In reply to comment #9) > The patch at bug #18411 fixes this issue so i'll mark this as dependent on that > bug. Feel free to remove the dependency if you think otherwise. > This bug seems to have been fixed by the patch at #18411. Closing as FIXED.
Note You need to log in before you can comment on or make changes to this bug.