Bug 79119 – [Qt][WK2] Infinite loop on history navigation, when panning.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[alan]

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

[Andras Becsi]

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.

[Andras Becsi]

Created attachment 129248 [details]
proposed patch

[WebKit Review Bot]

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.

[Andras Becsi]

Created attachment 129251 [details]
proposed patch with corrected style

[Kenneth Rohde Christiansen]

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?

[Andras Becsi]

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

[Andras Becsi]

Created attachment 129264 [details]
proposed patch

[Andras Becsi]

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

[Andras Becsi]

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.

[Andras Becsi]

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.

[Simon Hausmann]

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

[Andras Becsi]

Created attachment 129712 [details]
proposed patch

[Andras Becsi]

Created attachment 129877 [details]
updated patch

[Simon Hausmann]

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

[Andras Becsi]

Committed r109575: <http://trac.webkit.org/changeset/109575>

[Andras Becsi]

Comment on attachment 129877 [details]
updated patch

Clearing flags from attachment.