Crash in blink::LayoutMultiColumnFlowThread::flowThreadDescendantWillBeRemoved |
||||||||
Issue descriptionDetailed report: https://cluster-fuzz.appspot.com/testcase?key=6629065567240192 Fuzzer: bj_broddelwerk Job Type: linux_asan_chrome_mp Platform Id: linux Crash Type: UNKNOWN READ Crash Address: 0x000000000000 Crash State: blink::LayoutMultiColumnFlowThread::flowThreadDescendantWillBeRemoved blink::LayoutObject::removeFromLayoutFlowThreadRecursive blink::LayoutObject::willBeRemovedFromTree Sanitizer: address (ASAN) Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=441524:442831 Reproducer Testcase: https://cluster-fuzz.appspot.com/download/AMIfv95IbxvpjgFcJWbnz9_tDYQyLAF4l4s5l7I4_qBGFGfocaU6SISDFVjBDgF8iYHXJebc54nkd2lJLgdmYrYilx7YReKI8JoC-UXEKHoxH99QppSNUKGfZePtDuwju4D97YmHfsgr4oll1eSN4QfpKPBDeWymC7oVoW3myEAiQaebV3Kn0hm4VslQyCGqSiqVAD1DJp466Y2PpyGk4rrO305rwk5FRPUC_Zx_gA5nf5gzyW0TlvpZ3c0UsLgyiswuBAHEboJKw_JYJWEm1RnQ9roy7_XACuLPlhskjIaIuEC5btuOJ0F9mYJoZBJ11J_0zsiReb_tJZeLo_kk7tGHVf4XydChbDlpv_Gf-LWHlG5gsEW37c5FTaKc0CJPJsSLwyyhTk7ZN8D4rzvsv63Jbou2_-s3KA?testcase_id=6629065567240192 Issue filed automatically. See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Feb 6 2017
Assigning to the concern owner from Predator results -- The result is a list of CLs that change the crashed files. Author: nasko Project: chromium Changelist: https://chromium.googlesource.com/chromium/src/+/dc8651b80ac4d1faf2b864eb9fd01b8a5534ef7b Time: Sat Jan 07 04:37:35 2017 File LayoutObject.cpp is changed in this cl (and is part of stack frame #3, "blink::LayoutObject::removeFromLayoutFlowThreadRecursive"; frame #4, "blink::LayoutObject::willBeRemovedFromTree") Minimum distance from crash line to modified line: 18. (file: LayoutObject.cpp, crashed on: 2721, modified: 2739). @nasko -- Could you please look into the issue, kindly re-assign if this is not related to your changes. Thank You.
,
Feb 6 2017
The CL blamed is just a change of names, it doesn't have impact on actual code logic. I'm removing myself as an owner and it should be assigned to an owner in layout.
,
Feb 6 2017
,
Feb 7 2017
,
Feb 13 2017
Instead of reducing the fuzzed test case (which is in "random_number % aoElements.length" style, which is next to impossible to work with), I used gdb, and came up with a reduction (attached). Before running the script in the attached test case, we have this layout tree:
*LayoutView 0x43949604010 #document
LayoutBlockFlow 0x4394961c010 HTML
LayoutBlockFlow 0x4394961c138 BODY
LayoutBlockFlow 0x4394961c260 DIV style="columns:3;"
LayoutMultiColumnFlowThread (anonymous) 0x43949628010
LayoutInline 0x43949638010 SPAN id="sibling"
LayoutText 0x439496380d8 #text "\n "
LayoutVideo (positioned) 0x43949654010 VIDEO id="video" style="position:absolute;"
LayoutFlexibleBox (relative positioned) (floating) 0x43949664010 DIV
LayoutFlexibleBox (relative positioned) 0x439496641e0 DIV
LayoutBlockFlow 0x4394961c388 DIV
LayoutMultiColumnSet (anonymous) 0x43949648010
The out-of-flow positioned LayoutVideo object isn't contained by the multicol container, since the multicol container is its parent and is statically positioned. Therefore it shouldn't contribute to creating a LayoutMultiColumnSet. However, it does, because its ::-webkit-media-controls pseudo-element child (the outer LayoutFlexibleBox) manages to escape its containing block (the LayoutVideo). Unless I remember incorrectly, this problem is introduced by a recent change, more precisely https://codereview.chromium.org/2575423003 , which made sure to have LayoutObject::container() return a real LayoutBlock (and not just the parent()) when asking for the container() of a float (and this is achieved by delegating to containingBlock()). And LayoutVideo isn't a real block (it's LayoutReplaced). Now, how we're able to lay out anything at all inside a non-block like LayoutVideo is kind of a mystery to me, but at least shadow DOM is involved for ::-webkit-media-controls. It appears that LayoutVideo serves as a block container for its children, but containingBlock() cannot return it for children, since LayoutVideo isn't a LayoutBlock. This causes confusion for multicol, and other parts of the engine too. Simply trying to schedule a VIDEO with a floated control for paint should cause an assertion failure. No multicol required.
Back to the test: we can get the multicol machinery to kill the LayoutMultiColumnSet while there are still elements that require it to be there (i.e. the floated ::-webkit-media-controls pseudo element), by first removing the SPAN LayoutInline. When the SPAN is removed, all that's left is the absolutely positioned LayoutVideo. The multicol machinery will go ahead and remove the LayoutMultiColumnSet now, since there's really no column contents left. The problem is that the ::-webkit-media-controls outer LayoutFlexibleBox *is* (incorrectly) regarded at column contents in some sense, since its containingBlock() is the LayoutMultiColumnFlowThread. If we attempt to remove the LayoutVideo, we'll remove the ::-webkit-media-controls outer floated LayoutFlexibleBox, and since it believes that it is part of the flow thread, it will try to locate a LayoutMultiColumnSet (and expect to find one), which has already been deleted while removing the SPAN LayoutInline.
Not sure how to fix this yet. It may be that we need a multicol-specific fix, but the problem isn't really a multicol one.
,
Feb 14 2017
I don't have an idea, just 2 questions: - According to the spec, what's the 'containing block' of the floating div? - Which LayoutBlock should paint the floating div if its position is static? (I tried to add 'position:static' to prevent the floating div from being a self-painting layer, but it crashes on DCHECK(nextContainer) in LayoutObject::offsetFromAncestorContainer() during compositing update.)
,
Feb 14 2017
According to the spec, the containing block of a float is the same as the containing block of a regular block. In the test, the float is relatively positioned, but it shouldn't really matter whether it has position:static or position:relative. The most natural object to paint the float would in any case be its containing block. So, what you did in https://codereview.chromium.org/2575423003 makes a lot of sense. Any (non-atomic - e.g. regular SPANs) inline in the ancestry chain between the float and its containing block should be skipped. The problematic part is the LayoutVideo ancestor, which now is skipped too, since it's not a LayoutBlock. But it appears that the LayoutVideo serves as a containing block for its children somehow. I haven't looked at how this actually done yet.
,
Feb 14 2017
> In the test, the float is relatively positioned, but it shouldn't really matter whether it has position:static or position:relative. The most natural object to paint the float would in any case be its containing block.
In our current code, the position affects how the float is painted because with position:relative the float has a self-painting layer so it paints itself without needing its containing block to paint it. I tried position:static to see how LayoutBlockFlow and BlockFlowPainter handle such situation, but it seemed broken.
I think the case can be simplified as the following:
<multicol>
<out-of-flow-positioned non-block>
<float>
Is there a way to create an out-of-flow-positioned non-block using normal DOM?
,
Feb 15 2017
No, that's the thing. You cannot have a non-block contain things like that. Only blocks can establish block formatting contexts (to contain floats). VIDEO is a replaced element, and a replaced element is defined here: https://www.w3.org/TR/CSS22/conform.html#replaced-element In Blink we use the layout engine to render certain types of certain parts of replaced content, such as VIDEO controls (and regular INPUT type=text elements and a bunch of other things).
,
Feb 15 2017
Would it be reasonable or break anything *not* to create LayoutObject for anything that would escape control of the shadow DOM root (i.e. container() of the LayoutObject would be an ancestor above the shadow DOM root)?
We seem already doing this for out-of-flow positioned descents of LayoutVideo. Not sure how this happens. For example:
<style>
#video::-webkit-media-controls { display: block; }
</style>
<video id="video"></video>
the current code doesn't create LayoutObjecvt for the media controls:
*LayoutView 0x3a267ba04010 #document
LayoutBlockFlow 0x3a267ba1c010 HTML
LayoutBlockFlow 0x3a267ba1c138 BODY
LayoutVideo 0x3a267ba28010 VIDEO id="video"
,
Feb 15 2017
Correction: the example is not about "out-of-flow positioned descendants of LayoutVideo", but just a normal block.
,
Feb 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8442090e4581bca53869f94c34a82e906a92910 commit d8442090e4581bca53869f94c34a82e906a92910 Author: Xianzhu Wang <wangxianzhu@chromium.org> Date: Thu Feb 16 07:10:36 2017 Don't allow out-of-flow positioned and floating children under LayoutMedia BUG= 688774 R=eae@chromium.org Review-Url: https://codereview.chromium.org/2702473002 . Cr-Commit-Position: refs/heads/master@{#450885} [modify] https://crrev.com/d8442090e4581bca53869f94c34a82e906a92910/third_party/WebKit/Source/core/BUILD.gn [modify] https://crrev.com/d8442090e4581bca53869f94c34a82e906a92910/third_party/WebKit/Source/core/layout/LayoutMedia.cpp [add] https://crrev.com/d8442090e4581bca53869f94c34a82e906a92910/third_party/WebKit/Source/core/layout/LayoutMediaTest.cpp
,
Feb 16 2017
,
Mar 1 2017
ClusterFuzz has detected this issue as fixed in range 450875:450937. Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6629065567240192 Fuzzer: bj_broddelwerk Job Type: linux_asan_chrome_mp Platform Id: linux Crash Type: UNKNOWN READ Crash Address: 0x000000000000 Crash State: blink::LayoutMultiColumnFlowThread::flowThreadDescendantWillBeRemoved blink::LayoutObject::removeFromLayoutFlowThreadRecursive blink::LayoutObject::willBeRemovedFromTree Sanitizer: address (ASAN) Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=441524:442831 Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=450875:450937 Reproducer Testcase: https://cluster-fuzz.appspot.com/download/AMIfv95IbxvpjgFcJWbnz9_tDYQyLAF4l4s5l7I4_qBGFGfocaU6SISDFVjBDgF8iYHXJebc54nkd2lJLgdmYrYilx7YReKI8JoC-UXEKHoxH99QppSNUKGfZePtDuwju4D97YmHfsgr4oll1eSN4QfpKPBDeWymC7oVoW3myEAiQaebV3Kn0hm4VslQyCGqSiqVAD1DJp466Y2PpyGk4rrO305rwk5FRPUC_Zx_gA5nf5gzyW0TlvpZ3c0UsLgyiswuBAHEboJKw_JYJWEm1RnQ9roy7_XACuLPlhskjIaIuEC5btuOJ0F9mYJoZBJ11J_0zsiReb_tJZeLo_kk7tGHVf4XydChbDlpv_Gf-LWHlG5gsEW37c5FTaKc0CJPJsSLwyyhTk7ZN8D4rzvsv63Jbou2_-s3KA?testcase_id=6629065567240192 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.
,
Mar 1 2017
ClusterFuzz has detected this issue as fixed in range 450875:450937. Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6629065567240192 Fuzzer: bj_broddelwerk Job Type: linux_asan_chrome_mp Platform Id: linux Crash Type: UNKNOWN READ Crash Address: 0x000000000000 Crash State: blink::LayoutMultiColumnFlowThread::flowThreadDescendantWillBeRemoved blink::LayoutObject::removeFromLayoutFlowThreadRecursive blink::LayoutObject::willBeRemovedFromTree Sanitizer: address (ASAN) Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=441524:442831 Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=450875:450937 Reproducer Testcase: https://cluster-fuzz.appspot.com/download/AMIfv95IbxvpjgFcJWbnz9_tDYQyLAF4l4s5l7I4_qBGFGfocaU6SISDFVjBDgF8iYHXJebc54nkd2lJLgdmYrYilx7YReKI8JoC-UXEKHoxH99QppSNUKGfZePtDuwju4D97YmHfsgr4oll1eSN4QfpKPBDeWymC7oVoW3myEAiQaebV3Kn0hm4VslQyCGqSiqVAD1DJp466Y2PpyGk4rrO305rwk5FRPUC_Zx_gA5nf5gzyW0TlvpZ3c0UsLgyiswuBAHEboJKw_JYJWEm1RnQ9roy7_XACuLPlhskjIaIuEC5btuOJ0F9mYJoZBJ11J_0zsiReb_tJZeLo_kk7tGHVf4XydChbDlpv_Gf-LWHlG5gsEW37c5FTaKc0CJPJsSLwyyhTk7ZN8D4rzvsv63Jbou2_-s3KA?testcase_id=6629065567240192 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 |
||||||||
Comment 1 by tkent@chromium.org
, Feb 5 2017