Bug 77943 - REGRESSION (r106899): broke multiple tests on GTK
Summary: REGRESSION (r106899): broke multiple tests on GTK
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-07 01:10 PST by Philippe Normand
Modified: 2012-02-08 14:44 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.73 KB, patch)
2012-02-08 01:38 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2012-02-07 01:10:43 PST
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
Comment 1 zalan 2012-02-07 04:00:34 PST
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.
Comment 2 Kenneth Rohde Christiansen 2012-02-07 04:07:30 PST
(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.
Comment 3 zalan 2012-02-07 06:23:13 PST
> 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)
Comment 4 Philippe Normand 2012-02-07 07:03:18 PST
(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?
Comment 5 zalan 2012-02-07 07:07:33 PST
(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.
Comment 6 Philippe Normand 2012-02-07 07:39:18 PST
(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!
Comment 7 zalan 2012-02-08 01:38:29 PST
Created attachment 126026 [details]
Patch
Comment 8 Philippe Normand 2012-02-08 10:36:15 PST
(In reply to comment #7)
> Created an attachment (id=126026) [details]
> Patch

Kenneth, can you please review this patch?
Comment 9 Kenneth Rohde Christiansen 2012-02-08 13:22:54 PST
Comment on attachment 126026 [details]
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+
Comment 10 WebKit Review Bot 2012-02-08 14:44:14 PST
Comment on attachment 126026 [details]
Patch

Clearing flags on attachment: 126026

Committed r107135: <http://trac.webkit.org/changeset/107135>
Comment 11 WebKit Review Bot 2012-02-08 14:44:19 PST
All reviewed patches have been landed.  Closing bug.