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

Issue 604095 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jun 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

ASSERTION FAILED: interval.low() == m_layoutObject->logicalTopForFloat(floatingO

Project Member Reported by ClusterFuzz, Apr 16 2016

Issue description

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

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: ASSERT
Crash Address: 
Crash State:
  ASSERTION FAILED: interval.low() == m_layoutObject->logicalTopForFloat(floatingO
  blink::ComputeFloatOffsetAdapter<
  void blink::PODIntervalTree<blink::LayoutUnit, blink::FloatingObject*>::searchFo
  

Minimized Testcase (0.33 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv96HpS36A1V1KMEesnxJ9AxTsMcrD23dwDi1bXDkPOxOHeCr17QVPxO0-0O5w4dlXlLs6b1iYWQRZjfWTufb6daCKnpkyCjX_pPcPPtpSqvTNm1xoykj-WwPlLqFDj501GSYJ6KStW18GNhn5ioP6GaQauEwnw
<style>
   #header ul {
}
#header li {
	padding: 0 0 0 9px;
}
#header a {
	float: left;
  </style>
  <div id="header">
    <li>
     <a>
      This is link two and it shouldn't overlap link one
     </a>
    </li>
</script>
<style>
   html {
  writing-mode:vertical-rl;
}
#h {
  writing-mode: horizontal-tb;
  </style>
  <div id="h">


Filer: mummareddy

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Labels: findit-wrong Te-Logged M-50
Owner: robhogan@chromium.org
Status: Assigned (was: Available)
From find-it tool:

Author: robhogan
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/4827508d33eda3f39fa999e53d11998695f45c4f
Time: Fri Jan 15 18:43:26 2016
The CL last changed line 113 of file LayoutBlockFlow.h, which is stack frame 6.

Suspected Project: chromium
Suspected Component: Blink>Layout
Cc: kojii@chromium.org
What's happening here is:

- we have a float already laid out in a container;
- we change the writing mode of the container and its child and make them both orthogonal to each other;
- when we come to layout next we call layoutOrthogonalWritingModeRoots() on the orthogonal child; 
- when we attempt to shrinkLogicalWidthToAvoidFloats() on the orthogonal child we find that its container's float list has not been updated to reflect its new writing mode (because the container hasn't been laid out yet).
- we ASSERT because the interval tree's high/low values don't match those of the container in its new writing mode.

This is very specific to when the writing mode changes on *both* container and child making both orthogonal to each other and *only then* do we get to layout on them.

I don't think there's anything in this scenario that allows us to safely skip laying out the orthogonal root and just lay it out as normal.

I'm hoping kojii might have an idea on how to resolve this circular-looking dependency - the orthogonal root seems to need its container to be laid out first and I think the same could be said for the container and its orthoganal child!




604095.html
603 bytes View Download
Cc: -kojii@chromium.org robhogan@chromium.org
Owner: kojii@chromium.org
@kojii - assigning to you so you can take a look?

This is a regression introduced by https://codereview.chromium.org/1549153002.

Happy to do up a CL if you can give some feedback on #2.

Comment 4 by kojii@chromium.org, Apr 22 2016

Apologies for a late response, I was swamped by a few RBs. I'm not really familiar with how floats work, so please consider I understand only from what you wrote in #2.

From this bug IIUC, I think there are both cases where:
A. container depends on orthogonal child's layout, and
B. orthogonal child depends on parent's layout
so blindly doing one before the other can't serve all cases.

Examples of case A is where parent has "(logical-)width: fit-content" and that it needs to know the logical-height of orthogonal children to compute preferred width. This is where layoutOrthogonalWritingModeRoots() should help.

layoutOrthogonalWritingModeRoots() has some exceptions already. Can we identify this case and add to the exceptions?

Maybe this is more complicated if we add:
  body { height: fit-content } /* logical-width of body */
We will then need to know the logical-height of #h to layout body, but the position of #h depends on floats in body to be laid out? My lack of knowledge on floats prevents me whether this can be supported nicely or not. WDYT?
The layout in layoutOrthogonalWritingModeRoots() for #h is going to be wrong in this test case. #h gets corrected in normal layout.

So I think we want to skip orthoganal root layout for #h here and the class of object it represents. Here are some of the reasons orthoganal layout will always be wrong for #h:

- its container has an orthogonal writing mode to it (obviously)
- its container hasn't laid out yet in its orthogonal writing mode yet - its current stale 'layout state' reflects one that isn't orthogonal
- its container contains floats that affect #h's width, so when it lays out it needs to avoid them

We don't have a way of detecting that the orthogonal writing mode on the container hasn't been applied to the container yet. Do we? If we do, we could use that and the fact that container has floats to skip layoutOrthogonalWritingModeRoots() for #h in this instance.

Project Member

Comment 6 by ClusterFuzz, Apr 24 2016

ClusterFuzz has detected this testcase as flaky and is unable to reproduce it in the original crash revision. Skipping fixed testing check and marking it as potentially fixed.

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

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: ASSERT
Crash Address: 
Crash State:
  ASSERTION FAILED: interval.low() == m_layoutObject->logicalTopForFloat(floatingO
  blink::ComputeFloatOffsetAdapter<
  void blink::PODIntervalTree<blink::LayoutUnit, blink::FloatingObject*>::searchFo
  

Minimized Testcase (0.33 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv96HpS36A1V1KMEesnxJ9AxTsMcrD23dwDi1bXDkPOxOHeCr17QVPxO0-0O5w4dlXlLs6b1iYWQRZjfWTufb6daCKnpkyCjX_pPcPPtpSqvTNm1xoykj-WwPlLqFDj501GSYJ6KStW18GNhn5ioP6GaQauEwnw
<style>
   #header ul {
}
#header li {
	padding: 0 0 0 9px;
}
#header a {
	float: left;
  </style>
  <div id="header">
    <li>
     <a>
      This is link two and it shouldn't overlap link one
     </a>
    </li>
</script>
<style>
   html {
  writing-mode:vertical-rl;
}
#h {
  writing-mode: horizontal-tb;
  </style>
  <div id="h">


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.

Comment 7 by kojii@chromium.org, Apr 25 2016

#5: If we skip orthogonal root layout, out layout will be incorrect when the container is fit-content (such as display: inline-block), correct?
http://jsbin.com/zahepiw/edit?html,output
i.e., if we layout #c before #h, the width of #c will not count #h.

Maybe what's wrong is here:

- when we attempt to shrinkLogicalWidthToAvoidFloats() on the orthogonal child we find that its container's float list has not been updated to reflect its new writing mode (because the container hasn't been laid out yet).

If the child is orthogonal, parent's floats should not affect logical width, do they?

Comment 8 by robho...@gmail.com, Apr 27 2016

@kojii: Yes, I think it does - at least the orthogonal child should shrink to avoid it overlapping with it. I think the test case in #2 shows this in action. Wouldn't the child overlap with the float if it didn't have to shrink to avoid it? It overlaps with it when the float and the div have the same writing mode (and thus the same formatting context, so they don't repel each other).

That said I'm not very certain that I'm correct - so am hoping you can point to another, better reason why the orthoganal flow div doesn't need to worry about avoiding floats from other formatting contexts.

Cc: cloudfuz...@gmail.com

Comment 10 Deleted

Comment 11 by kojii@chromium.org, May 29 2016

From #6, it's no longer reproducible. I'm guessing this was caused by a side effect fixed in r389691.

Shall we close this now?

Comment 12 by robho...@gmail.com, May 29 2016

No, it still asserts for me at r395275.
604095.html
603 bytes View Download

Comment 13 by kojii@chromium.org, May 29 2016

Thanks, I'll check your case.

Comment 14 by kojii@chromium.org, May 29 2016

Labels: -OS-Linux OS-All
Confirmed that the assert fires with the case in #12, thanks robhogan@.

Unlike  issue 613869 , this is not use-after-free. I guess  issue 613869  is easier to track to see where we fail to clear m_floatingObjects.m_lowestFloatBottomCache.

Comment 15 by kojii@chromium.org, May 31 2016

From your comment #8, it looks like we have different views but I can't figure it out exactly what it is, so let me try a bit more.

When shrinking logical width to avoid floats, and when it's orthogonal, it should try to shrink logical height instead. However, height is not shrinkable.

But your comment #8 reads to me that you want to shrink logical height. I also don't understand what you mean when you say "overlaps".

In case this helps, I'm attaching screenshot of test case in #2 with the WIP patch. From this screenshot:
* It no longer asserts.
* The height of "Text" is no shrinkable.
* The "Text" does not overlap with anything else.

Sorry that I just tried to explain my understanding, but from this, can you figure out what I miss?
604095-with-wip.png
9.4 KB View Download
Cc: e...@chromium.org kojii@chromium.org
Owner: robhogan@chromium.org
"When shrinking logical width to avoid floats, and when it's orthogonal, it should try to shrink logical height instead. However, height is not shrinkable."
Not with you here - the 'Text' child needs to shrinkLogicalWidthToAvoidFloats(), i.e. shrink it's width to avoid the block of vertical floating text to it's own logical right. When it tries to do this it gets into trouble because the co-ordinates of the float in its container's float list does not reflect the container's current writing mode.

We get a correct result because the orthogonal child relayouts during the 'main' layout.

Rather than removing floats in this situation I think it would be better to skip laying out the orthogonal child and let it relayout later.

Cc: -kojii@chromium.org
Owner: kojii@chromium.org
Assigning to kojii@chromium.org as this is an issue with orthogonal layout.

Comment 19 by kojii@chromium.org, Jun 10 2016

Labels: -Pri-1 Pri-3
Let me lower the priority, because this is assertion-failure-only, no crashing bug, and engineers are having hard time to come up with good resolution.

WIP here
https://codereview.chromium.org/2025543002
but I won't have time to work on it in near term.
Cc: ashej...@chromium.org
 Issue 619030  has been merged into this issue.
Project Member

Comment 21 by ClusterFuzz, Jun 13 2016

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase is verified as fixed, closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Comment 22 by kojii@chromium.org, Jun 14 2016

Cc: wkorman@chromium.org
Project Member

Comment 23 by bugdroid1@chromium.org, Sep 17 2016

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

commit 5c43382692e6b687373576984181e52c30ccd142
Author: kojii <kojii@chromium.org>
Date: Sat Sep 17 19:39:59 2016

Fix when orthogonal writing mode roots have floating siblings

When orthogonal writing mode roots have floating siblings, its
containing block may still have old or even deleted LayoutObjects.
This occurs when LayoutMultiColumnFlowThread::populate(),
LayoutBoxModelObject::moveChildrenTo() with !fullRemoveInsert,
or more, for the optimization purposes.

This patch clears such objects to be re-created when the containing
block is laid out.

BUG= 604095 ,  633409 ,  646178 

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

[add] https://crrev.com/5c43382692e6b687373576984181e52c30ccd142/third_party/WebKit/LayoutTests/fast/writing-mode/orthogonal-writing-modes-floats-crash-expected.txt
[add] https://crrev.com/5c43382692e6b687373576984181e52c30ccd142/third_party/WebKit/LayoutTests/fast/writing-mode/orthogonal-writing-modes-floats-crash.html
[modify] https://crrev.com/5c43382692e6b687373576984181e52c30ccd142/third_party/WebKit/Source/core/frame/FrameView.cpp

Project Member

Comment 24 by bugdroid1@chromium.org, Sep 20 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0a010e317a1043e7faf7160f6d2afb760d6f1f5

commit f0a010e317a1043e7faf7160f6d2afb760d6f1f5
Author: Koji Ishii <kojii@chromium.org>
Date: Tue Sep 20 13:11:28 2016

Merge 2840: Fix when orthogonal writing mode roots have floating siblings

When orthogonal writing mode roots have floating siblings, its
containing block may still have old or even deleted LayoutObjects.
This occurs when LayoutMultiColumnFlowThread::populate(),
LayoutBoxModelObject::moveChildrenTo() with !fullRemoveInsert,
or more, for the optimization purposes.

This patch clears such objects to be re-created when the containing
block is laid out.

BUG= 604095 ,  633409 ,  646178 

Review-Url: https://codereview.chromium.org/2025543002
Cr-Commit-Position: refs/heads/master@{#419376}
(cherry picked from commit 5c43382692e6b687373576984181e52c30ccd142)

Review URL: https://codereview.chromium.org/2355793002 .

Cr-Commit-Position: refs/branch-heads/2840@{#436}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[add] https://crrev.com/f0a010e317a1043e7faf7160f6d2afb760d6f1f5/third_party/WebKit/LayoutTests/fast/writing-mode/orthogonal-writing-modes-floats-crash-expected.txt
[add] https://crrev.com/f0a010e317a1043e7faf7160f6d2afb760d6f1f5/third_party/WebKit/LayoutTests/fast/writing-mode/orthogonal-writing-modes-floats-crash.html
[modify] https://crrev.com/f0a010e317a1043e7faf7160f6d2afb760d6f1f5/third_party/WebKit/Source/core/frame/FrameView.cpp

Project Member

Comment 25 by bugdroid1@chromium.org, Oct 27 2016

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

commit f0a010e317a1043e7faf7160f6d2afb760d6f1f5
Author: Koji Ishii <kojii@chromium.org>
Date: Tue Sep 20 13:11:28 2016

Merge 2840: Fix when orthogonal writing mode roots have floating siblings

When orthogonal writing mode roots have floating siblings, its
containing block may still have old or even deleted LayoutObjects.
This occurs when LayoutMultiColumnFlowThread::populate(),
LayoutBoxModelObject::moveChildrenTo() with !fullRemoveInsert,
or more, for the optimization purposes.

This patch clears such objects to be re-created when the containing
block is laid out.

BUG= 604095 ,  633409 ,  646178 

Review-Url: https://codereview.chromium.org/2025543002
Cr-Commit-Position: refs/heads/master@{#419376}
(cherry picked from commit 5c43382692e6b687373576984181e52c30ccd142)

Review URL: https://codereview.chromium.org/2355793002 .

Cr-Commit-Position: refs/branch-heads/2840@{#436}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[add] https://crrev.com/f0a010e317a1043e7faf7160f6d2afb760d6f1f5/third_party/WebKit/LayoutTests/fast/writing-mode/orthogonal-writing-modes-floats-crash-expected.txt
[add] https://crrev.com/f0a010e317a1043e7faf7160f6d2afb760d6f1f5/third_party/WebKit/LayoutTests/fast/writing-mode/orthogonal-writing-modes-floats-crash.html
[modify] https://crrev.com/f0a010e317a1043e7faf7160f6d2afb760d6f1f5/third_party/WebKit/Source/core/frame/FrameView.cpp

Project Member

Comment 26 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment