Fix invalidity in HitTestCache with LayoutParts.

Fix invalidity in HitTestCache with LayoutParts.
    
The UMA metrics indicate that we don't have a matching HitTestRequest
after a hit test. The only path that adjusts this is the LayoutPart.
Attempt to fix this by recursively clearing the hit test cache
when a child frame clears its own.
    
BUG=398920
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200439

[dtapuska, 2015-07-17 16:30:58]

dtapuska@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-07-17 17:32:26]

We really need tests, you're landing all these fixes without them.

I don't understand the patch description. What do you mean a widget is added to
the part?

What's this case you're talking about with the child frame?

[dtapuska, 2015-07-17 18:14:57]

On 2015/07/17 17:32:26, esprehn wrote:
> We really need tests, you're landing all these fixes without them.
> 
> I don't understand the patch description. What do you mean a widget is added
to
> the part?
> 
> What's this case you're talking about with the child frame?

So taking a step back; I'll tell you how I arrived at this...

So the UMA metric indicates a bit set value of 6 (0b00000110); with 8.02% rate.
This means the hit cache result and the true result differed by that it was over
the widget; the hit test request, the inner node, inner point and inner pseudo
mode.

The only path that would imply that it was over a widget (set in only one place
in all the code L154; LayoutPart.cpp). To get into this case; we must have
cached a result from calling nodeAtPointOverWidget.

Entering this code occurs in two spots and the conditions are:
1)  if (!widget() || !widget()->isFrameView() ||
!result.hitTestRequest().allowsChildFrameContent())
2)  if (!visibleToHitTestRequest(result.hitTestRequest()) && childRoot) ||
notInsideChildFrame

AllowsChildFrameContent can't be the conditional that causes this situation as
that changes the hit test request; and that is one of the conditions of using
the cache in the first place.

So when an parent document changes an iframe src/srcdoc; I believe (could be
wrong) it rebuilds the widget. That is why I wanted to clear the cache on the
updateOnWidgetChange.

Secondly the other case where it could cache if it falls through the second set
of conditions; and I wanted to avoid that by just not caching the result in that
case.

I have examined this code and thought about this problem in the back of my mind
for a while. I have tried to write LayoutTests to demonstrated this issue and
haven't to any avail; and I wonder if it is a timing issue between when the
widget gets loaded into the iframe vs if it just got a hit test just before it.

The problem with my testing approach by throwing a lot of hit tests at the
iframe; and then adjust the src of the iframe causes the dom version to change
so it can't really be that; but I think it is a race between when the src is set
and when the widget is actually built for the layout part it gets a hit test in
before the widget is set.

Alternatively I've wondered if it is some timing related to plugins for the
iframes.

I'm happy not to land this as I can't really reproduce it.

As for the mac scrollbar bug; that was certainly an issue I reproduced (and
found via the analysis of the uma bitset validity metric) and I will try writing
a layout test for mac for that.

[esprehn, 2015-07-17 18:31:29]

I don't think we can land this without a test. 

If you change the src of the frame that would change the dom tree version.
Perhaps what you're seeing is the frame itself navigate? That will swap the
widget but won't dirty the layout/style/dom of the parent. I don't really know
what you mean by "rebuild", I assume you mean swap.

ex.
1) <iframe src="navigate.html">
2) Hit test so it hits the iframe
3) postMessage the iframe so it does window.location.href =
"navigate-other.html"
4) Hit test again so it would hit the frame.

I think that might be the case you're missing? 

Also how do you handle clearing the cache across frame boundaries in the general
case? If you pass AllowsChildFrameContent and hit test down through many levels
of descendant frames, what clears the cache way up at the top?

[dtapuska, 2015-07-17 18:40:33]

On 2015/07/17 18:31:29, esprehn wrote:
> I don't think we can land this without a test. 
> 
> If you change the src of the frame that would change the dom tree version.
> Perhaps what you're seeing is the frame itself navigate? That will swap the
> widget but won't dirty the layout/style/dom of the parent. I don't really know
> what you mean by "rebuild", I assume you mean swap.
> 
> ex.
> 1) <iframe src="navigate.html">
> 2) Hit test so it hits the iframe
> 3) postMessage the iframe so it does window.location.href =
> "navigate-other.html"
> 4) Hit test again so it would hit the frame.
> 
> I think that might be the case you're missing? 
> 
> Also how do you handle clearing the cache across frame boundaries in the
general
> case? If you pass AllowsChildFrameContent and hit test down through many
levels
> of descendant frames, what clears the cache way up at the top?

The cache is only valid for the containing document. So if you hit something in
an iframe; it will only be cached in that
layout view and not in the parent's cache. Line 182 of LayoutPart prevents this
by setting the setCacheable(false) if it got
a hit inside a LayoutPart.

So caches across documents aren't necessary. The alternate is to implement
clearCache so it chains across documents;
but I wanted clearCache to be cheap as it gets called in scroll.

I'm wondering if it is in fact related to plugins. I see that plugins are loaded
on a timer from FrameView; and that calls
setWidget (at least for HTMLAppletElement).

I'll try your example; thanks for the ideas.

[esprehn, 2015-07-24 08:26:02]

esprehn@chromium.org changed reviewers:
+ leviw@chromium.org, ojan@chromium.org

[esprehn, 2015-07-24 08:26:23]

I'm out for the next 2 weeks, perhaps ojan@ or leviw@ can help you during that
time. I'm happy to help when I get back too.

[dtapuska, 2015-08-06 19:42:17]

On 2015/07/24 08:26:23, esprehn (ooo-until-aug-10) wrote:
> I'm out for the next 2 weeks, perhaps ojan@ or leviw@ can help you during that
> time. I'm happy to help when I get back too.

Can someone take a look at this revised patch with the layout test since
esprehn@ is out?

[ojan, 2015-08-06 20:42:27]

ojan@chromium.org changed reviewers:
+ pdr@chromium.org

[ojan, 2015-08-06 20:42:27]

pdr, this looks like it's in your wheelhouse. Mind reviewing? I won't be able to
review this anytime soon.

[pdr., 2015-08-07 05:33:53]

On 2015/08/06 at 20:42:27, ojan wrote:
> pdr, this looks like it's in your wheelhouse. Mind reviewing? I won't be able
to review this anytime soon.

Sorry, I didn't get to this today. I'll take a look tomorrow.

[pdr., 2015-08-08 05:44:14]

https://codereview.chromium.org/1242593004/diff/20001/LayoutTests/fast/events...
File LayoutTests/fast/events/hit-test-cache-iframes.html (right):

https://codereview.chromium.org/1242593004/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache-iframes.html:1: <meta name="viewport"
content="width=device-width, initial-scale=1">
Lets remove the meta viewport and instead put <!DOCTYPE html>

https://codereview.chromium.org/1242593004/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache-iframes.html:15: description("Ensure hit
test cache works in when an iframe is in between loading and loaded.");
Maybe rephrase this as "Ensure the hit test cache is disabled for loading
iframes."?

https://codereview.chromium.org/1242593004/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache-iframes.html:18:
document.body.insertAdjacentHTML('beforeend', '<iframe id="myframe"
srcdoc="<p>Hi</p>" onload="iFrameLoaded()"></iframe>');
Lets remove <p>Hi</p> so it does't affect hit test results by accident.

https://codereview.chromium.org/1242593004/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1242593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.cpp:152: // Check to see if we are really over the
widget itself (and not just in the border/padding area).
It looks like there are three modes here:
1) hit test on border/padding
2) hit test on loading content
3) hit test on loaded content

The patch description sounds like we don't want to cache any of these. I see why
grouping 2 and 3 makes sense for simplicity but why can't we cache hits on the
border or padding?

https://codereview.chromium.org/1242593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.cpp:157: // likely isn't loaded yet; so don't
cache this result.
Can we replace "...the child FrameView likely isn't loaded yet" with an assert
that it hasn't loaded yet?

Can you help me understand why loading matters here? It looks like the codepath
here depends on more than loading. I wonder if we really just want prevent
content hit tests entirely, regardless of loading.

[dtapuska, 2015-08-10 19:35:42]

https://codereview.chromium.org/1242593004/diff/20001/LayoutTests/fast/events...
File LayoutTests/fast/events/hit-test-cache-iframes.html (right):

https://codereview.chromium.org/1242593004/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache-iframes.html:1: <meta name="viewport"
content="width=device-width, initial-scale=1">
On 2015/08/08 05:44:14, pdr wrote:
> Lets remove the meta viewport and instead put <!DOCTYPE html>

Done.

https://codereview.chromium.org/1242593004/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache-iframes.html:15: description("Ensure hit
test cache works in when an iframe is in between loading and loaded.");
On 2015/08/08 05:44:14, pdr wrote:
> Maybe rephrase this as "Ensure the hit test cache is disabled for loading
> iframes."?

Done.

https://codereview.chromium.org/1242593004/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache-iframes.html:18:
document.body.insertAdjacentHTML('beforeend', '<iframe id="myframe"
srcdoc="<p>Hi</p>" onload="iFrameLoaded()"></iframe>');
On 2015/08/08 05:44:13, pdr wrote:
> Lets remove <p>Hi</p> so it does't affect hit test results by accident.

Done.

https://codereview.chromium.org/1242593004/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1242593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.cpp:152: // Check to see if we are really over the
widget itself (and not just in the border/padding area).
On 2015/08/08 05:44:14, pdr wrote:
> It looks like there are three modes here:
> 1) hit test on border/padding
> 2) hit test on loading content
> 3) hit test on loaded content
> 
> The patch description sounds like we don't want to cache any of these. I see
why
> grouping 2 and 3 makes sense for simplicity but why can't we cache hits on the
> border or padding?

Yes ideally we could cache the border or padding areas; but that seemed like
more complex code to add here for minimal gain.

https://codereview.chromium.org/1242593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.cpp:157: // likely isn't loaded yet; so don't
cache this result.
On 2015/08/08 05:44:14, pdr wrote:
> Can we replace "...the child FrameView likely isn't loaded yet" with an assert
> that it hasn't loaded yet?
> 
> Can you help me understand why loading matters here? It looks like the
codepath
> here depends on more than loading. I wonder if we really just want prevent
> content hit tests entirely, regardless of loading.

Effectively yes this patch prevents caching hit tests entirely if they are
inside a LayoutPart. The LayoutPart isn't told when the child widget is really
ready to do hit tests and we may cache an item incorrectly if it is anywhere
inside the LayoutPart but then the child then loads, creates its layers and then
can accept hit tests.

The other conditions of whether you allow child frame content and whether it is
visible to the hit test should be covered when checking that the hit test
request is actually the same type that is stored in the cache.

I presume I could add an assert for loading but it would effectively be all the
conditions that lead to the execution of the nodeAtPointOverWidget method; and
that doesn't seem to meaningful.

[pdr., 2015-08-10 20:01:27]

>
https://codereview.chromium.org/1242593004/diff/20001/Source/core/layout/Layo...
> Source/core/layout/LayoutPart.cpp:157: // likely isn't loaded yet; so don't
cache this result.
> On 2015/08/08 05:44:14, pdr wrote:
> > Can we replace "...the child FrameView likely isn't loaded yet" with an
assert
> > that it hasn't loaded yet?
> > 
> > Can you help me understand why loading matters here? It looks like the
codepath
> > here depends on more than loading. I wonder if we really just want prevent
> > content hit tests entirely, regardless of loading.
> 
> Effectively yes this patch prevents caching hit tests entirely if they are
inside a LayoutPart. The LayoutPart isn't told when the child widget is really
ready to do hit tests and we may cache an item incorrectly if it is anywhere
inside the LayoutPart but then the child then loads, creates its layers and then
can accept hit tests.
> 
> The other conditions of whether you allow child frame content and whether it
is visible to the hit test should be covered when checking that the hit test
request is actually the same type that is stored in the cache.
> 
> I presume I could add an assert for loading but it would effectively be all
the conditions that lead to the execution of the nodeAtPointOverWidget method;
and that doesn't seem to meaningful.

Ok, my only remaining issue is the comment then. This code can be hit in many
cases, not just loading. Lets update the comment to say that we can't cache hit
tests on frame content because we won't get callbacks to invalidate the cache,
and we're including borders and padding for simplicity.

[dtapuska, 2015-08-10 20:11:10]

On 2015/08/10 20:01:27, pdr wrote:
> >
>
https://codereview.chromium.org/1242593004/diff/20001/Source/core/layout/Layo...
> > Source/core/layout/LayoutPart.cpp:157: // likely isn't loaded yet; so don't
> cache this result.
> > On 2015/08/08 05:44:14, pdr wrote:
> > > Can we replace "...the child FrameView likely isn't loaded yet" with an
> assert
> > > that it hasn't loaded yet?
> > > 
> > > Can you help me understand why loading matters here? It looks like the
> codepath
> > > here depends on more than loading. I wonder if we really just want prevent
> > > content hit tests entirely, regardless of loading.
> > 
> > Effectively yes this patch prevents caching hit tests entirely if they are
> inside a LayoutPart. The LayoutPart isn't told when the child widget is really
> ready to do hit tests and we may cache an item incorrectly if it is anywhere
> inside the LayoutPart but then the child then loads, creates its layers and
then
> can accept hit tests.
> > 
> > The other conditions of whether you allow child frame content and whether it
> is visible to the hit test should be covered when checking that the hit test
> request is actually the same type that is stored in the cache.
> > 
> > I presume I could add an assert for loading but it would effectively be all
> the conditions that lead to the execution of the nodeAtPointOverWidget method;
> and that doesn't seem to meaningful.
> 
> Ok, my only remaining issue is the comment then. This code can be hit in many
> cases, not just loading. Lets update the comment to say that we can't cache
hit
> tests on frame content because we won't get callbacks to invalidate the cache,
> and we're including borders and padding for simplicity.

Done

[esprehn, 2015-08-10 20:11:54]

Why don't we get invalidation callbacks for frames?

[dtapuska, 2015-08-10 20:16:22]

On 2015/08/10 20:11:54, esprehn (ooo-until-aug-10) wrote:
> Why don't we get invalidation callbacks for frames?

There are a few cases I know of on the top of my head:
1) We don't propagate the clearCacheEvent across frames. Since it was frequently
called; I didn't want to propagate it since it is a walk.
2) We don't get told when the widget changes or when the LayoutView on the
FrameView changes in the LayoutPart object.

[esprehn, 2015-08-10 21:22:02]

On 2015/08/10 at 20:16:22, dtapuska wrote:
> On 2015/08/10 20:11:54, esprehn (ooo-until-aug-10) wrote:
> > Why don't we get invalidation callbacks for frames?
> 
> There are a few cases I know of on the top of my head:
> 1) We don't propagate the clearCacheEvent across frames. Since it was
frequently called; I didn't want to propagate it since it is a walk.

What does frequently mean? Depth of the frame tree is not usually too bad, once
per scroll that should be fine.

> 2) We don't get told when the widget changes or when the LayoutView on the
FrameView changes in the LayoutPart object.

Is that true? I'm pretty sure any of those things changing cause a recomposite
so someone got told.

https://codereview.chromium.org/1242593004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1242593004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.cpp:158: if (inside)
Why not just use the isOverWidget check we did above? That avoids the
borders/padding and we're already doing it anyway.

[dtapuska, 2015-08-11 18:25:15]

On 2015/08/10 21:22:02, esprehn (ooo-until-aug-10) wrote:
> On 2015/08/10 at 20:16:22, dtapuska wrote:
> > On 2015/08/10 20:11:54, esprehn (ooo-until-aug-10) wrote:
> > > Why don't we get invalidation callbacks for frames?
> > 
> > There are a few cases I know of on the top of my head:
> > 1) We don't propagate the clearCacheEvent across frames. Since it was
> frequently called; I didn't want to propagate it since it is a walk.
> 
> What does frequently mean? Depth of the frame tree is not usually too bad,
once
> per scroll that should be fine.
> 
> > 2) We don't get told when the widget changes or when the LayoutView on the
> FrameView changes in the LayoutPart object.
> 
> Is that true? I'm pretty sure any of those things changing cause a recomposite
> so someone got told.
> 
>
https://codereview.chromium.org/1242593004/diff/60001/Source/core/layout/Layo...
> File Source/core/layout/LayoutPart.cpp (right):
> 
>
https://codereview.chromium.org/1242593004/diff/60001/Source/core/layout/Layo...
> Source/core/layout/LayoutPart.cpp:158: if (inside)
> Why not just use the isOverWidget check we did above? That avoids the
> borders/padding and we're already doing it anyway.

What about this change; it makes clearHitTestCache call the parent whenever it
changes inside an iframe.

[eae, 2015-08-12 20:25:56]

eae@chromium.org changed reviewers:
- leviw@chromium.org, ojan@chromium.org

[esprehn, 2015-08-12 21:33:50]

lgtm

[dtapuska, 2015-08-12 21:58:52]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2015-08-12 21:59:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1242593004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1242593004/80001

[commit-bot: I haz the power, 2015-08-13 00:00:07]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200439