ASSERT in RenderLayer::hitTestContents can fire

[Adam Barth]

ASSERT in RenderLayer::hitTestContents can fire

[Adam Barth]

Created attachment 169298 [details]
Patch

[Adam Barth]

This patch comes from 6034d2de096bbdaf1be3474b0141c4d57094a956 in the chromium-android branch (for those with access to that source tree).

[Adam Barth]

    if (!renderer()->hitTest(request, result, hitTestLocation,
                            toLayoutPoint(layerBounds.location() - renderBoxLocation()),
                            hitTestFilter)) {
        // It's wrong to set innerNode, but then claim that you didn't hit anything, unless it is
        // a rect-based test.
        ASSERT(!result.innerNode() || (result.isRectBasedTest() && result.rectBasedTestResult().size()));
        return false;
    }

Apparently that is the ASSERT that fires.

[Adam Barth]

Created attachment 169313 [details]
Some attempts at a test

[Allan Sandfeld Jensen]

Comment on attachment 169298 [details]
Patch

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

The patch makes sense. 

This is is one of those cases that makes me think addNodeToRectBasedResult really should be called from inside updateHitTestResult().

> Source/WebCore/rendering/RenderBlock.h:570
> +    Node* nodeForHitTest();

Could be const.

[Arun Patole]

Created attachment 171151 [details]
test case

I could repro this issue on android JB stock browser and chromium content shell(I had some weeks old content shell code but not the latest one) with attached small test case. You need to touch exactly at the border of image element. Looks to be a problem when hit test point falls in anonymous block.

As Adam rightly pointed out, Assert is because we are setting inner node of HitTestResult and also saying that HitTest doesn't found anything by returning false in HitTest function which is incorrect. This is happening because we are using different nodes in RenderBlock::nodeAtPoint and when setting innerNode in RenderBlock::updateHitTestResult. 

In RenderBlock::updateHitTestResult, in case of anonymous block continuation, we are using continuation()->node() which returns enclosing node i.e. <a> in our case. But in RenderBlock::nodeAtPoint, while calling addNodeToRectBasedTestResult we use "node()" which returns NULL for anonymous blocks. Using same node at both the places fixes the problem. Patch on this bug looks good to me,  Antonio can confirm this as from file revisions, looks like mainly he had implememented rect based hittest.

[Arun Patole]

cc Antonio

[Antonio Gomes]

Comment on attachment 169298 [details]
Patch

could not Internals::nodesFromRect be used to trigger this assert?

[Adam Barth]

Comment on attachment 169298 [details]
Patch

I'm going to try to construct a test based on the comments above.

[Adam Barth]

Created attachment 172085 [details]
LayoutTest that hits the ASSERT

[Adam Barth]

Comment on attachment 169298 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlock.h:570
>> +    Node* nodeForHitTest();
> 
> Could be const.

Done.

[Antonio Gomes]

CC'ing some BlackBerry people, since it gets hitten for them as well.

[Adam Barth]

Created attachment 172087 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 172087 [details]
Patch

Yay!

[WebKit Review Bot]

Comment on attachment 172087 [details]
Patch

Clearing flags on attachment: 172087

Committed r133330: <http://trac.webkit.org/changeset/133330>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

Comment on attachment 172087 [details]
Patch

Nice fix!

[Adam Barth]

Thanks.  :)