Bug 26983 – tst_qwebpage segfaults

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Robert Hogan]

Created attachment 32280 [details]
bt

Running tst_qwebpage gives:

********* Start testing of tst_QWebPage *********
Config: Using QTest library 4.5.0, Qt 4.5.0
PASS   : tst_QWebPage::initTestCase()
PASS   : tst_QWebPage::acceptNavigationRequest()
PASS   : tst_QWebPage::loadFinished()
PASS   : tst_QWebPage::acceptNavigationRequestWithNewWindow()
ASSERTION FAILED: m_frame->view() == this
(../../../WebCore/page/FrameView.cpp:493 void WebCore::FrameView::layout(bool))

The backtrace shows that the Frame is still being created at this point. 
(gdb) bt
bt
#0  0xb7522b58 in WebCore::FrameView::layout (this=0x9f28c28, allowSubtree=true) at ../../../WebCore/page/FrameView.cpp:493
#1  0xb75235b6 in WebCore::FrameView::visibleContentsResized (this=0x9f28c28) at ../../../WebCore/page/FrameView.h:215
#2  0xb759a306 in WebCore::ScrollView::updateScrollbars (this=0x9f28c28, desiredOffset=@0xbffc18c4) at ../../../WebCore/platform/ScrollView.cpp:353
#3  0xb759b4ad in WebCore::ScrollView::setFixedLayoutSize (this=0x9f28c28, newSize=@0xbffc1994) at ../../../WebCore/platform/ScrollView.cpp:199
#4  0xb7515923 in WebCore::Frame::createView (this=0x9e5c9d8, viewportSize=@0xbffc19a4, backgroundColor=@0xbffc199c, transparent=false, fixedLayoutSize=@0xbffc1994, useFixedLayout=false, horizontalScrollbarMode=WebCore::ScrollbarAuto, verticalScrollbarMode=WebCore::ScrollbarAuto) at ../../../WebCore/page/Frame.cpp:1757

In fact createView() calls setFixedLayoutSize() before it calls setView(frameView):

    setView(0);

    RefPtr<FrameView> frameView;
    if (isMainFrame) {
        frameView = FrameView::create(this, viewportSize);
        frameView->setFixedLayoutSize(fixedLayoutSize);
        frameView->setUseFixedLayout(useFixedLayout);
    } else
        frameView = FrameView::create(this);

    frameView->setScrollbarModes(horizontalScrollbarMode, verticalScrollbarMode);
    frameView->updateDefaultScrollbarState();

    setView(frameView);

Calling setView(frameView) before the layout calls causes numerous other cases in the qwebpage unit test to fail and times out the one that segfaults. So the answer seems to be to cater for this specific call chain in FrameView::layout():

    /* The call to setFixedLayoutSize() in Frame::createView can bring us here, and at that point the
       frame's view is still 0. */
    if (!m_frame->view())
        return;

    ASSERT(m_frame->view() == this);

My patch removes the comment and if statement from http://trac.webkit.org/changeset/29878 because it now appears to be redundant. The ASSERT above that it was intended to replace is currently un-commented in the code.

[Robert Hogan]

Created attachment 32281 [details]
patch

Decided against removing the comment and return statement from http://trac.webkit.org/changeset/29878. 

The assert makes the return redundant but maybe the rdar issue is still open.

[Maciej Stachowiak]

I wonder why this doesn't affect other ports? Perhaps the Qt port is calling Frame methods differently, thus violating their assumptions.

[Robert Hogan]

(In reply to comment #2)
> I wonder why this doesn't affect other ports? Perhaps the Qt port is calling
> Frame methods differently, thus violating their assumptions.

I'm not sure this is the case here. The sequence of events in WebCore::Frame::createView is:


    setView(0); 
    frameView = FrameView::create(this, viewportSize);
    frameView->setFixedLayoutSize(fixedLayoutSize);
    frameView->setUseFixedLayout(useFixedLayout);
    ...
    setView(frameView);

So the Frames m_view gets set to 0, a new view is created, and then various layout calls are made on it. It is during the call to setUseFixedLayout that WebCore::FrameView::layout fails on an ASSERT (the backtrace shows there are no intermediate calls to the qt port). This is because setView(frameView) has not yet been called on the frame.

So as you can see, the assumptions all seem to be contained within WebCore::Frame::createView. I haven't done enough research to know why this situation doesn't crop up more often.

I experimented with moving setView(frameView) to right after FrameView::create but it resulted in the segfaulting test case timing out, and introduced failures in others. So it seems to be in its current position for good reason.

[Simon Hausmann]

This could be a quirk in the fixed layout size implementation. However I think if this turns out to be the correct fix, then it would be best to have it covered by a layout test for all platforms. After all the fixed layout size looks like a cross-platform feature.

This may however require adding new interfaces to DumpRenderTree.

[Adam Treat]

This behavior was modified in:

https://bugs.webkit.org/show_bug.cgi?id=25125

Before this change, setFixedLayoutSize did not explicitly cause a layout.

As for why this is hitting the Qt port and not others?  Guessing: we're probably doing something different in our test case that causes a 'needsLayout.'  Not sure what.  However, I think for robustness this should be fixed as Robert's analysis looks correct.  It is a happy coincidence that other ports aren't running into this.

[Adam Treat]

I would add that this is not just a problem with setFixedLayout.  As a little bit down in Frame::createView() you can see:

    frameView->setScrollbarModes(horizontalScrollbarMode, verticalScrollbarMode);
    ...
    setView(frameView);

The call to 'setScrollbarModes' can also cause a layout now as it calls 'updateScrollbars' which in turn calls 'visibleContentsResized' which can call 'layout()'.  All with the frame's current view set to 0.

[Simon Hausmann]

This assertion is also triggered many times in the Qt DRT

[Antonio Gomes]

as pointed out by simon, while running layout test in debug mode the following tests show the same bt:


fast/dom/location-new-window-no-crash.html
fast/dom/Document/early-document-access.html
fast/dom/Window/closure-access-after-navigation-window.html
fast/dom/Window/dom-access-from-closure-window.html
fast/dom/Window/setting-properties-on-closed-window.html
fast/dom/Window/window-early-properties.html
fast/dom/Window/window-open-pending-url.html
fast/history/history_reload.html
storage/domstorage/localstorage/window-open.html
storage/domstorage/sessionstorage/window-open.html



ASSERTION FAILED: m_frame->view() == this
(../../../../WebCore/page/FrameView.cpp:490 void WebCore::FrameView::layout(bool))
Segmentation fault
0: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/bin/DumpRenderTree [0x8053486]
1: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/bin/DumpRenderTree [0x8053899]
2: /lib/tls/i686/cmov/libc.so.6 [0xb4b97778]
3: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore9FrameView6layoutEb+0x1a2) [0xb70a1c0a]
4: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore9FrameView22visibleContentsResizedEv+0x34) [0xb70a2636]
5: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore10ScrollView16updateScrollbarsERKNS_7IntSizeE+0x140) [0xb711b95c]
6: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore10ScrollView18setFixedLayoutSizeERKNS_7IntSizeE+0x75) [0xb711cb1b]
7: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore5Frame10createViewERKNS_7IntSizeERKNS_5ColorEbS3_bNS_13ScrollbarModeES7_+0x1f3) [0xb70946fb]
8: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore19FrameLoaderClientQt31transitionToCommittedForNewPageEv+0x265) [0xb72e6999]
9: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore11FrameLoader21transitionToCommittedEN3WTF10PassRefPtrINS_10CachedPageEEE+0x3c8) [0xb70102ba]
10: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore11FrameLoader21commitProvisionalLoadEN3WTF10PassRefPtrINS_10CachedPageEEE+0x213) [0xb7011005]
11: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore14DocumentLoader13commitIfReadyEv+0x6c) [0xb6fe6f86]
12: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore14DocumentLoader10commitLoadEPKci+0x31) [0xb6fea455]
13: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore14DocumentLoader12receivedDataEPKci+0x58) [0xb6fea518]
14: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore11FrameLoader12receivedDataEPKci+0x35) [0xb6fff3d5]
15: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore18MainResourceLoader7addDataEPKcib+0x5c) [0xb702cc26]
16: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore14ResourceLoader14didReceiveDataEPKcixb+0x60) [0xb70345c0]
17: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore18MainResourceLoader14didReceiveDataEPKcixb+0x184) [0xb702b970]
18: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore14ResourceLoader14didReceiveDataEPNS_14ResourceHandleEPKcii+0x3e) [0xb70334cc]
19: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore20QNetworkReplyHandler11forwardDataEv+0x120) [0xb72ba154]
20: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/lib/libQtWebKit.so.4(_ZN7WebCore20QNetworkReplyHandler11qt_metacallEN11QMetaObject4CallEiPPv+0x92) [0xb72bbe7c]
21: /usr/lib/libQtCore.so.4(_ZN14QMetaCallEvent13placeMetaCallEP7QObject+0x34) [0xb4fb366e]
22: /usr/lib/libQtCore.so.4(_ZN7QObject5eventEP6QEvent+0x121) [0xb4fb8985]
23: /usr/lib/libQtGui.so.4(_ZN19QApplicationPrivate13notify_helperEP7QObjectP6QEvent+0x17f) [0xb5237f37]
24: /usr/lib/libQtGui.so.4(_ZN12QApplication6notifyEP7QObjectP6QEvent+0x352) [0xb52382ac]
25: /usr/lib/libQtCore.so.4(_ZN16QCoreApplication14notifyInternalEP7QObjectP6QEvent+0xa8) [0xb4fa0ad4]
26: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/bin/DumpRenderTree(_ZN16QCoreApplication9sendEventEP7QObjectP6QEvent+0x3e) [0x805277e]
27: /usr/lib/libQtCore.so.4(_ZN23QCoreApplicationPrivate16sendPostedEventsEP7QObjectiP11QThreadData+0x358) [0xb4fa105e]
28: /usr/lib/libQtCore.so.4(_ZN16QCoreApplication16sendPostedEventsEP7QObjecti+0x33) [0xb4fa12f3]
29: /usr/lib/libQtGui.so.4(_ZN16QCoreApplication16sendPostedEventsEv+0x26) [0xb5305cf6]
30: /usr/lib/libQtCore.so.4 [0xb4fd8b82]
31: /usr/lib/libglib-2.0.so.0(g_main_context_dispatch+0x1e8) [0xb475eb88]
32: /usr/lib/libglib-2.0.so.0 [0xb47620eb]
33: /usr/lib/libglib-2.0.so.0(g_main_context_iteration+0x68) [0xb4762268]
34: /usr/lib/libQtCore.so.4(_ZN20QEventDispatcherGlib13processEventsE6QFlagsIN10QEventLoop17ProcessEventsFlagEE+0xb4) [0xb4fd7d66]
35: /usr/lib/libQtGui.so.4 [0xb5304174]
36: /usr/lib/libQtCore.so.4(_ZN10QEventLoop13processEventsE6QFlagsINS_17ProcessEventsFlagEE+0xb0) [0xb4f9d0f0]
37: /usr/lib/libQtCore.so.4(_ZN10QEventLoop4execE6QFlagsINS_17ProcessEventsFlagEE+0x129) [0xb4f9d34b]
38: /usr/lib/libQtCore.so.4(_ZN16QCoreApplication4execEv+0x12f) [0xb4fa1429]
39: /usr/lib/libQtGui.so.4(_ZN12QApplication4execEv+0x24) [0xb5237c50]
40: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/bin/DumpRenderTree [0x8053237]
41: /lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe5) [0xb4b82775]
42: /home/agomes/webkit/staikos/webkit/WebKitBuild/Qt/Debug/bin/DumpRenderTree [0x804e711]

[Adam Treat]

Ok, I know the problem.

setFixedLayoutSize is called with a different value than the default because WebCore::IntSize's default constructed value is valid: 0x0.  Qt's QSize default constructed value is invalid: -1x-1.  Thus, a mismatch occurs and it setFixedLayoutSize tries to do a layout.

This only happens with Qt port because other ports use IntSize(0x0) which is the same default constructed.

However, it is important to note that if Frame::createView(...) was called with different values than the default ScrollView constructed values for scrollbarModes for instance, this same crash would happen.

I'm producing a patch for the QSize/IntSize problem, but the larger issue should still be fixed.

[Adam Treat]

Comment on attachment 32281 [details]
patch

Larger issues at work.

[Adam Treat]

Created attachment 32717 [details]
Fix the QSize/IntSize mismatch

[Zack Rusin]

Comment on attachment 32717 [details]
Fix the QSize/IntSize mismatch

Personally I'd prefer this in IntSizeQt.cpp but the issue is that it's impossible to detect whether it's QSize() or QSize(-1, -1). We'd like to convert the first one to QSize(0, 0) but not the latter (because it will mess up conversions after doing any kind of subtraction of IntSizes). So I think this will do for now.

[Adam Treat]

Landed the Qt part and fixed with r45862.

[Adam Treat]

Created attachment 32721 [details]
Fixes the more general problem

I made a check in the new FrameView::visibleContentsResized method to ensure the view is attached before layouting.  I think this is a better place for it so we can still keep the ASSERT in layout() which proved to be valuable.

[Dave Hyatt]

Comment on attachment 32721 [details]
Fixes the more general problem

Just make it a null check instead rather than checking for inequality with this.

r=me with that change.

[Adam Treat]

Landed with r45863.  All better.

[Erik Arvidsson]

Created attachment 42827 [details]
Fixes the test

[Erik Arvidsson]

Comment on attachment 42827 [details]
Fixes the test

wrong bug number