Bug 163877 – Prevent hit tests from being performed on an invalid render tree

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brent Fulgham]

Hit testing should be performed with the render tree in a valid state. Some content can cause a hit test operation to trigger a style recalculation. When this happens, we need to make sure the full recalculation is performed before attempting to return hit test results, since style recalculation can cause elements to change their size, position, visibility, etc., causing the hit test to return invalid information.

This patch adds some debug assertions to help catch when this is happening.

[Brent Fulgham]

Created attachment 292555 [details]
WIP Patch

[Brent Fulgham]

Comment on attachment 292555 [details]
WIP Patch

Initial patch to see if this new assertion triggers too frequently.

[zalan]

Comment on attachment 292555 [details]
WIP Patch

The assertions check that we neither _schedule_ a style recalc nor _mark_ the renderview for layout, while the changelog entry says " Assert that we are not hit testing during layout/style recalculation.". I am not sure why we should assert on scheduling/dirtying.

[Simon Fraser (smfr)]

(In reply to comment #3)
> Comment on attachment 292555 [details]
> WIP Patch
> 
> The assertions check that we neither _schedule_ a style recalc nor _mark_
> the renderview for layout, while the changelog entry says " Assert that we
> are not hit testing during layout/style recalculation.". I am not sure why
> we should assert on scheduling/dirtying.

We assert on dirtying because if a subframe dirties its parent frame, the parent frame gets laid out as soon as hit testing traverses to the next frame (updateLayout updates all ancestors), and so we mutate the layer tree that we're traversing in the parent frame.

[Build Bot]

Comment on attachment 292555 [details]
WIP Patch

Attachment 292555 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2355589

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 292581 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Brent Fulgham]

So it seems like a bunch of XSL-related tests are unhappy with this change:


+fast/events/xsl-onload.xhtml
+fast/parser/external-entities-in-xslt.xml
+fast/parser/xslt-with-html.xml
+fast/xsl/document-function.xml
+fast/xsl/dtd-in-source-document.xml
+fast/xsl/exslt-node-set.xml
+fast/xsl/import-after-comment.xml
+fast/xsl/mozilla-tests.xml
+fast/xsl/sort-locale.xml
+fast/xsl/sort-unicode.xml
+fast/xsl/subframe-location.html
+fast/xsl/transform-to-html.xml
+fast/xsl/utf8-chunks.xml
+fast/xsl/xslt-doc-enc.xml
+fast/xsl/xslt-doc-noenc.xml
+http/tests/misc/location-test-xsl-style-sheet.xml
+http/tests/security/contentSecurityPolicy/xsl-allowed.php
+http/tests/security/contentSecurityPolicy/xsl-img-blocked.php
+http/tests/security/contentSecurityPolicy/xsl-redirect-allowed.html
+http/tests/security/contentSecurityPolicy/xsl-redirect-allowed2.html
+http/tests/security/contentSecurityPolicy/xsl-unaffected-by-style-src-2.php
+http/tests/security/cookies/first-party-cookie-allow-xslt.xml
+http/tests/security/cookies/third-party-cookie-blocking-xslt.xml
+http/tests/security/xss-DENIED-xsl-document-redirect.xml
+http/tests/security/xss-DENIED-xsl-document-securityOrigin.xml
+http/tests/security/xss-DENIED-xsl-document.xml
+http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml
+http/tests/security/xss-DENIED-xsl-external-entity.xml
+http/tests/xmlviewer/dumpAsText/xsl-stylesheet.xml
+http/tests/xsl/xslt-transform-with-javascript-disabled.html

[Brent Fulgham]

(In reply to comment #7)
> So it seems like a bunch of XSL-related tests are unhappy with this change:
> 
> 
> +fast/events/xsl-onload.xhtml
> +fast/parser/external-entities-in-xslt.xml
[...]

Oh, wait -- it's just a null pointer dereference. I guess m_renderView isn't always guaranteed to exist.

New patch...

[Darin Adler]

Comment on attachment 292555 [details]
WIP Patch

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

Removed review? since the comment says this is a "work in progress" patch.

> Source/WebCore/dom/Document.cpp:1762
> +    ASSERT(!m_renderView->inHitTesting());

Should we move this assertion further up to make it more deterministic? For example, before the if statement just above?

I also think this needs a simple why comment.

> Source/WebCore/page/EventHandler.cpp:1151
> +    if (m_frame.view())
> +        m_frame.view()->updateLayoutAndStyleIfNeededRecursive();

I suggest using a local variable instead of calling the function twice.

> Source/WebCore/page/FrameView.cpp:2850
> +        ASSERT(!renderView->inHitTesting());
>          renderView->setNeedsLayout();

Can we put this assertion into setNeedsLayout instead of here?

> Source/WebCore/rendering/RenderView.h:253
> +    bool inHitTesting() const { return m_inHitTesting; }

This function is only used in assertions. Maybe we should make that formal by putting it and the boolean inside #ifndef NDEBUG or #if ASSERT_ENABLED?

[Brent Fulgham]

Created attachment 292623 [details]
Patch

[Darin Adler]

Comment on attachment 292623 [details]
Patch

Looks like this was posted before you saw any of my comments, or you chose not to act on any of my comments.

[Darin Adler]

Comment on attachment 292623 [details]
Patch

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

> Source/WebCore/rendering/RenderView.cpp:188
> +    TemporaryChange<bool> hitTestRestorer(m_inHitTesting, true);

I new code, I think for object construction we should use the form that looks less like a function call and has more strict rules for types of arguments:

    TemporaryChange<bool> hitTestRestorer { m_inHitTesting, true };

[Brent Fulgham]

(In reply to comment #11)
> Comment on attachment 292623 [details]
> Patch
> 
> Looks like this was posted before you saw any of my comments, or you chose
> not to act on any of my comments.

The former. I wanted EWS to run with the assertion change to confirm that was the only problem with the implementation. I'll address your comments in a revised patch.

[Brent Fulgham]

Comment on attachment 292623 [details]
Patch

Clearing the review request since this is WIP.

[Brent Fulgham]

Comment on attachment 292623 [details]
Patch

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

>> Source/WebCore/rendering/RenderView.cpp:188
>> +    TemporaryChange<bool> hitTestRestorer(m_inHitTesting, true);
> 
> I new code, I think for object construction we should use the form that looks less like a function call and has more strict rules for types of arguments:
> 
>     TemporaryChange<bool> hitTestRestorer { m_inHitTesting, true };

Will do!

[Brent Fulgham]

Comment on attachment 292555 [details]
WIP Patch

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

>> Source/WebCore/dom/Document.cpp:1762
>> +    ASSERT(!m_renderView->inHitTesting());
> 
> Should we move this assertion further up to make it more deterministic? For example, before the if statement just above?
> 
> I also think this needs a simple why comment.

OK.

>> Source/WebCore/page/EventHandler.cpp:1151
>> +        m_frame.view()->updateLayoutAndStyleIfNeededRecursive();
> 
> I suggest using a local variable instead of calling the function twice.

Good idea!

>> Source/WebCore/page/FrameView.cpp:2850
>>          renderView->setNeedsLayout();
> 
> Can we put this assertion into setNeedsLayout instead of here?

Not without creating a RenderView-specific overload of 'setNeedsLayout'. The 'setNeedsLayout' method is defined on RenderObject, while our "inHitTesting" predicate is defined at the 'RenderView' level.

[Simon Fraser (smfr)]

rdar://problem/28675761

[Brent Fulgham]

Created attachment 292814 [details]
Patch

[Build Bot]

Comment on attachment 292814 [details]
Patch

Attachment 292814 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2370055

New failing tests:
fast/layers/prevent-hit-test-during-layout.html

[Build Bot]

Created attachment 292829 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Brent Fulgham]

It looks like 'internals.rangeForDictionaryLookupAtLocation' does not exist in iOS. I'll add skips for the platforms that don't support it.

[Brent Fulgham]

Created attachment 292836 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 292836 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        Hit testing under certain conditions can trigger a new layout. If a hit test is performed
> +        while in this state, we can make bad assessments of the render tree state.
> +
> +        Add some assertions to warn us if this is happening. Modify the hit test logic to make sure
> +        a recursive layout is performed when needed to avoid the tree being in an inconsistent state.
> +
> +        We should also assert that we are not performing a hit test when dirtying the tree because if
> +        a subframe dirties its parent frame, the parent frame will get laid out as soon as hit testing
> +        traverses to the next frame, since 'updateLayout' updates all ancestors. This will mutate the
> +        layer tree we are in the process of traversing, resulting in incorrect hit test results.

This needs to describe the underlying cause more precisely; details in the internal bug.

> LayoutTests/fast/layers/prevent-hit-test-during-layout.html:27
> +        var lookupRange = internals.rangeForDictionaryLookupAtLocation(x, y);

Why is this important?

[Brent Fulgham]

Comment on attachment 292836 [details]
Patch

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

>> LayoutTests/fast/layers/prevent-hit-test-during-layout.html:27
>> +        var lookupRange = internals.rangeForDictionaryLookupAtLocation(x, y);
> 
> Why is this important?

This call triggers the recursive hit test needed to demonstrate the problem.

[Simon Fraser (smfr)]

(In reply to comment #24)
> Comment on attachment 292836 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292836&action=review
> 
> >> LayoutTests/fast/layers/prevent-hit-test-during-layout.html:27
> >> +        var lookupRange = internals.rangeForDictionaryLookupAtLocation(x, y);
> > 
> > Why is this important?
> 
> This call triggers the recursive hit test needed to demonstrate the problem.

I know, I wanted you to explain this in a comment or the changelog.

[Brent Fulgham]

Created attachment 293024 [details]
Patch

[Brent Fulgham]

Comment on attachment 292836 [details]
Patch

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

>> Source/WebCore/ChangeLog:18
>> +        layer tree we are in the process of traversing, resulting in incorrect hit test results.
> 
> This needs to describe the underlying cause more precisely; details in the internal bug.

Done (many thanks to Zalan!)

>>>> LayoutTests/fast/layers/prevent-hit-test-during-layout.html:27
>>>> +        var lookupRange = internals.rangeForDictionaryLookupAtLocation(x, y);
>>> 
>>> Why is this important?
>> 
>> This call triggers the recursive hit test needed to demonstrate the problem.
> 
> I know, I wanted you to explain this in a comment or the changelog.

Done.

[zalan]

Comment on attachment 293024 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:1150
> +    if (auto frameView = m_frame.view())

auto*

> Source/WebCore/page/FrameView.cpp:2848
> +    if (RenderView* renderView = this->renderView()) {

auto*

> Source/WebCore/rendering/RenderView.h:253
> +#ifndef NDEBUG

#if !ASSERT_DISABLED

[Darin Adler]

Comment on attachment 292836 [details]
Patch

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

One small thing to perhaps consider as follow-up.

> Source/WebCore/rendering/RenderView.cpp:188
> +    TemporaryChange<bool> hitTestRestorer { m_inHitTesting, true };

Could put #ifndef NDEBUG around this too.

> Source/WebCore/rendering/RenderView.h:367
> +    bool m_inHitTesting { false };

Could put #ifndef NDEBUG around this too.

[Darin Adler]

Comment on attachment 292836 [details]
Patch

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

>> Source/WebCore/rendering/RenderView.cpp:188
>> +    TemporaryChange<bool> hitTestRestorer { m_inHitTesting, true };
> 
> Could put #ifndef NDEBUG around this too.

Or the same !ASSERT_DISABLED check you may have eventually landed.

>> Source/WebCore/rendering/RenderView.h:367
>> +    bool m_inHitTesting { false };
> 
> Could put #ifndef NDEBUG around this too.

Ditto.

[Simon Fraser (smfr)]

Comment on attachment 293024 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        Hit testing under certain conditions can trigger a new layout. If a hit test is performed
> +        while in this state, we can make bad assessments of the render tree state.
> +
> +        Normally, when we perform style recalc and layout as the result of a style change, we stay
> +        within the context of the current frame. Certain style changes, however, can also trigger
> +        changes in the compositing layer tree, which in turn requires updates to the parent compositing
> +        layers. When this happens, the render layers in the current context are no longer guaranteed
> +        to be valid.
> +
> +        The existing hit test logic would perform a shallow layout, which could leave the render tree
> +        in an invalid state.

Replace with:

r200971 added code to ensure that layout is up-to-date before hit testing, but
did so only for the main frame. It was still possible to enter cross-frame hit
testing with a subframe needing style recalc. In that situation, the subframe's
updateLayout() would get called, which could trigger a compositing change that
marked the parent frame as needing style recalc. A subsequent layout on the parent
frame (for example by hit testing traversing into a second subframe) could then
mutate the parent frame's layer tree while hit testing was traversing it.

> Source/WebCore/dom/Document.cpp:1758
> +    // We should not schedule a style recalc during hit testing or will will return invalid results.

Remove this, since it's inaccurate.

[Brent Fulgham]

Comment on attachment 293024 [details]
Patch

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

>> Source/WebCore/ChangeLog:19
>> +        in an invalid state.
> 
> Replace with:
> 
> r200971 added code to ensure that layout is up-to-date before hit testing, but
> did so only for the main frame. It was still possible to enter cross-frame hit
> testing with a subframe needing style recalc. In that situation, the subframe's
> updateLayout() would get called, which could trigger a compositing change that
> marked the parent frame as needing style recalc. A subsequent layout on the parent
> frame (for example by hit testing traversing into a second subframe) could then
> mutate the parent frame's layer tree while hit testing was traversing it.

OK!

>> Source/WebCore/dom/Document.cpp:1758
>> +    // We should not schedule a style recalc during hit testing or will will return invalid results.
> 
> Remove this, since it's inaccurate.

OK!

>> Source/WebCore/page/EventHandler.cpp:1150
>> +    if (auto frameView = m_frame.view())
> 
> auto*

OK!

>> Source/WebCore/page/FrameView.cpp:2848
>> +    if (RenderView* renderView = this->renderView()) {
> 
> auto*

OK!

>> Source/WebCore/rendering/RenderView.h:253
>> +#ifndef NDEBUG
> 
> #if !ASSERT_DISABLED

OK!

[Brent Fulgham]

Committed r208003: <http://trac.webkit.org/changeset/208003>