Bug 26983 - tst_qwebpage segfaults
Summary: tst_qwebpage segfaults
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 26886
  Show dependency treegraph
Reported: 2009-07-05 13:01 PDT by Robert Hogan
Modified: 2009-11-09 18:26 PST (History)
4 users (show)

See Also:

bt (7.91 KB, text/plain)
2009-07-05 13:01 PDT, Robert Hogan
no flags Details
patch (1.78 KB, patch)
2009-07-05 13:16 PDT, Robert Hogan
manyoso: review-
Details | Formatted Diff | Diff
Fix the QSize/IntSize mismatch (1.72 KB, patch)
2009-07-14 09:14 PDT, Adam Treat
no flags Details | Formatted Diff | Diff
Fixes the more general problem (2.27 KB, patch)
2009-07-14 10:41 PDT, Adam Treat
no flags Details | Formatted Diff | Diff
Fixes the test (2.26 KB, patch)
2009-11-09 18:25 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2009-07-05 13:01:20 PDT
Created attachment 32280 [details]

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


    RefPtr<FrameView> frameView;
    if (isMainFrame) {
        frameView = FrameView::create(this, viewportSize);
    } else
        frameView = FrameView::create(this);

    frameView->setScrollbarModes(horizontalScrollbarMode, verticalScrollbarMode);


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

    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.
Comment 1 Robert Hogan 2009-07-05 13:16:57 PDT
Created attachment 32281 [details]

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.
Comment 2 Maciej Stachowiak 2009-07-06 00:31:25 PDT
I wonder why this doesn't affect other ports? Perhaps the Qt port is calling Frame methods differently, thus violating their assumptions.
Comment 3 Robert Hogan 2009-07-06 07:27:32 PDT
(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:

    frameView = FrameView::create(this, viewportSize);

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.
Comment 4 Simon Hausmann 2009-07-10 12:39:37 PDT
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.
Comment 5 Adam Treat 2009-07-14 06:11:38 PDT
This behavior was modified in:


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.
Comment 6 Adam Treat 2009-07-14 06:18:59 PDT
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);

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.
Comment 7 Simon Hausmann 2009-07-14 07:22:10 PDT
This assertion is also triggered many times in the Qt DRT
Comment 8 Antonio Gomes 2009-07-14 08:23:21 PDT
as pointed out by simon, while running layout test in debug mode the following tests show the same bt:


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]
Comment 9 Adam Treat 2009-07-14 08:58:31 PDT
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.
Comment 10 Adam Treat 2009-07-14 08:59:15 PDT
Comment on attachment 32281 [details]

Larger issues at work.
Comment 11 Adam Treat 2009-07-14 09:14:46 PDT
Created attachment 32717 [details]
Fix the QSize/IntSize mismatch
Comment 12 Zack Rusin 2009-07-14 09:39:45 PDT
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.
Comment 13 Adam Treat 2009-07-14 09:56:53 PDT
Landed the Qt part and fixed with r45862.
Comment 14 Adam Treat 2009-07-14 10:41:22 PDT
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.
Comment 15 Dave Hyatt 2009-07-14 10:47:10 PDT
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.
Comment 16 Adam Treat 2009-07-14 10:55:01 PDT
Landed with r45863.  All better.
Comment 17 Erik Arvidsson 2009-11-09 18:25:17 PST
Created attachment 42827 [details]
Fixes the test
Comment 18 Erik Arvidsson 2009-11-09 18:26:49 PST
Comment on attachment 42827 [details]
Fixes the test

wrong bug number