Bug 94345 – [Qt][WK2] Accelerated compositing example with links being animated crashes tap highlight

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

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

[Allan Sandfeld Jensen]

This looks very much like what I fixed in bug #93845, do you have a recent checkout?

[Jesus Sanchez-Palencia]

(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

[Allan Sandfeld Jensen]

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

[Jesus Sanchez-Palencia]

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

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

See bug #84936 for more details on how this could be solved with a different design for calculating tap highlights.

[Allan Sandfeld Jensen]

Reported as QTBUG-32454 now. https://bugreports.qt-project.org/browse/QTBUG-32454

[Allan Sandfeld Jensen]

Created attachment 208344 [details]
Patch

[Jocelyn Turcotte]

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.

[Allan Sandfeld Jensen]

Created attachment 208418 [details]
Patch

[Jocelyn Turcotte]

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;

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

Committed r153893: <http://trac.webkit.org/changeset/153893>