Bug 83428

Summary: [Qt][WK2] Zoom gesture with double tap crashes on iframe when main frame has scroll offset.
Product: WebKit Reporter: zalan <zalan>
Component: WebCore Misc.Assignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, allan.jensen, gustavo, kenneth, ossy, pnormand, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 79668    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
kenneth: review+
Patch none

Description zalan 2012-04-07 12:33:13 PDT
tsia
Comment 1 zalan 2012-04-07 12:54:42 PDT
Created attachment 136138 [details]
Patch
Comment 2 zalan 2012-04-07 12:56:39 PDT
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);
Comment 3 zalan 2012-04-07 12:57:43 PDT
(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.
Comment 4 Allan Sandfeld Jensen 2012-04-09 09:13:48 PDT
(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.
Comment 5 zalan 2012-04-09 10:39:23 PDT
(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.
Comment 6 Allan Sandfeld Jensen 2012-04-09 11:58:10 PDT
(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.
Comment 7 zalan 2012-04-10 12:25:17 PDT
Created attachment 136507 [details]
Patch
Comment 8 zalan 2012-04-10 12:27:26 PDT
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.
Comment 9 Philippe Normand 2012-04-10 12:33:22 PDT
Comment on attachment 136507 [details]
Patch

Attachment 136507 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12380445
Comment 10 Build Bot 2012-04-10 12:57:23 PDT
Comment on attachment 136507 [details]
Patch

Attachment 136507 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12381436
Comment 11 Build Bot 2012-04-10 13:17:18 PDT
Comment on attachment 136507 [details]
Patch

Attachment 136507 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12385246
Comment 12 zalan 2012-04-11 11:50:40 PDT
Created attachment 136716 [details]
Patch
Comment 13 zalan 2012-04-11 11:51:52 PDT
ixed exporting. (hopefully for gtk too)
Comment 14 zalan 2012-04-11 11:58:32 PDT
(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.
Comment 15 Allan Sandfeld Jensen 2012-04-12 01:25:03 PDT
(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.
Comment 16 Kenneth Rohde Christiansen 2012-04-12 01:32:43 PDT
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?
Comment 17 zalan 2012-04-12 01:45:45 PDT
(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.
Comment 18 zalan 2012-04-12 01:47:00 PDT
(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.
Comment 19 zalan 2012-04-12 02:35:45 PDT
Created attachment 136859 [details]
Patch
Comment 20 WebKit Review Bot 2012-04-12 04:12:29 PDT
Comment on attachment 136859 [details]
Patch

Clearing flags on attachment: 136859

Committed r113962: <http://trac.webkit.org/changeset/113962>
Comment 21 WebKit Review Bot 2012-04-12 04:12:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Csaba Osztrogonác 2012-04-12 07:30:25 PDT
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?
Comment 23 zalan 2012-04-12 22:45:46 PDT
created bug#83860 for this problem.
Comment 24 zalan 2012-04-13 04:13:37 PDT
fixed in http://trac.webkit.org/changeset/114109