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
Created attachment 105809 [details] Fix for TouchWebView seg fault I have attached a sample qml file as test case.
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.
Created attachment 105810 [details] sample qml file for touchwebview
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.
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 ?
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 ** **********************************************************************
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.
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?
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.
Created attachment 105832 [details] Took care of review suggestions Thanks, Banjamin and Alexis. I have incorporated the changes you suggested.
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???
> "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
Created attachment 105840 [details] updated patch with more test cases Thanks again
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.
Created attachment 105958 [details] Edge case tests requested by Benjamin Additional test to check edge case
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"
Created attachment 105963 [details] modified change log
Created attachment 105967 [details] simplified changelog based on reviewer comments
Created attachment 105972 [details] All the coverage in one patch
Comment on attachment 105972 [details] All the coverage in one patch great
Comment on attachment 105972 [details] All the coverage in one patch Clearing flags on attachment: 105972 Committed r94313: <http://trac.webkit.org/changeset/94313>
All reviewed patches have been landed. Closing bug.