18064 2008-03-25 03:34:29 -0700 assert-fail in WebCore::FrameLoader::saveDocumentState (document is null) 2008-04-19 01:55:13 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) PC Linux RESOLVED FIXED http://acid3.acidtests.org/reference.html Gtk P2 Normal --- 18411 1 jasper webkit-unassigned jchaffraix jmalonzo xan.lopez oldest_to_newest 75092 0 jasper 2008-03-25 03:34:29 -0700 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 75286 1 20080 jasper 2008-03-26 03:35:59 -0700 Created attachment 20080 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. 75460 2 jchaffraix 2008-03-27 08:07:18 -0700 (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). 75802 3 jchaffraix 2008-03-29 19:26:08 -0700 *** Bug 17984 has been marked as a duplicate of this bug. *** 76129 4 20080 darin 2008-04-02 02:33:59 -0700 Comment on attachment 20080 remove bogus assertion r=me 76153 5 20080 darin 2008-04-02 09:18:17 -0700 Comment on attachment 20080 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. 76169 6 jchaffraix 2008-04-02 11:22:30 -0700 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. 76444 7 jchaffraix 2008-04-06 13:31:26 -0700 *** Bug 18334 has been marked as a duplicate of this bug. *** 76446 8 beidson 2008-04-06 13:42:36 -0700 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? 77551 9 jmalonzo 2008-04-14 06:31:22 -0700 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. 78245 10 jmalonzo 2008-04-19 01:55:13 -0700 (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. 20080 2008-03-26 03:35:59 -0700 2008-04-02 09:18:17 -0700 remove bogus assertion FrameLoader-saveDocumentState-remove-bogus-assertion.patch text/plain 1241 jasper SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzMTMxMSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMDgtMDMtMjYgIEphc3BlciBCcnlhbnQtR3JlZW5lICA8amFzcGVy QHVuaXguZ2Vlay5uej4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBGaXhlZCAiYXNzZXJ0LWZhaWwgaW4gV2ViQ29yZTo6RnJhbWVMb2FkZXI6OnNhdmVE b2N1bWVudFN0YXRlIChkb2N1bWVudCBpcyBudWxsKSIKKyAgICAgICAgaHR0cDovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgwNjQKKworICAgICAgICBSZW1vdmVkIGJvZ3VzIGFz c2VydGlvbi4KKworICAgICAgICAqIGxvYWRlci9GcmFtZUxvYWRlci5jcHA6CisgICAgICAgIChX ZWJDb3JlOjpGcmFtZUxvYWRlcjo6c2F2ZURvY3VtZW50U3RhdGUpOgorCiAyMDA4LTAzLTI0ICBN YWNpZWogU3RhY2hvd2lhayAgPG1qc0BhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkg RXJpYy4KSW5kZXg6IFdlYkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBXZWJDb3JlL2xvYWRlci9GcmFtZUxvYWRlci5jcHAJKHJldmlzaW9uIDMxMzA5KQorKysgV2Vi Q29yZS9sb2FkZXIvRnJhbWVMb2FkZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00MTM4LDggKzQx MzgsNiBAQCB2b2lkIEZyYW1lTG9hZGVyOjpzYXZlRG9jdW1lbnRTdGF0ZSgpCiAgICAgICAgIHJl dHVybjsKIAogICAgIERvY3VtZW50KiBkb2N1bWVudCA9IG1fZnJhbWUtPmRvY3VtZW50KCk7Ci0g ICAgQVNTRVJUKGRvY3VtZW50KTsKLSAgICAKICAgICBpZiAoZG9jdW1lbnQgJiYgaXRlbS0+aXND dXJyZW50RG9jdW1lbnQoZG9jdW1lbnQpKSB7CiAgICAgICAgIExPRyhMb2FkaW5nLCAiV2ViQ29y ZUxvYWRpbmcgJXM6IHNhdmluZyBmb3JtIHN0YXRlIHRvICVwIiwgbV9mcmFtZS0+dHJlZSgpLT5u YW1lKCkuc3RyaW5nKCkudXRmOCgpLmRhdGEoKSwgaXRlbSk7CiAgICAgICAgIGl0ZW0tPnNldERv Y3VtZW50U3RhdGUoZG9jdW1lbnQtPmZvcm1FbGVtZW50c1N0YXRlKCkpOwo=