Bug 83428 – [Qt][WK2] Zoom gesture with double tap crashes on iframe when main frame has scroll offset.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[zalan]

tsia

[zalan]

Created attachment 136138 [details]
Patch

[zalan]

also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this

if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node))
    return;

ASSERT(node);

[zalan]

(In reply to comment #2)
> also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this
> 
> if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node))
>     return;
> 
> ASSERT(node);
'if (!mainframe' obviously.

[Allan Sandfeld Jensen]

(In reply to comment #2)
> also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this
> 
> if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node))
>     return;
> 
> ASSERT(node);

I am not sure if the return makes sense. The ASSERT is better. If nothing else is found the root element should be returned.

[zalan]

(In reply to comment #4)
> (In reply to comment #2)
> > also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this
> > 
> > if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node))
> >     return;
> > 
> > ASSERT(node);
> 
> I am not sure if the return makes sense. The ASSERT is better. If nothing else is found the root element should be returned.

ok, sounds good. We lose(ignore) a bool return value on this callstack (findZoomableAreaForPoint()->bestZoomableAreaForTouchPoint()->findBestZoomableArea()->findNodeWithLowestDistanceMetric()) and that is either defective or it suggests that it was originally designed with a different mindset and needs to be changed.

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this
> > > 
> > > if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node))
> > >     return;
> > > 
> > > ASSERT(node);
> > 
> > I am not sure if the return makes sense. The ASSERT is better. If nothing else is found the root element should be returned.
> 
> ok, sounds good. We lose(ignore) a bool return value on this callstack (findZoomableAreaForPoint()->bestZoomableAreaForTouchPoint()->findBestZoomableArea()->findNodeWithLowestDistanceMetric()) and that is either defective or it suggests that it was originally designed with a different mindset and needs to be changed.

True. More accurately I have no problem with the return if it happens after an ASSERT. It can theoretically happen, but would mean the touch-point given is outside the frame, which is not really wrong, but indicates something has transformed the pointer wrong, or has send the event to the wrong application.

[zalan]

Created attachment 136507 [details]
Patch

[zalan]

Comment on attachment 136507 [details]
Patch

Now looking at the patch, I might need to split it up. I'll do after the review if the direction is okay.

[Philippe Normand]

Comment on attachment 136507 [details]
Patch

Attachment 136507 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12380445

[Build Bot]

Comment on attachment 136507 [details]
Patch

Attachment 136507 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12381436

[Build Bot]

Comment on attachment 136507 [details]
Patch

Attachment 136507 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12385246

[zalan]

Created attachment 136716 [details]
Patch

[zalan]

ixed exporting. (hopefully for gtk too)

[zalan]

(In reply to comment #8)
> (From update of attachment 136507 [details])
> Now looking at the patch, I might need to split it up. I'll do after the review if the direction is okay.

This patch also modifies the behavior of highlighting a bit. Previously when hittest returned no valid node, the ongoing highlighting was cancelled, even though the UI process sends (0,0) to cancel it. With this patch, only explicit cancelling terminates highlighting.

[Allan Sandfeld Jensen]

(In reply to comment #14)
> (In reply to comment #8)
> > (From update of attachment 136507 [details] [details])
> > Now looking at the patch, I might need to split it up. I'll do after the review if the direction is okay.
> 
> This patch also modifies the behavior of highlighting a bit. Previously when hittest returned no valid node, the ongoing highlighting was cancelled, even though the UI process sends (0,0) to cancel it. With this patch, only explicit cancelling terminates highlighting.

That is fine I guess, but it is kinda of theoretical. The highlight is only active for a short time during or shortly after a touch and I guess the gesture recognizer will cancel the old highlight before trying to highlight a new touch.

The patch looks great btw.

[Kenneth Rohde Christiansen]

Comment on attachment 136716 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:650
> +void Internals::setDelegatesScrolling(bool enabled, Document* document, ExceptionCode& ec)

Nice!

> LayoutTests/touchadjustment/iframe-with-mainframe-scroll-offset.html:1
> +<html>

Shouldn\t the name of this file indicate that it is using scroll delegation? or subdir?

[zalan]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #8)
> > > (From update of attachment 136507 [details] [details] [details])
> > > Now looking at the patch, I might need to split it up. I'll do after the review if the direction is okay.
> > 
> > This patch also modifies the behavior of highlighting a bit. Previously when hittest returned no valid node, the ongoing highlighting was cancelled, even though the UI process sends (0,0) to cancel it. With this patch, only explicit cancelling terminates highlighting.
> 
> That is fine I guess, but it is kinda of theoretical. The highlight is only active for a short time during or shortly after a touch and I guess the gesture recognizer will cancel the old highlight before trying to highlight a new touch.
Yes, this is merely making the functionality more explicit, but hardly involves change in real life usecases. the code looked more compact after this change which was the only reason to have it included.
Thanks for the review.

[zalan]

(In reply to comment #16)
> (From update of attachment 136716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136716&action=review
> 
> > Source/WebCore/testing/Internals.cpp:650
> > +void Internals::setDelegatesScrolling(bool enabled, Document* document, ExceptionCode& ec)
> 
> Nice!
> 
> > LayoutTests/touchadjustment/iframe-with-mainframe-scroll-offset.html:1
> > +<html>
> 
> Shouldn\t the name of this file indicate that it is using scroll delegation? or subdir?

Good idea. i'll make a subdir for scroll delegation cases.
Thanks.

[zalan]

Created attachment 136859 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136859 [details]
Patch

Clearing flags on attachment: 136859

Committed r113962: <http://trac.webkit.org/changeset/113962>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because a test started to assert from this change.

crash log for DumpRenderTree (pid 15712):
STDOUT: <empty>
STDERR: ASSERTION FAILED: compositor->inCompositingMode()
STDERR: ../../../../Source/WebCore/page/FrameView.cpp(657) : void WebCore::FrameView::clearBackingStores()
STDERR: 1   0x7f2815382862 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore9FrameView18clearBackingStoresEv+0x6a) [0x7f2815382862]
STDERR: 2   0x7f2815386428 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore9FrameView27delegatesScrollingDidChangeEv+0x18) [0x7f2815386428]
STDERR: 3   0x7f281548c430 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore10ScrollView21setDelegatesScrollingEb+0x48) [0x7f281548c430]
STDERR: 4   0x7f28156ea9e3 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore9Internals21setDelegatesScrollingEbPNS_8DocumentERi+0xa1) [0x7f28156ea9e3]
STDERR: 5   0x7f2815dcf76f /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore49jsInternalsPrototypeFunctionSetDelegatesScrollingEPN3JSC9ExecStateE+0x297) [0x7f2815dcf76f]
STDERR: 6   0x7f27c8ef7258 [0x7f27c8ef7258]

Could you check it, please?

[zalan]

created bug#83860 for this problem.

[zalan]

fixed in http://trac.webkit.org/changeset/114109