Bug 79119

Summary: [Qt][WK2] Infinite loop on history navigation, when panning.
Product: WebKit Reporter: zalan <zalan>
Component: WebKit QtAssignee: Andras Becsi <abecsi>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, hausmann, kenneth, menard, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 80029, 80425    
Bug Blocks:    
Attachments:
Description Flags
proposed patch
none
proposed patch with corrected style
none
proposed patch
none
proposed patch
none
updated patch none

Description zalan 2012-02-21 08:57:39 PST
Looks like Minibrowser gets into an infinite loop, when panning while navigating in the back-forward history. It's not reproducable every time, but it happens within 10 attempts.
steps to reproduce:
load at least 2 pages (news.google.com, cnn.com)
navigate between them using back/forward arrows.
start panning right after, the history navigation kicks in.

Minibrowser ends up producing output like this:

MiniBrowser: Going backward in session history.
MiniBrowser: Going backward in session history.
MiniBrowser: Going backward in session history.
MiniBrowser: Going backward in session history.
MiniBrowser: Going backward in session history.

with the following backtrace:

#0  QQuickWebView::goBack (this=0x237f0e0) at ../../../../Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1107
#1  0x00007fa39352f299 in QQuickWebView::qt_static_metacall (_o=0x237f0e0, _c=QMetaObject::InvokeMetaMethod, _id=13, _a=0x7fff0f4cb5f0) at moc/moc_qquickwebview_p.cpp:146
#2  0x00007fa39352f8a8 in QQuickWebView::qt_metacall (this=0x237f0e0, _c=QMetaObject::InvokeMetaMethod, _id=13, _a=0x7fff0f4cb5f0) at moc/moc_qquickwebview_p.cpp:292
#3  0x00007fa3911d9d16 in QDeclarativeProxyMetaObject::metaCall (this=0x2376db0, c=QMetaObject::InvokeMetaMethod, id=46, a=0x7fff0f4cb5f0) at qml/qdeclarativeproxymetaobject.cpp:121
#4  0x00007fa39010697c in QMetaObject::metacall (object=0x237f0e0, cl=QMetaObject::InvokeMetaMethod, idx=46, argv=0x7fff0f4cb5f0) at kernel/qmetaobject.cpp:246
#5  0x00007fa3912f4e07 in CallMethod (object=0x237f0e0, index=46, returnType=0, argCount=0, argTypes=0x0, engine=0x220b640, callArgs=...) at qml/v8/qv8qobjectwrapper.cpp:1495
#6  0x00007fa3912f58ff in CallPrecise (object=0x237f0e0, data=..., engine=0x220b640, callArgs=...) at qml/v8/qv8qobjectwrapper.cpp:1708
#7  0x00007fa3912f6941 in QV8QObjectWrapper::Invoke (args=...) at qml/v8/qv8qobjectwrapper.cpp:1910
#8  0x00007fa38c653bab in v8::internal::HandleApiCallHelper<false> (args=..., isolate=0x218d5e0) at ../3rdparty/v8/src/builtins.cc:1164
#9  0x00007fa38c64e669 in v8::internal::Builtin_Impl_HandleApiCall (args=..., isolate=0x218d5e0) at ../3rdparty/v8/src/builtins.cc:1181
#10 0x00007fa38c64e63a in v8::internal::Builtin_HandleApiCall (args=..., isolate=0x218d5e0) at ../3rdparty/v8/src/builtins.cc:1180
#11 0x0000150b9ec0428e in ?? ()
#12 0x0000150b9ec04201 in ?? ()
#13 0x00007fff0f4cbdc0 in ?? ()
#14 0x00007fff0f4cbe38 in ?? ()
#15 0x0000150b9ec28650 in ?? ()
#16 0x00002300f3d97531 in ?? ()
#17 0x00000dc171e2a259 in ?? ()
#18 0x0000000000000000 in ?? ()
Comment 1 Andras Becsi 2012-02-23 05:17:06 PST
The infinite loop happens due to MiniBrowser's touch mocking code also sending TouchBegin/TouchUpdate events for MouseButtonDblClick events, which do not get handled and end up as mouse events again in notify where the loop starts again.

I'm currently working on changing the MiniBrowser touch mocking code that it only delivers touch events to the canvas, this way we do not end up in a mess of having both touch and mouse events when using the flickable web view.
Comment 2 Andras Becsi 2012-02-28 07:25:39 PST
Created attachment 129248 [details]
proposed patch
Comment 3 WebKit Review Bot 2012-02-28 07:28:48 PST
Attachment 129248 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Andras Becsi 2012-02-28 07:34:56 PST
Created attachment 129251 [details]
proposed patch with corrected style
Comment 5 Kenneth Rohde Christiansen 2012-02-28 07:37:36 PST
Comment on attachment 129248 [details]
proposed patch

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

> Source/WebKit2/ChangeLog:9
> +        Because the WebView item accepts all touch events it receives and sends
> +        them to the web process before propagating them to the gesture recognizers

You don't say whether this is the right thing to do. Is it?

> Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:124
> +    // Do the touch to mouse event conversion for the Flickable here
> +    // to prevent infinite loops between the WebView and this function.

Why not explain how that could happen instead?

> Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:151
> +    // to the Flickable. This makes sure that the Flickable grabbs

grabbs??? grabs? :-)

> Tools/MiniBrowser/qt/BrowserWindow.cpp:78
> -    return rootObject()->property("webview").value<QQuickWebView*>();
> +    return m_webView.read(rootObject()).value<QQuickWebView*>();

Could you explain the difference?

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:129
> +        // If the event is not meant for the WebView send it directly to the target.

, after WebView?

the target -> its target?

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:134
> +        if (QQuickWebView* webView = browserWindow->webView()) {
> +            QRectF viewportRect = webView->mapRectToScene(webView->boundingRect());
> +            if (viewportRect.isEmpty() || !viewportRect.contains(mouseEvent->localPos()))
> +                return QGuiApplication::notify(target, event);
> +        }

could it be covered by something else? why would this work in that case?
Comment 6 Andras Becsi 2012-02-28 07:51:53 PST
(In reply to comment #5)
> (From update of attachment 129248 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129248&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        Because the WebView item accepts all touch events it receives and sends
> > +        them to the web process before propagating them to the gesture recognizers
> 
> You don't say whether this is the right thing to do. Is it?
Yes, this is the right thing to do.

> 
> > Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:124
> > +    // Do the touch to mouse event conversion for the Flickable here
> > +    // to prevent infinite loops between the WebView and this function.
> 
> Why not explain how that could happen instead?
Will do.

> 
> > Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:151
> > +    // to the Flickable. This makes sure that the Flickable grabbs
> 
> grabbs??? grabs? :-)
d'oh, thanks.

> 
> > Tools/MiniBrowser/qt/BrowserWindow.cpp:78
> > -    return rootObject()->property("webview").value<QQuickWebView*>();
> > +    return m_webView.read(rootObject()).value<QQuickWebView*>();
> 
> Could you explain the difference?
Will add a comment about avoiding string look-ups

> 
> > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:134
> > +        if (QQuickWebView* webView = browserWindow->webView()) {
> > +            QRectF viewportRect = webView->mapRectToScene(webView->boundingRect());
> > +            if (viewportRect.isEmpty() || !viewportRect.contains(mouseEvent->localPos()))
> > +                return QGuiApplication::notify(target, event);
> > +        }
> 
> could it be covered by something else? why would this work in that case?
This is a corner case of the desktop use-case an is needed because the WebView is the only item which suppors touch events natively.
Previously we always sent both, touch and mouse, which sometimes ended up in the described infinite loop.
I also tried doing touch mocking for every mouse event and let Qt convert the touch back to mouse if noone accepted it, but QGuiApplication cashes the global position and button state of the real (initial) mouse event and does not send it again when it was converted back from a touch event.
So this is the only solution I see for making the MiniBrowser multi-touch mocking work.
Comment 7 Andras Becsi 2012-02-28 08:43:54 PST
Created attachment 129264 [details]
proposed patch
Comment 8 Andras Becsi 2012-02-28 09:29:42 PST
(In reply to comment #6)
> (In reply to comment #5)
> > could it be covered by something else? why would this work in that case?
Sorry, I misunderstood the point of the question.

We need a mechanism to prevent touch events being accepted by the WebView in the case of dialogs for example, which depend on mouse events, because they would never be converted to mouse events on the device (and neither on the desktop).

As far as I know this issue was also present in grob and was solved with a boolean flag in the WebView, but I think this case would need a better solution in Qt which is out of scope for this change.
There are already ongoing discussions about the issues arising from QQuickItems not supporting touch events.
Comment 9 Andras Becsi 2012-02-29 09:32:52 PST
Comment on attachment 129264 [details]
proposed patch

Remove from review queue until further discussions have a conclusion on whether or not to support multi-touch mocking in MiniBrowser.
Comment 10 Andras Becsi 2012-02-29 12:39:37 PST
Comment on attachment 129264 [details]
proposed patch

If we create the touch events in MiniBrowserApplication and do not use QWindowSystemInterface to create them we can send the original mouse event in case the touch event was not accepted. I think this would be a better solution than the hit-testing.
Comment 11 Simon Hausmann 2012-03-01 00:08:09 PST
(In reply to comment #10)
> (From update of attachment 129264 [details])
> If we create the touch events in MiniBrowserApplication and do not use QWindowSystemInterface to create them we can send the original mouse event in case the touch event was not accepted. I think this would be a better solution than the hit-testing.

I agree.

We do need a workaround for this for the time being (for us to be able to continue development), until QQ accepts touch and mouse events equally without intermediate synthetization.
Comment 12 Andras Becsi 2012-03-01 08:45:27 PST
Created attachment 129712 [details]
proposed patch
Comment 13 Andras Becsi 2012-03-02 04:27:59 PST
Created attachment 129877 [details]
updated patch
Comment 14 Simon Hausmann 2012-03-02 05:53:35 PST
Comment on attachment 129877 [details]
updated patch

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

r=me with a few quirks :)

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:41
> +    static QRectF touchRect(0, 0, 40, 40);

I think the static keyboard should be removed. I doubt that there's a performance impact here, and IMO it makes the code harder to read ("What does he mean with static here?")

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:138
> +        static QPointF lastPos;

Why not make this a member variable? IMHO that's cleaner.

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:147
> +        static QPointF lastScreenPos;

Same here.

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:168
> +                event->accept();

Perhaps a comment here (as well as below) would be good here, to explain why we _swallow_ the event instead of delivering it. (flickable mess...)
Comment 15 Andras Becsi 2012-03-02 07:29:12 PST
Committed r109575: <http://trac.webkit.org/changeset/109575>
Comment 16 Andras Becsi 2012-03-02 07:31:52 PST
Comment on attachment 129877 [details]
updated patch

Clearing flags from attachment.