Bug 94345

Summary: [Qt][WK2] Accelerated compositing example with links being animated crashes tap highlight
Product: WebKit Reporter: Jesus Sanchez-Palencia <jesus>
Component: WebKit QtAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, allan.jensen, cmarcelo, jturcotte, luciano.wolf, marcelo.lira, ossy
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 79668, 110211    
Attachments:
Description Flags
Falling leaves example modified to one link per leaf
none
Patch
none
Patch jturcotte: review+

Description Jesus Sanchez-Palencia 2012-08-17 06:57:03 PDT
Created attachment 159115 [details]
Falling leaves example modified to one link per leaf

The attached modified version of the classical falling leaves (http://www.webkit.org/blog-files/leaves/) makes each leaf become a link to another web page.

It crashes MiniBrowser and snowshoe on tap highlight code path. I believe the UIProcess is sending the position of where the highlight should be, but since the link element is being animated the position has changed when this information reaches the other process.

#0  0x00007f1a72d8ecad in WebCore::RenderObject::offsetFromAncestorContainer (this=0x1c4f008, container=0x1a3b4b8) at /home/jeez/code/webkit/Source/WebCore/rendering/RenderObject.cpp:2154
#1  0x00007f1a72ad3b23 in WebCore::(anonymous namespace)::absolutePathForRenderer (o=0x1c4f008) at /home/jeez/code/webkit/Source/WebCore/page/GestureTapHighlighter.cpp:205
#2  0x00007f1a72ad3ef1 in WebCore::GestureTapHighlighter::pathForNodeHighlight (node=0x1c856b0) at /home/jeez/code/webkit/Source/WebCore/page/GestureTapHighlighter.cpp:247
#3  0x00007f1a7217f22c in WebKit::TapHighlightController::highlight (this=0x18ff858, node=0x1c856b0)
at /home/jeez/code/webkit/Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:60
#4  0x00007f1a72195369 in WebKit::WebPage::highlightPotentialActivation (this=0x18ff420, point=..., area=...) at /home/jeez/code/webkit/Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1565
Comment 1 Allan Sandfeld Jensen 2012-08-17 07:27:48 PDT
This looks very much like what I fixed in bug #93845, do you have a recent checkout?
Comment 2 Jesus Sanchez-Palencia 2012-08-20 08:23:17 PDT
(In reply to comment #1)
> This looks very much like what I fixed in bug #93845, do you have a recent checkout?

I've just tested it with today's trunk (3e50844462d4997e76300743ac5109868e233e5e) and the ASSERT is still reached:

ASSERTION FAILED: !currContainer->hasTransform()
/home/jeez/code/webkit/Source/WebCore/rendering/RenderObject.cpp(2167) : WebCore::LayoutSize WebCore::RenderObject::offsetFromAncestorContainer(WebCore::RenderObject*) const



Also, if you try it in release mode to avoid the assert, you can see Highlight being drawn as a rectangle. I know this is a different bug, but it's a bug. :P
Comment 3 Allan Sandfeld Jensen 2012-08-20 08:35:33 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > This looks very much like what I fixed in bug #93845, do you have a recent checkout?
> 
> I've just tested it with today's trunk (3e50844462d4997e76300743ac5109868e233e5e) and the ASSERT is still reached:
> 
> ASSERTION FAILED: !currContainer->hasTransform()
> /home/jeez/code/webkit/Source/WebCore/rendering/RenderObject.cpp(2167) : WebCore::LayoutSize WebCore::RenderObject::offsetFromAncestorContainer(WebCore::RenderObject*) const
> 
Ahh, that makes sense, so the problem is offsetFromAncestorContainer can not handle transformation.
 
> 
> Also, if you try it in release mode to avoid the assert, you can see Highlight being drawn as a rectangle. I know this is a different bug, but it's a bug. :P

Don't be too sure it is a different bug, it could very well be a consequence of this one.
Comment 4 Jesus Sanchez-Palencia 2012-08-20 08:39:22 PDT
(In reply to comment #3)
> Don't be too sure it is a different bug, it could very well be a consequence of this one.

Can you reproduce the reported bugs, Allan?
Comment 5 Allan Sandfeld Jensen 2012-08-20 08:51:21 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Don't be too sure it is a different bug, it could very well be a consequence of this one.
> 
> Can you reproduce the reported bugs, Allan?

Yes, though I don't get any highlight when using the release version.

I also know what the problem is better now, and there is no easy way to fix it. This is a design flaw in how we draw the tap-highlight. At best we can just ignore clipping layers when transforms are involved.
Comment 6 Allan Sandfeld Jensen 2012-08-20 08:54:22 PDT
See bug #84936 for more details on how this could be solved with a different design for calculating tap highlights.
Comment 7 Allan Sandfeld Jensen 2013-08-08 06:42:49 PDT
Reported as QTBUG-32454 now. https://bugreports.qt-project.org/browse/QTBUG-32454
Comment 8 Allan Sandfeld Jensen 2013-08-08 08:03:16 PDT
Created attachment 208344 [details]
Patch
Comment 9 Jocelyn Turcotte 2013-08-09 03:35:43 PDT
Comment on attachment 208344 [details]
Patch

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

> Source/WebCore/page/GestureTapHighlighter.cpp:224
> +                // Use bounding box because we can only highlight rects currently.
> +                ringRect = ringQuad.enclosingBoundingBox();

This will look bogus with rotation, perspective, etc.
If you could check ringQuad.isRectilinear() first that would make this patch an improvement in every case I can think of.
Comment 10 Allan Sandfeld Jensen 2013-08-09 05:28:06 PDT
Created attachment 208418 [details]
Patch
Comment 11 Jocelyn Turcotte 2013-08-09 07:28:21 PDT
Comment on attachment 208418 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [Qt][WK2] Accelerated compositing example with links being animated crashes tap highlight

The Qt style bot will complain downstream, "Accelerated compositing" -> "AC" would help :P

> Source/WebCore/page/GestureTapHighlighter.cpp:227
> +                if (ringQuad.isRectilinear())
> +                    ringRect = ringQuad.enclosingBoundingBox();
> +                else
> +                    ringRect = LayoutRect();

Nit: Could you just break directly here?
if (!isRectilinear())
    break;
Comment 12 Allan Sandfeld Jensen 2013-08-09 07:54:09 PDT
(In reply to comment #11)
> (From update of attachment 208418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208418&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [Qt][WK2] Accelerated compositing example with links being animated crashes tap highlight
> 
> The Qt style bot will complain downstream, "Accelerated compositing" -> "AC" would help :P
> 
Good idea.

> > Source/WebCore/page/GestureTapHighlighter.cpp:227
> > +                if (ringQuad.isRectilinear())
> > +                    ringRect = ringQuad.enclosingBoundingBox();
> > +                else
> > +                    ringRect = LayoutRect();
> 
> Nit: Could you just break directly here?
> if (!isRectilinear())
>     break;

No, then ringRect would have its untransformed value. The rects are painted in a later iteration.

Thanks for the review :)
Comment 13 Allan Sandfeld Jensen 2013-08-09 07:58:55 PDT
Committed r153893: <http://trac.webkit.org/changeset/153893>