Bug 77943 – REGRESSION (r106899): broke multiple tests on GTK

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

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

[zalan]

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.

[Kenneth Rohde Christiansen]

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

[zalan]

> 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)

[Philippe Normand]

(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?

[zalan]

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

[Philippe Normand]

(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!

[zalan]

Created attachment 126026 [details]
Patch

[Philippe Normand]

(In reply to comment #7)
> Created an attachment (id=126026) [details]
> Patch

Kenneth, can you please review this patch?

[Kenneth Rohde Christiansen]

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+

[WebKit Review Bot]

Comment on attachment 126026 [details]
Patch

Clearing flags on attachment: 126026

Committed r107135: <http://trac.webkit.org/changeset/107135>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.