77943 2012-02-07 01:10:43 -0800 REGRESSION (r106899): broke multiple tests on GTK 2012-02-08 14:44:19 -0800 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 pnormand zalan kenneth wangxianzhu webkit.review.bot zalan oldest_to_newest 550700 0 pnormand 2012-02-07 01:10:43 -0800 A bunch for fast/ tests now crash: Program terminated with signal 11, Segmentation fault. #0 0x00002b92ff4bf983 in WebCore::Document::viewportArguments (this=0x0) at ../../Source/WebCore/dom/Document.h:326 326 ViewportArguments viewportArguments() const { return m_viewportArguments; } Thread 1 (Thread 0x2b930cbf1a40 (LWP 21706)): #0 0x00002b92ff4bf983 in WebCore::Document::viewportArguments (this=0x0) at ../../Source/WebCore/dom/Document.h:326 #1 0x00002b92ff4e21a5 in webkitViewportAttributesRecompute (viewportAttributes=0x1b37700) at ../../Source/WebKit/gtk/webkit/webkitviewportattributes.cpp:535 #2 0x00002b92ff4b1122 in WebKit::ChromeClient::dispatchViewportPropertiesDidChange (this=0x2693cc0, arguments=...) at ../../Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:838 #3 0x00002b92ffcc1236 in WebCore::Chrome::dispatchViewportPropertiesDidChange (this=0x26b94e0, arguments=...) at ../../Source/WebCore/page/Chrome.cpp:484 #4 0x00002b92ff856376 in WebCore::Document::updateViewportArguments (this=0x24b9ab0) at ../../Source/WebCore/dom/Document.cpp:2803 #5 0x00002b92ff85b032 in WebCore::Document::setInPageCache (this=0x24b9ab0, flag=false) at ../../Source/WebCore/dom/Document.cpp:4081 #6 0x00002b92ffc3327f in WebCore::FrameLoader::open (this=0x26bc2c8, cachedFrame=...) at ../../Source/WebCore/loader/FrameLoader.cpp:2034 #7 0x00002b92ff9e3ae5 in WebCore::CachedFrame::open (this=0x26ae210) at ../../Source/WebCore/history/CachedFrame.cpp:208 #8 0x00002b92ff9e5585 in WebCore::CachedPage::restore (this=0x26947b0, page=0x26ba090) at ../../Source/WebCore/history/CachedPage.cpp:79 #9 0x00002b92ffc321a7 in WebCore::FrameLoader::commitProvisionalLoad (this=0x26bc2c8) at ../../Source/WebCore/loader/FrameLoader.cpp:1782 #10 0x00002b92ffc379b2 in WebCore::FrameLoader::loadProvisionalItemFromCachedPage (this=0x26bc2c8) at ../../Source/WebCore/loader/FrameLoader.cpp:3034 #11 0x00002b92ffc36d2e in WebCore::FrameLoader::continueLoadAfterNavigationPolicy (this=0x26bc2c8, formState=..., shouldContinue=true) at ../../Source/WebCore/loader/FrameLoader.cpp:2905 #12 0x00002b92ffc364bc in WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy (argument=0x26bc2c8, request=..., formState=..., shouldContinue=true) at ../../Source/WebCore/loader/FrameLoader.cpp:2782 #13 0x00002b92ffc6de73 in WebCore::PolicyChecker::checkNavigationPolicy (this=0x26bc2d8, request=..., loader=0x26c5790, formState=..., function=0x2b92ffc36466 <WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy(void*, WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>, argument=0x26bc2c8) at ../../Source/WebCore/loader/PolicyChecker.cpp:69 #14 0x00002b92ffc30311 in WebCore::FrameLoader::loadWithDocumentLoader (this=0x26bc2c8, loader=0x26c5790, type=WebCore::FrameLoadTypeIndexedBackForward, prpFormState=...) at ../../Source/WebCore/loader/FrameLoader.cpp:1385 #15 0x00002b92ffc37e09 in WebCore::FrameLoader::loadDifferentDocumentItem (this=0x26bc2c8, item=0x2663fa0, loadType=WebCore::FrameLoadTypeIndexedBackForward) at ../../Source/WebCore/loader/FrameLoader.cpp:3092 #16 0x00002b92ffc384d3 in WebCore::FrameLoader::loadItem (this=0x26bc2c8, item=0x2663fa0, loadType=WebCore::FrameLoadTypeIndexedBackForward) at ../../Source/WebCore/loader/FrameLoader.cpp:3178 #17 0x00002b92ffc46b35 in WebCore::HistoryController::recursiveGoToItem (this=0x26bc528, item=0x2663fa0, fromItem=0x24d7c30, type=WebCore::FrameLoadTypeIndexedBackForward) at ../../Source/WebCore/loader/HistoryController.cpp:729 #18 0x00002b92ffc44aba in WebCore::HistoryController::goToItem (this=0x26bc528, targetItem=0x2663fa0, type=WebCore::FrameLoadTypeIndexedBackForward) at ../../Source/WebCore/loader/HistoryController.cpp:273 #19 0x00002b92ffd29cbe in WebCore::Page::goToItem (this=0x26ba090, item=0x2663fa0, type=WebCore::FrameLoadTypeIndexedBackForward) at ../../Source/WebCore/page/Page.cpp:383 #20 0x00002b92ffd29c01 in WebCore::Page::goBackOrForward (this=0x26ba090, distance=-1) at ../../Source/WebCore/page/Page.cpp:371 #21 0x00002b92ff9de6e5 in WebCore::BackForwardController::goBackOrForward (this=0x26956c0, distance=-1) at ../../Source/WebCore/history/BackForwardController.cpp:59 #22 0x00002b92ffc73710 in WebCore::ScheduledHistoryNavigation::fire (this=0x261db90, frame=0x26bc210) at ../../Source/WebCore/loader/NavigationScheduler.cpp:205 #23 0x00002b92ffc725b9 in WebCore::NavigationScheduler::timerFired (this=0x26bc700) at ../../Source/WebCore/loader/NavigationScheduler.cpp:418 #24 0x00002b92ffc74798 in WebCore::Timer<WebCore::NavigationScheduler>::fired (this=0x26bc708) at ../../Source/WebCore/platform/Timer.h:100 #25 0x00002b92ffe5c8e0 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x10bc4c0) at ../../Source/WebCore/platform/ThreadTimers.cpp:115 #26 0x00002b92ffe5c817 in WebCore::ThreadTimers::sharedTimerFired () at ../../Source/WebCore/platform/ThreadTimers.cpp:93 #27 0x00002b93007cb4e6 in WebCore::timeout_cb () at ../../Source/WebCore/platform/gtk/SharedTimerGtk.cpp:49 #28 0x00002b930457cbbb in g_timeout_dispatch () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0 #29 0x00002b930457adf3 in g_main_dispatch () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0 #30 0x00002b930457bab9 in g_main_context_dispatch () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0 #31 0x00002b930457bca3 in g_main_context_iterate () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0 #32 0x00002b930457c0d9 in g_main_loop_run () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0 #33 0x00002b930321ce99 in gtk_main () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libgtk-3.so.0 #34 0x0000000000435326 in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:700 #35 0x00000000004349c0 in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:491 #36 0x00000000004378ec in main (argc=2, argv=0x7fffbfde5608) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1374 550813 1 zalan 2012-02-07 04:00:34 -0800 We might need to move updateViewportArguments() call from setInPageCache() to some other function. When setInPageCache(false) is called, both the document and the view are still detached from the mainframe. Update should probably be dispatched, when the page has finished being restored and not while it is in transition. It would ensure, that calls like gtk has (webView->priv->corePage->mainFrame()->document()->viewportArguments();) will refer to the same document instance. 550818 2 kenneth 2012-02-07 04:07:30 -0800 (In reply to comment #1) > We might need to move updateViewportArguments() call from setInPageCache() to some other function. When setInPageCache(false) is called, both the document and the view are still detached from the mainframe. Update should probably be dispatched, when the page has finished being restored and not while it is in transition. It would ensure, that calls like gtk has (webView->priv->corePage->mainFrame()->document()->viewportArguments();) will refer to the same document instance. It still would need to be done before we are starting to layout. #4 0x00002b92ff856376 in WebCore::Document::updateViewportArguments (this=0x24b9ab0) at ../../Source/WebCore/dom/Document.cpp:2803 If this is the new document, it might be possible to pass it to dispatchViewportPropertiesDidChange but that of course requires that it has up to date viewport info. 550930 3 zalan 2012-02-07 06:23:13 -0800 > It still would need to be done before we are starting to layout. Agree. Layout is forced on the view, soon after the cached frame is all set with the restore. See at FrameLoader::commitProvisionalLoad(). The following change could work, where updateViewportArguments() is moved to Document::documentDidResumeFromPageCache() callback. Index: Source/WebCore/dom/Document.cpp =================================================================== --- Source/WebCore/dom/Document.cpp (revision 106913) +++ Source/WebCore/dom/Document.cpp (working copy) @@ -4078,8 +4078,6 @@ setRenderer(m_savedRenderer); m_savedRenderer = 0; - updateViewportArguments(); - if (childNeedsStyleRecalc()) scheduleStyleRecalc(); } @@ -4120,6 +4118,8 @@ ASSERT(m_frame); m_frame->loader()->client()->dispatchDidBecomeFrameset(isFrameSet()); + + updateViewportArguments(); } void Document::registerForPageCacheSuspensionCallbacks(Element* e) 550952 4 pnormand 2012-02-07 07:03:18 -0800 (In reply to comment #3) > > It still would need to be done before we are starting to layout. > Agree. Layout is forced on the view, soon after the cached frame is all set with the restore. See at FrameLoader::commitProvisionalLoad(). > The following change could work, where updateViewportArguments() is moved to Document::documentDidResumeFromPageCache() callback. > Can we give this patch a try? 550956 5 zalan 2012-02-07 07:07:33 -0800 (In reply to comment #4) > (In reply to comment #3) > > > It still would need to be done before we are starting to layout. > > Agree. Layout is forced on the view, soon after the cached frame is all set with the restore. See at FrameLoader::commitProvisionalLoad(). > > The following change could work, where updateViewportArguments() is moved to Document::documentDidResumeFromPageCache() callback. > > > > Can we give this patch a try? yes, please and if works i'll make it a proper patch. 550973 6 pnormand 2012-02-07 07:39:18 -0800 (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > > It still would need to be done before we are starting to layout. > > > Agree. Layout is forced on the view, soon after the cached frame is all set with the restore. See at FrameLoader::commitProvisionalLoad(). > > > The following change could work, where updateViewportArguments() is moved to Document::documentDidResumeFromPageCache() callback. > > > > > > > Can we give this patch a try? > > yes, please and if works i'll make it a proper patch. I confirm it works. Please send a proper patch :) Thanks a bunch! 551886 7 126026 zalan 2012-02-08 01:38:29 -0800 Created attachment 126026 Patch 552274 8 pnormand 2012-02-08 10:36:15 -0800 (In reply to comment #7) > Created an attachment (id=126026) [details] > Patch Kenneth, can you please review this patch? 552432 9 126026 kenneth 2012-02-08 13:22:54 -0800 Comment on attachment 126026 Patch I would love a test for this :-) To make sure that the viewport arguments are emitted when resuming from the page cache. Zalan can you make such a test? I will r=me this for now as it breaks GTK+ 552507 10 126026 webkit.review.bot 2012-02-08 14:44:14 -0800 Comment on attachment 126026 Patch Clearing flags on attachment: 126026 Committed r107135: <http://trac.webkit.org/changeset/107135> 552508 11 webkit.review.bot 2012-02-08 14:44:19 -0800 All reviewed patches have been landed. Closing bug. 126026 2012-02-08 01:38:29 -0800 2012-02-08 14:44:14 -0800 Patch bug-77943-20120208103827.patch text/plain 1773 zalan SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwNzA1MikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBACisyMDEyLTAyLTA4ICBaYWxhbiBC dWp0YXMgIDx6YnVqdGFzQGdtYWlsLmNvbT4KKworICAgICAgICBEaXNwYXRjaCB1cGRhdGVWaWV3 cG9ydEFyZ3VtZW50cygpLCB3aGVuIERvY3VtZW50IGlzIGZpbmlzaGVkCisgICAgICAgIHJlc3Rv cmluZyBmcm9tIHBhZ2UgY2FjaGUuCisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTc3OTQzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgTW92ZSB1cGRhdGVWaWV3cG9ydEFyZ3VtZW50cygpIGNhbGwgZnJvbSBz ZXRQYWdlSW5DYWNoZSgpIHRvCisgICAgICAgIGRvY3VtZW50RGlkUmVzdW1lRnJvbVBhZ2VDYWNo ZSgpIHRvIGVuc3VyZSwgdGhhdCB0aGUgRG9jdW1lbnQgaXMKKyAgICAgICAgZnVsbHkgcmVzdW1l ZCBmcm9tIHRoZSBwYWdlIGNhY2hlIGFuZCBhdHRhY2hlZCB0byB0aGUgbWFpbmZyYW1lLAorICAg ICAgICB3aGVuIHRoZSB2aWV3cG9ydCBhcmd1bWVudHMgYXJlIHVwZGF0ZWQuCisKKyAgICAgICAg Tm8gdGVzdHMuIE5vIGNoYW5nZSBpbiBiZWhhdmlvdXIuCisKKyAgICAgICAgKiBkb20vRG9jdW1l bnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RG9jdW1lbnQ6OnNldEluUGFnZUNhY2hlKToKKyAg ICAgICAgKFdlYkNvcmU6OkRvY3VtZW50Ojpkb2N1bWVudERpZFJlc3VtZUZyb21QYWdlQ2FjaGUp OgorCiAyMDEyLTAyLTA4ICBLZW50YXJvIEhhcmEgIDxoYXJha2VuQGNocm9taXVtLm9yZz4KIAog ICAgICAgICBSZW5hbWUgW0RlbGVnYXRpbmdQdXRGdW5jdGlvbl0gSURMIHRvIFtDdXN0b21OYW1l ZFNldHRlcl0gSURMCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9kb20vRG9jdW1lbnQuY3BwCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2RvbS9Eb2N1bWVudC5jcHAJKHJldmlzaW9uIDEwNzA0 NikKKysrIFNvdXJjZS9XZWJDb3JlL2RvbS9Eb2N1bWVudC5jcHAJKHdvcmtpbmcgY29weSkKQEAg LTQwNzgsOCArNDA3OCw2IEBAIHZvaWQgRG9jdW1lbnQ6OnNldEluUGFnZUNhY2hlKGJvb2wgZmxh ZykKICAgICAgICAgc2V0UmVuZGVyZXIobV9zYXZlZFJlbmRlcmVyKTsKICAgICAgICAgbV9zYXZl ZFJlbmRlcmVyID0gMDsKIAotICAgICAgICB1cGRhdGVWaWV3cG9ydEFyZ3VtZW50cygpOwotCiAg ICAgICAgIGlmIChjaGlsZE5lZWRzU3R5bGVSZWNhbGMoKSkKICAgICAgICAgICAgIHNjaGVkdWxl U3R5bGVSZWNhbGMoKTsKICAgICB9CkBAIC00MTIwLDYgKzQxMTgsOCBAQCB2b2lkIERvY3VtZW50 Ojpkb2N1bWVudERpZFJlc3VtZUZyb21QYWdlCiAKICAgICBBU1NFUlQobV9mcmFtZSk7CiAgICAg bV9mcmFtZS0+bG9hZGVyKCktPmNsaWVudCgpLT5kaXNwYXRjaERpZEJlY29tZUZyYW1lc2V0KGlz RnJhbWVTZXQoKSk7CisKKyAgICB1cGRhdGVWaWV3cG9ydEFyZ3VtZW50cygpOwogfQogCiB2b2lk IERvY3VtZW50OjpyZWdpc3RlckZvclBhZ2VDYWNoZVN1c3BlbnNpb25DYWxsYmFja3MoRWxlbWVu dCogZSkK