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

Issue 753515 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Cells which span rows need to have layout invalidated on visibility change to adjacent rows

Project Member Reported by joysyu@google.com, Aug 8 2017

Issue description

Chrome Version: (copy from chrome://version)
OS: (e.g. Win7, OSX 10.9.5, etc...)

What steps will reproduce the problem?
(1)use flag --enable-experimental-web-platform-features
(2)go to https://jsfiddle.net/dgrogan/sqah2j2L/
(3)click "change visibility" button twice

What is the expected result?
The browser shouldn't crash and the content in the spanning cell should toggle between the normal state and being collapsed.

What happens instead?
The browser crashes when you toggle from collapsed -> uncollapsed. FindPaintOffsetAndVisualRectNeedingUpdate::CheckVisualRect() fails. This only happens when content is clipped and collapsed.

Please use labels and text to provide additional information.


For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.

 

Comment 1 by joysyu@google.com, Aug 8 2017

LayoutView 0x3b46b9604010              	#document
  LayoutBlockFlow 0x3b46b9624010       	HTML
    LayoutBlockFlow 0x3b46b9624130     	BODY
      LayoutTable 0x3b46b96041a0       	TABLE
        LayoutTableSection 0x3b46b9630010	TBODY
          LayoutTableRow 0x3b46b9638010	TR
            LayoutTableCell 0x3b46b963c010	TD
              LayoutText 0x3b46b9644010	#text "cell1"
            LayoutTableCell 0x3b46b963c148	TD
              LayoutBlockFlow 0x3b46b9624250	DIV id="overflowdiv" style="outline: 1px solid cyan"
*               LayoutText 0x3b46b96440d8	#text "\n        Overflow Overflow Overflow"
          LayoutTableRow 0x3b46b9638120	TR id="rowToChange1" style="visibility: visible;"
            LayoutTableCell 0x3b46b963c280	TD
              LayoutText 0x3b46b96441a0	#text "hihi"
      LayoutBlockFlow (anonymous) 0x3b46b96245b0
        LayoutButton (positioned) 0x3b46b9654010	BUTTON id="changeVisibility" style="position:absolute; top:80px" (focused)
          LayoutBlockFlow (anonymous) 0x3b46b9624370
            LayoutText 0x3b46b9644268  	#text "\n  Change Visibility With Text\n"
        LayoutBlockFlow (positioned) 0x3b46b9624490	DIV style="position:absolute; top:115px"
          LayoutText 0x3b46b9644330    	#text "\nClicks on text div: "
          LayoutInline 0x3b46b9660010  	SPAN id="clickcount"
            LayoutText 0x3b46b96443f8  	#text "0"
          LayoutText 0x3b46b96444c0    	#text "\n"
        LayoutBR 0x3b46b9644588        	BR
        LayoutBR 0x3b46b9644650        	BR
        LayoutBR 0x3b46b9644718        	BR
        LayoutBR 0x3b46b96447e0        	BR
      LayoutTable 0x3b46b9604330       	TABLE
        LayoutTableSection 0x3b46b9630180	TBODY
          LayoutTableRow 0x3b46b9638230	TR
            LayoutTableCell 0x3b46b963c3b8	TD
              LayoutText 0x3b46b96448a8	#text "cell1"
            LayoutTableCell 0x3b46b963c4f0	TD
              LayoutBlockFlow 0x3b46b96246d0	DIV id="overflowingnotextdiv" style="outline: 1px solid cyan; height: 200px; width: 50px;"
          LayoutTableRow 0x3b46b9638340	TR id="rowToChange"
            LayoutTableCell 0x3b46b963c628	TD
              LayoutText 0x3b46b9644970	#text "hihi"
      LayoutBlockFlow (anonymous) 0x3b46b96247f0
        LayoutButton 0x3b46b9654190    	BUTTON id="changeVisibilityNoText"
          LayoutBlockFlow (anonymous) 0x3b46b9624910
            LayoutText 0x3b46b9644a38  	#text "\n  Change Visibility No Text\n"
        LayoutText 0x3b46b9644b00      	#text "\n\n"
[1:1:0808/140541.048996:686451584283:FATAL:FindPaintOffsetAndVisualRectNeedingUpdate.h(107)] Check failed: (old_visual_rect_.IsEmpty() && new_visual_rect.IsEmpty()) || object_.EnclosingLayer()->SubtreeIsInvisible() || old_visual_rect_ == new_visual_rect || (InflatedRect(old_visual_rect_).Contains(new_visual_rect) && InflatedRect(new_visual_rect).Contains(old_visual_rect_)). Visual rect changed without needing update object="LayoutText #text" old="57,26 241x10" new="57,26 241x23"
#0 0x7f1659ee434d base::debug::StackTrace::StackTrace()
#1 0x7f1659ee271c base::debug::StackTrace::StackTrace()
#2 0x7f1659f72e3a logging::LogMessage::~LogMessage()
#3 0x7f1651d6594c blink::FindVisualRectNeedingUpdateScopeBase::CheckVisualRect()
#4 0x7f1652ae901b blink::FindObjectVisualRectNeedingUpdateScope::~FindObjectVisualRectNeedingUpdateScope()
#5 0x7f1652ae848d blink::PaintInvalidator::UpdateVisualRectIfNeeded()
#6 0x7f1652ae68f7 blink::PaintInvalidator::InvalidatePaint()
#7 0x7f1652b4eabc blink::PrePaintTreeWalk::Walk()
#8 0x7f1652b4ebba blink::PrePaintTreeWalk::Walk()
#9 0x7f1652b4ebba blink::PrePaintTreeWalk::Walk()
#10 0x7f1652b4ebba blink::PrePaintTreeWalk::Walk()


Cc: dgro...@chromium.org
Components: Blink>Layout
Owner: joysyu@google.com
Status: Assigned (was: Untriaged)
Summary: Cells which span rows need to have layout invalidated on visibility change to adjacent rows (was: Browser crashes because visual rect needs update)
The bug is just as I thought: when changing 'visibility' CSS of one row of a table,
the border box rect of adjacent rows which have rowspan > 1 may change, if those
rows' rowspan included the row which was made to collapse via 'visibility: collapse'.

To fix this, it may be you need to add code to find other rows for which to invalidate
layout by iterating the rows and looking for rowspans, perhaps checking if those rowspans intersect the row just made collapsed.
Joy, look at LayoutTableRow::StyleDidChange. Try something like
if ((old_style->Visibility() == EVisibility::kCollapse) !=
      (Style()->Visibility() == EVisibility::kCollapse)) {
  // iterate through all the rows in the section {
    row->SetNeedsLayout();
  }
}

If that fixes it then we can get smarter about only calling setNeedsLayout on the rows that need it by inspecting grid_ in LayoutTableSection.
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 14 2017

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

commit 08f66e07f892c37693021a68088235bdc73836cc
Author: Joy Yu <joysyu@google.com>
Date: Mon Aug 14 19:08:29 2017

Properly mark cell's children as needing layout.

When a cell is spanning a collapsed row, it may get clipped. When the
row is then uncollapsed, the cell's children need to be laid out again.
Before, CellWidthChanged() determined whether the cell's children
needed layout. Now, CellWidthChanged() has been more accurately renamed
to CellChildrenNeedLayout().

Bug:  753515 
Change-Id: Ic138066b8e2b4a96deba4e67c28348ae211ec7c0
Reviewed-on: https://chromium-review.googlesource.com/610894
Commit-Queue: Joy Yu <joysyu@google.com>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494122}
[add] https://crrev.com/08f66e07f892c37693021a68088235bdc73836cc/third_party/WebKit/LayoutTests/external/wpt/css/css-tables-3/visibility-collapse-rowspan-crash.html
[modify] https://crrev.com/08f66e07f892c37693021a68088235bdc73836cc/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
[modify] https://crrev.com/08f66e07f892c37693021a68088235bdc73836cc/third_party/WebKit/Source/core/layout/LayoutTableCell.h
[modify] https://crrev.com/08f66e07f892c37693021a68088235bdc73836cc/third_party/WebKit/Source/core/layout/LayoutTableRow.cpp

Comment 5 by joysyu@google.com, Aug 14 2017

Status: Fixed (was: Assigned)

Sign in to add a comment