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

Issue 688774 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Crash in blink::LayoutMultiColumnFlowThread::flowThreadDescendantWillBeRemoved

Project Member Reported by ClusterFuzz, Feb 5 2017

Issue description

Comment 1 by tkent@chromium.org, Feb 5 2017

Components: Blink>Layout>MultiCol
Cc: msrchandra@chromium.org
Labels: Test-Predator-Correct-CLs
Owner: nasko@chromium.org
Status: Assigned (was: Untriaged)
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.

Comment 3 by nasko@chromium.org, Feb 6 2017

Owner: ----
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.

Comment 4 by nasko@chromium.org, Feb 6 2017

Status: Untriaged (was: Assigned)

Comment 5 by e...@chromium.org, Feb 7 2017

Labels: -Pri-1 Pri-2
Owner: msten...@opera.com
Status: Assigned (was: Untriaged)

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

Cc: wangxianzhu@chromium.org
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.
tc.html
406 bytes View Download
Cc: chrishtr@chromium.org
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.)

Comment 8 by msten...@opera.com, 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.
> 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?

Comment 10 by msten...@opera.com, 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).
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"

Correction: the example is not about "out-of-flow positioned descendants of LayoutVideo", but just a normal block.

Comment 14 by msten...@opera.com, Feb 16 2017

Cc: msten...@opera.com
Owner: wangxianzhu@chromium.org
Status: Fixed (was: Assigned)
Project Member

Comment 15 by ClusterFuzz, 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.
Project Member

Comment 16 by ClusterFuzz, 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