Bug 67308 – [Qt] TouchWebView crashes with segmentation fault

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Gopal Raghavan]

Qml debugging is enabled. Only use this in a safe environment!
ASSERTION FAILED: availableWidth > 0 && availableHeight > 0
/home/gopal/wk/wk3/Source/WebCore/dom/ViewportArguments.cpp(51) : WebCore::ViewportAttributes WebCore::computeViewportAttributes(WebCore::ViewportArguments, int, int, int, int, WebCore::IntSize)
Segmentation fault

[Gopal Raghavan]

Created attachment 105809 [details]
Fix for TouchWebView seg fault

I have attached a sample qml file as test case.

[Gopal Raghavan]

If you instantiate TouchWebView element with height and width in a qml file and load a url it fails with segmentation fault. The ASSERTION in ViewportArguments.cpp under computeViewportAttributes seems to be wrong. During first round updateViewportConstraints has non-zero width and 0 as height. As a result availableSize is set to (width,0). Height gets set by subsequent call to setHeight. However, the ASSERT in computeViewportAttributes expects non-zero values for both height and width. Due to sequential nature of setWidth and setHeight call, this ASSERT is not satisfied. This patch fixes the problem by removing the assert.

[Gopal Raghavan]

Created attachment 105810 [details]
sample qml file for touchwebview

[Alexis Menard (darktears)]

Comment on attachment 105809 [details]
Fix for TouchWebView seg fault

View in context: https://bugs.webkit.org/attachment.cgi?id=105809&action=review

This is wrong. The code assume that availableWidth and availableHeight are positive. The problem is not from the assert, it is there for a reason. I believe we can't remove it. Now perhaps the best way is to delayed the updateViewportConstraints call or not call it at all if one of the value is 0. Anyway you patch miss a test case.

> Source/WebCore/ChangeLog:8
> +        If you instantiate TouchWebView element with height and width in a qml file and load a url it fails with segmentation fault. The ASSERTION in ViewportArguments.cpp under computeViewportAttributes seems to be wrong. During first round updateViewportConstraints has non-zero width and 0 as height. As a result availableSize is set to (width,0). Height gets set by subsequent call to setHeight. However, the ASSERT in computeViewportAttributes expects non-zero values for both height and width. Due to sequential nature of setWidth and setHeight call, this ASSERT is not satisfied. This patch fixes the problem by removing the assert. 

Lines are two long. Wrap them.

[Gopal Raghavan]

what is the reason for the assert ?
The other thing I can do is to check if both x,y are non-zero before it calls computeViewportAttributes in QTouchWebViewPrivate::updateViewportConstraints. What do you think ?
What kind of test case can I write for this? Any ideas ?

[Gopal Raghavan]

Created attachment 105827 [details]
updated patch 

Alexis, Thanks for your input. I have made suggested changes and uploaded a patch with test case.

Here is the complete api test result:


********* Start testing of tst_CommonViewTests *********
Config: Using QTest library 5.0.0, Qt 5.0.0
PASS   : tst_CommonViewTests::initTestCase()
PASS   : tst_CommonViewTests::baseUrl()
PASS   : tst_CommonViewTests::loadEmptyUrl()
QWARN  : tst_CommonViewTests::loadEmptyPageViewVisible() QSocketNotifier: socket notifiers cannot be enabled from another thread
QWARN  : tst_CommonViewTests::loadEmptyPageViewVisible() QSocketNotifier: socket notifiers cannot be enabled from another thread
QWARN  : tst_CommonViewTests::loadEmptyPageViewVisible() QObject::startTimer: timers cannot be started from another thread
PASS   : tst_CommonViewTests::loadEmptyPageViewVisible()
QWARN  : tst_CommonViewTests::loadEmptyPageViewHidden() QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : tst_CommonViewTests::loadEmptyPageViewHidden()
PASS   : tst_CommonViewTests::loadNonexistentFileUrl()
QWARN  : tst_CommonViewTests::backAndForward() QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : tst_CommonViewTests::backAndForward()
QWARN  : tst_CommonViewTests::reload() QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : tst_CommonViewTests::reload()
QWARN  : tst_CommonViewTests::stop() QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : tst_CommonViewTests::stop()
PASS   : tst_CommonViewTests::loadProgress()
QWARN  : tst_CommonViewTests::show() QSocketNotifier: socket notifiers cannot be enabled from another thread
QWARN  : tst_CommonViewTests::show() QObject::startTimer: timers cannot be started from another thread
PASS   : tst_CommonViewTests::show()
PASS   : tst_CommonViewTests::cleanupTestCase()
Totals: 12 passed, 0 failed, 0 skipped
********* Finished testing of tst_CommonViewTests *********

********* Start testing of tst_QTouchWebView *********
Config: Using QTest library 5.0.0, Qt 5.0.0
PASS   : tst_QTouchWebView::initTestCase()
PASS   : tst_QTouchWebView::accessPage()
PASS   : tst_QTouchWebView::navigationActionsStatusAtStartup()
PASS   : tst_QTouchWebView::cleanupTestCase()
Totals: 4 passed, 0 failed, 0 skipped
********* Finished testing of tst_QTouchWebView *********

********* Start testing of tst_QDesktopWebView *********
Config: Using QTest library 5.0.0, Qt 5.0.0
PASS   : tst_QDesktopWebView::initTestCase()
PASS   : tst_QDesktopWebView::navigationActionsStatusAtStartup()
QWARN  : tst_QDesktopWebView::stopActionEnabledAfterLoadStarted() QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : tst_QDesktopWebView::stopActionEnabledAfterLoadStarted()
PASS   : tst_QDesktopWebView::cleanupTestCase()
Totals: 4 passed, 0 failed, 0 skipped
********* Finished testing of tst_QDesktopWebView *********

********* Start testing of qmltests *********
Config: Using QTest library 5.0.0, Qt 5.0.0
PASS   : DesktopWebViewNavigationPolicyForUrl::initTestCase()
QWARN  : DesktopWebViewNavigationPolicyForUrl::test_ignorePolicy() QObject::startTimer: timers cannot be started from another thread
PASS   : DesktopWebViewNavigationPolicyForUrl::test_ignorePolicy()
PASS   : DesktopWebViewNavigationPolicyForUrl::test_usePolicy()
PASS   : DesktopWebViewNavigationPolicyForUrl::cleanupTestCase()
PASS   : DesktopWebViewProperties::initTestCase()
PASS   : DesktopWebViewProperties::test_title()
PASS   : DesktopWebViewProperties::test_url()
PASS   : DesktopWebViewProperties::cleanupTestCase()
PASS   : TouchWebViewLoad::initTestCase()
PASS   : TouchWebViewLoad::test_load()
PASS   : TouchWebViewLoad::cleanupTestCase()
PASS   : TouchWebViewProperties::initTestCase()
PASS   : TouchWebViewProperties::test_title()
PASS   : TouchWebViewProperties::test_url()
PASS   : TouchWebViewProperties::cleanupTestCase()
Totals: 15 passed, 0 failed, 0 skipped
********* Finished testing of qmltests *********


**********************************************************************
**              TOTALS: 35 passed, 0 failed, 0 skipped              **
**********************************************************************

[Benjamin Poulain]

Comment on attachment 105827 [details]
updated patch 

View in context: https://bugs.webkit.org/attachment.cgi?id=105827&action=review

Looks like the right solution. Comments:

> Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:67
> +    if (availableSize.width() < 1 || availableSize.height() < 1)
> +        return;
> +

Please use "size <= 0", which is a bit easier to read. Isn't there a QSize::isValid() or something like that?

You should also put that just after availableSize get its value. The span between the variable definition and its use is unnecessary.

[Benjamin Poulain]

Comment on attachment 105827 [details]
updated patch 

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/TouchWebView/tst_load.qml:8
> +    height: 600
> +    width: 400

What about height: 0 and width: 0? Wouldn't that be a more interesting test?

[Alexis Menard (darktears)]

Comment on attachment 105827 [details]
updated patch 

View in context: https://bugs.webkit.org/attachment.cgi?id=105827&action=review

> Source/WebKit2/ChangeLog:9
> +        load a url it fails with segmentation fault. During first round

it crashes (or assert in debug).

> Source/WebKit2/ChangeLog:15
> +        QTouchWebViewPrivate::updateViewportConstraints before calling computeViewportAttributes.

Could you just say. setWidth and setHeight are called sequentially therefore it can happen that computeViewportAttributes was called with a size like (width, 0) breaking the assumption of the function that the size is valid. The patch make sure we compute the viewport when both height and width are valid. Something like that.

[Gopal Raghavan]

Created attachment 105832 [details]
Took care of review suggestions

Thanks, Banjamin and Alexis. I have incorporated the changes you suggested.

[Benjamin Poulain]

Comment on attachment 105832 [details]
Took care of review suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=105832&action=review

Patch looks good but the changelog and the tests can be made better.

> Source/WebKit2/ChangeLog:15
> +        If you instantiate TouchWebView element with height and width in a qml file and
> +        load a url it crashes. During first round
> +        updateViewportConstraints has non-zero width and 0 as height. As a result availableSize
> +        is set to (width,0). Height gets set by subsequent call to setHeight. However, the
> +        ASSERT in computeViewportAttributes expects non-zero values for both height and width.
> +        setWidth and setHeight are called sequentially therefore it can happen that computeViewportAttributes
> +        was called with a size like (width, 0) breaking the assumption of the function that the size is valid. The patch makes sure we compute the viewport when both height and width are valid.
> +

Please rewrap this and rephrase it a bit clearer.

"If you instantiate TouchWebView element with height and width in a qml file and load a url it crashes" -> QTouchWebView can asserts when loading a page if the viewport does not have a valid geometry.

"setWidth and setHeight" -> The functions setWidth() and setHeight() so that sentence is a bit easier to read.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/TouchWebView/tst_load.qml:8
> +    height: 600
> +    width: 400

0, 0???

[Benjamin Poulain]

> "If you instantiate TouchWebView element with height and width in a qml file and load a url it crashes" -> QTouchWebView can asserts when loading a page if the viewport does not have a valid geometry.
> 
> "setWidth and setHeight" -> The functions setWidth() and setHeight() so that sentence is a bit easier to read.

Damn, forgot the "e.g." :) 
Those are just examples, you don't force you to use those sentences.  :-D

[Gopal Raghavan]

Created attachment 105840 [details]
updated patch with more test cases

Thanks again

[Gopal Raghavan]

Created attachment 105957 [details]
splitting patch for easy review

I had promised Benjamin that I would add separate patch for test. Sorry, the previous patch 105840 had both clubbed in. For easier review, I am splitting the patch.
This patch is similar to attachment 105832 [details] that has r+ from Benjamin already.

[Gopal Raghavan]

Created attachment 105958 [details]
Edge case tests requested by Benjamin

Additional test to check edge case

[Benjamin Poulain]

Comment on attachment 105958 [details]
Edge case tests requested by Benjamin

View in context: https://bugs.webkit.org/attachment.cgi?id=105958&action=review

Please add the test in the other patch.

> Source/WebKit2/ChangeLog:7
> +        Additoinal test for edge cases to check behavior with zero sized width and height. 

"Additoinal"

[Gopal Raghavan]

Created attachment 105963 [details]
modified change log

[Gopal Raghavan]

Created attachment 105967 [details]
simplified changelog based on reviewer comments

[Gopal Raghavan]

Created attachment 105972 [details]
All the coverage in one patch

[Benjamin Poulain]

Comment on attachment 105972 [details]
All the coverage in one patch

great

[WebKit Review Bot]

Comment on attachment 105972 [details]
All the coverage in one patch

Clearing flags on attachment: 105972

Committed r94313: <http://trac.webkit.org/changeset/94313>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.