Bug 163877 - Prevent hit tests from being performed on an invalid render tree
Summary: Prevent hit tests from being performed on an invalid render tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-23 14:40 PDT by Brent Fulgham
Modified: 2016-10-27 13:59 PDT (History)
10 users (show)

See Also:


Attachments
WIP Patch (5.31 KB, patch)
2016-10-23 14:42 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (2.36 MB, application/zip)
2016-10-23 22:45 PDT, Build Bot
no flags Details
Patch (5.69 KB, patch)
2016-10-24 10:27 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (9.39 KB, patch)
2016-10-25 13:43 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (deleted)
2016-10-25 14:57 PDT, Build Bot
no flags Details
Patch (12.40 KB, patch)
2016-10-25 15:33 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (12.81 KB, patch)
2016-10-27 09:05 PDT, Brent Fulgham
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-10-23 14:40:26 PDT
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.
Comment 1 Brent Fulgham 2016-10-23 14:42:24 PDT
Created attachment 292555 [details]
WIP Patch
Comment 2 Brent Fulgham 2016-10-23 14:42:45 PDT
Comment on attachment 292555 [details]
WIP Patch

Initial patch to see if this new assertion triggers too frequently.
Comment 3 zalan 2016-10-23 15:19:51 PDT
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.
Comment 4 Simon Fraser (smfr) 2016-10-23 22:02:26 PDT
(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.
Comment 5 Build Bot 2016-10-23 22:45:13 PDT
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.
Comment 6 Build Bot 2016-10-23 22:45:18 PDT
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
Comment 7 Brent Fulgham 2016-10-24 10:09:48 PDT
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
Comment 8 Brent Fulgham 2016-10-24 10:17:24 PDT
(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...
Comment 9 Darin Adler 2016-10-24 10:21:22 PDT
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?
Comment 10 Brent Fulgham 2016-10-24 10:27:55 PDT
Created attachment 292623 [details]
Patch
Comment 11 Darin Adler 2016-10-24 10:41:15 PDT
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.
Comment 12 Darin Adler 2016-10-24 10:42:09 PDT
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 };
Comment 13 Brent Fulgham 2016-10-24 12:44:07 PDT
(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.
Comment 14 Brent Fulgham 2016-10-24 12:44:27 PDT
Comment on attachment 292623 [details]
Patch

Clearing the review request since this is WIP.
Comment 15 Brent Fulgham 2016-10-24 14:30:57 PDT
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!
Comment 16 Brent Fulgham 2016-10-24 14:33:47 PDT
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.
Comment 17 Simon Fraser (smfr) 2016-10-25 10:41:04 PDT
rdar://problem/28675761
Comment 18 Brent Fulgham 2016-10-25 13:43:43 PDT
Created attachment 292814 [details]
Patch
Comment 19 Build Bot 2016-10-25 14:57:21 PDT
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
Comment 20 Build Bot 2016-10-25 14:57:26 PDT
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
Comment 21 Brent Fulgham 2016-10-25 15:24:48 PDT
It looks like 'internals.rangeForDictionaryLookupAtLocation' does not exist in iOS. I'll add skips for the platforms that don't support it.
Comment 22 Brent Fulgham 2016-10-25 15:33:49 PDT
Created attachment 292836 [details]
Patch
Comment 23 Simon Fraser (smfr) 2016-10-25 15:44:12 PDT
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?
Comment 24 Brent Fulgham 2016-10-25 17:37:46 PDT
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.
Comment 25 Simon Fraser (smfr) 2016-10-25 18:19:03 PDT
(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.
Comment 26 Brent Fulgham 2016-10-27 09:05:40 PDT
Created attachment 293024 [details]
Patch
Comment 27 Brent Fulgham 2016-10-27 09:06:32 PDT
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.
Comment 28 zalan 2016-10-27 09:24:06 PDT
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
Comment 29 Darin Adler 2016-10-27 11:41:32 PDT
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.
Comment 30 Darin Adler 2016-10-27 11:42:20 PDT
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.
Comment 31 Simon Fraser (smfr) 2016-10-27 11:54:01 PDT
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.
Comment 32 Brent Fulgham 2016-10-27 13:00:06 PDT
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!
Comment 33 Brent Fulgham 2016-10-27 13:59:09 PDT
Committed r208003: <http://trac.webkit.org/changeset/208003>