New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 685527 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

nextContainer in LayoutObject.cpp

Project Member Reported by ClusterFuzz, Jan 26 2017

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5206576291119104

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  nextContainer in LayoutObject.cpp
  blink::LayoutObject::offsetFromAncestorContainer
  blink::ThemePainterDefault::paintSearchFieldCancelButton
  
Sanitizer: address (ASAN)

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=317049:317420

Minimized Testcase (0.18 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv96A67vczuKY3fYxIzSfdiMZ-pjZyvpw1PKbH2ZXxiLbT5wZV3h5RyUThfPPMevAkh9X6OUsX7PoQBDxTbunDfI1zOquRWqmeeOrCk9gAO5AZw0AD5kAi4LrOid4R96HoNhcxyy0jtfVhep5FF9wpznFIJTtZh_I12QHkhYmUqo6NAZbt1FTU3ckAe4Ezb-1DdWGJCULWdOxHJe5eqErGTVgb0kdB62tjLrRwxbJHXdK8-WJqrw0mi8JY7V-rUIE907iEng9GvTrxRix7HpleWCSA2JCM7peeQ0SFgi9psLwSiu5aoe_f8duroskwVEXGfDALJRXUdrnofBtcCPKwSd7EMxNkisIaHr25QtMjbgE-uIukCQ?testcase_id=5206576291119104
<style>
#test::-webkit-search-cancel-button {
    position: absolute;
</style>
<input id="test" type="search">
<script>
    document.getElementById("test").value = "foobar";
</script>


Additional requirements: Requires Gestures

Issue filed automatically.

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Components: Blink>Layout
Labels: Test-Predator-Wrong M-56
Owner: msten...@opera.com
Status: Assigned (was: Untriaged)
Through code search on file LayoutObject.cpp, suspected CL is 
https://chromium.googlesource.com/chromium/src/+/10212c417a1326fd26f8aeee0f2f446d9f1e9793
mstensho@, could you please take a look?

Comment 2 by msten...@opera.com, Feb 1 2017

Not caused by that change. I tried to revert it (with some complications, due to d8e6e751f1e89), and it still crashes. But I'll investigate anyway.

Comment 3 by msten...@opera.com, Feb 1 2017

Cc: tkent@chromium.org
So, ThemePainterDefault::paintSearchFieldCancelButton() wants to find the offset from the cancel button to the input element, and it uses offsetFromAncestorContainer() to calculate that.

LayoutView 0x200bf5e04010              	#document
  LayoutBlockFlow 0x200bf5e1c010       	HTML
    LayoutBlockFlow 0x200bf5e1c138     	BODY
      LayoutBlockFlow 0x200bf5e1c700   	P
        LayoutText 0x200bf5e28010      	#text "Hovering the input field below should not trigger an assertion\n    failure or a crash, but rather show a search cancel button inside the input field."
      LayoutBlockFlow (anonymous) 0x200bf5e1c260
        LayoutTextControl 0x200bf5e38010	INPUT id="test"
          LayoutFlexibleBox 0x200bf5e48010	DIV id="text-field-container"
            LayoutBlockFlow 0x200bf5e1c4b0	DIV id="editing-view-port"
              LayoutBlockFlow 0x200bf5e1c5d8	DIV id="inner-editor" (editable)
                LayoutText 0x200bf5e280d8	#text "hover me"
*           LayoutBlockFlow (positioned) 0x200bf5e1c388	DIV id="search-clear"
        LayoutText 0x200bf5e281a0      	#text "\n"

So, we want the offset from #test to #search-clear. However, offsetFromAncestorContainer() walks the containing block ancestry, and since #search-clear is absolutely positioned, we'll skip #test (so that we eventually end up trying to get the container() of LayoutView), since #test is a regular statically positioned object.

There are several solutions to this, and I'm not sure which one to pick:

1. Use something better than offsetFromAncestorContainer(), so that it actually works
2. Make input elements behave as containing blocks for everything inside (i.e. so that it returns true for LayoutObject::canContainFixedPositionObjects()).
3. Force position:static on ::-webkit-search-cancel-button pseudo elements
4. ???

I'm voting for #2.
tc.html
303 bytes View Download

Comment 4 by tkent@chromium.org, Feb 2 2017

I wonder why offsetFromAncestorContainer() is necessary. Vertical centering is done by flexbox. We should be able to just paint on |cancelButtonObject|.



Comment 5 by msten...@opera.com, Feb 6 2017

The reason seems to be that we want to scale down the X to make it fit if the input field is really small (and keep the X centered). And we do this on the paint side, for some reason. Hit testing seems to still use the original size, for instance, even if it's scaled down on the paint side.

<input type="search" value="hover me and cancel the search" style="width:20em; height:10px;">

I'm pretty sure we'd be able to avoid the special-code on the paint side if we instead use flexbox for all it's worth.

Comment 6 by tkent@chromium.org, Mar 8 2017

Components: -Blink>Layout Blink>Forms>Search Blink>Paint
Cc: schenney@chromium.org
Labels: PaintTeamTriaged-20170308 BugSource-Chromium

Comment 8 by msten...@opera.com, Mar 8 2017

I'm working on solution #2 (see #c3) here: https://codereview.chromium.org/2733593002/

The code specific to ThemePainterDefault::paintSearchFieldCancelButton() should be improved independently of this. It seems like a good idea to me to guard against such situations on a general basis - hence solution #2.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f4b7a0506f076b26b76dd6aaf624860a6d3fda1

commit 8f4b7a0506f076b26b76dd6aaf624860a6d3fda1
Author: mstensho <mstensho@opera.com>
Date: Thu Mar 09 02:28:50 2017

Text control elements should contain all (shadow DOM) children.

For instance, INPUT type="search" elements allow styling of the cancel button,
via a ::-webkit-search-cancel-button pseudo element selector. We don't want authors
to be able to escape the containing INPUT element by styling the cancel button as
position:absolute, etc.

Force INPUT and other text control elements to be in the containing block chain of
all its descendants. This should be a good idea in general (and at least harmless),
and there's also C++ code [1] that essentially assumes that it is so.

Since this change makes canContainFixedPositionObjects() in LayoutObject and
ComputedStyle even more different than they used to be, we need to change some
code from using the one in ComputedStyle to the one in LayoutObject.

Two existing tests in fast/forms/ had to be updated, since they were adding together
offsetLeft of an INPUT element and offsetLeft of something inside the INPUT element
in a way that used to work by accident. Use getBoundingClientRect() instead, since
the test ultimately wants absolute coordinates anyway.

[1] See ThemePainterDefault::paintSearchFieldCancelButton()

BUG= 685527 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2733593002
Cr-Commit-Position: refs/heads/master@{#455644}

[modify] https://crrev.com/8f4b7a0506f076b26b76dd6aaf624860a6d3fda1/third_party/WebKit/LayoutTests/fast/forms/resources/common-spinbutton-change-and-input-events.js
[modify] https://crrev.com/8f4b7a0506f076b26b76dd6aaf624860a6d3fda1/third_party/WebKit/LayoutTests/fast/forms/resources/common-spinbutton-click-in-iframe.js
[add] https://crrev.com/8f4b7a0506f076b26b76dd6aaf624860a6d3fda1/third_party/WebKit/LayoutTests/fast/forms/search/abspos-cancel-button-crash.html
[modify] https://crrev.com/8f4b7a0506f076b26b76dd6aaf624860a6d3fda1/third_party/WebKit/Source/core/layout/LayoutBlock.cpp
[modify] https://crrev.com/8f4b7a0506f076b26b76dd6aaf624860a6d3fda1/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/8f4b7a0506f076b26b76dd6aaf624860a6d3fda1/third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp
[modify] https://crrev.com/8f4b7a0506f076b26b76dd6aaf624860a6d3fda1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp

Status: Fixed (was: Assigned)
Project Member

Comment 11 by ClusterFuzz, Mar 9 2017

ClusterFuzz has detected this issue as fixed in range 455565:455674.

Detailed report: https://clusterfuzz.com/testcase?key=5206576291119104

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  nextContainer in LayoutObject.cpp
  blink::LayoutObject::offsetFromAncestorContainer
  blink::ThemePainterDefault::paintSearchFieldCancelButton
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_debug_content_shell_drt&range=317049:317420
Fixed: https://clusterfuzz.com/revisions?job=linux_debug_content_shell_drt&range=455565:455674

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv96A67vczuKY3fYxIzSfdiMZ-pjZyvpw1PKbH2ZXxiLbT5wZV3h5RyUThfPPMevAkh9X6OUsX7PoQBDxTbunDfI1zOquRWqmeeOrCk9gAO5AZw0AD5kAi4LrOid4R96HoNhcxyy0jtfVhep5FF9wpznFIJTtZh_I12QHkhYmUqo6NAZbt1FTU3ckAe4Ezb-1DdWGJCULWdOxHJe5eqErGTVgb0kdB62tjLrRwxbJHXdK8-WJqrw0mi8JY7V-rUIE907iEng9GvTrxRix7HpleWCSA2JCM7peeQ0SFgi9psLwSiu5aoe_f8duroskwVEXGfDALJRXUdrnofBtcCPKwSd7EMxNkisIaHr25QtMjbgE-uIukCQ?testcase_id=5206576291119104


Additional requirements: Requires Gestures

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Sign in to add a comment