New issue
Advanced search Search tips

Issue 781698 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 707656



Sign in to add a comment

Caret{Min,Max}Offset() in VisibleUnits.cpp should be called with layout clean

Project Member Reported by yosin@chromium.org, Nov 6 2017

Issue description

Because of Caret{Min,Max}Offset() call LayoutObject::Caret{Min,Max}Offset(),
we should call them with layout clean
 

Comment 1 by yosin@chromium.org, Nov 6 2017

Blocking: 707656
When we enable LayoutNG and LayoutNGPaintFragments, following tests failed by
calling CaretMinOffset() with dirty layout:

- fast/inline/inline-box-adjust-position-crash.html
- fast/text/selection/first-letter-bad-line-boxes-crash.html
- fast/text/selection/insert-text-crash.html

Comment 2 by yosin@chromium.org, Nov 6 2017

Example call stack:


crash log for renderer (pid <unknown>):
STDOUT: #CRASHED - renderer
STDERR: [167824:35096:1106/170344.564:FATAL:ng_inline_node.cc(426)] Check failed: !GetLayoutBlockFlow()->GetDocument().NeedsLayoutTreeUpdate(). 
STDERR: Backtrace:
STDERR: 	base::debug::StackTrace::StackTrace [0x0290CDB6+102] (C:\src\w\cr2\src\base\debug\stack_trace_win.cc:286)
STDERR: 	base::debug::StackTrace::StackTrace [0x0290B953+35] (C:\src\w\cr2\src\base\debug\stack_trace.cc:199)
STDERR: 	logging::LogMessage::~LogMessage [0x02981935+149] (C:\src\w\cr2\src\base\logging.cc:581)
STDERR: 	blink::NGInlineNode::ComputeOffsetMappingIfNeeded [0x0CD6E318+232] (C:\src\w\cr2\src\third_party\WebKit\Source\core\layout\ng\inline\ng_inline_node.cc:428)
STDERR: 	blink::NGOffsetMapping::GetFor [0x0CD89B84+452] (C:\src\w\cr2\src\third_party\WebKit\Source\core\layout\ng\inline\ng_offset_mapping.cc:142)
STDERR: 	blink::LayoutText::GetNGOffsetMapping [0x0CC58ADE+78] (C:\src\w\cr2\src\third_party\WebKit\Source\core\layout\LayoutText.cpp:1919)
STDERR: 	blink::LayoutText::CaretMinOffset [0x0CC59910+32] (C:\src\w\cr2\src\third_party\WebKit\Source\core\layout\LayoutText.cpp:1965)
STDERR: 	blink::CaretMinOffset [0x0BD80E1C+76] (C:\src\w\cr2\src\third_party\WebKit\Source\core\editing\VisibleUnits.cpp:825)
STDERR: 	blink::DeleteSelectionCommand::HandleGeneralDelete [0x0BDD9C8D+2861] (C:\src\w\cr2\src\third_party\WebKit\Source\core\editing\commands\DeleteSelectionCommand.cpp:714)
STDERR: 	blink::DeleteSelectionCommand::DoApply [0x0BDDD153+2019] (C:\src\w\cr2\src\third_party\WebKit\Source\core\editing\commands\DeleteSelectionCommand.cpp:1148)
STDERR: 	blink::CompositeEditCommand::ApplyCommandToComposite [0x0BDBE1B1+81] (C:\src\w\cr2\src\third_party\WebKit\Source\core\editing\commands\CompositeEditCommand.cpp:205)
STDERR: 	blink::CompositeEditCommand::DeleteSelection [0x0BDC2244+244] (C:\src\w\cr2\src\third_party\WebKit\Source\core\editing\commands\CompositeEditCommand.cpp:614)
STDERR: 	blink::CompositeEditCommand::MoveParagraphs [0x0BDC6E90+3488] (C:\src\w\cr2\src\third_party\WebKit\Source\core\editing\commands\CompositeEditCommand.cpp:1497)
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 7 2017

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

commit 9839c66152d44a7cbce5c767365e61085cc44b81
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Tue Nov 07 14:05:37 2017

Make DeleteSelectionCommand to call CaretMinOffset() with clean layout

This patch makes |DeleteSelectionCommand::HandleGeneralDelete()| to call
|CaretMinOffset()| with clean layout to prevent crash in following layout
tests:
- fast/inline/inline-box-adjust-position-crash.html
- fast/text/selection/first-letter-bad-line-boxes-crash.html
- fast/text/selection/insert-text-crash.html
when enabling LayoutNG and LayoutNGPaintFragments.

Because of |CaretMinOffset()| calls |LayoutObject::CaretMinOffset()|, we
should call |CaretMinOffset()| with clean layout even if we don't enable
LayoutNG.

Note: fast/inline/inline-box-adjust-position-crash.html is crash when
enabling only LayoutNG in |InsertListCommand::DoApply()| at
 DCHECK_EQ(should_be_later, CanonicalPositionOf(should_be_later))

Bug:  781698 
Change-Id: I1599082faf5cd069508b910aa39c558cfd30383d
Reviewed-on: https://chromium-review.googlesource.com/753274
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514460}
[modify] https://crrev.com/9839c66152d44a7cbce5c767365e61085cc44b81/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 20 2017

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

commit 169389495c8c62485d4f831892baf3cfaf7eae0e
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Mon Nov 20 07:55:39 2017

Ensure clean layout before calling LayoutText::CaretMaxOffset

LayoutText::CaretMaxOffset() requires clean layout, but currently can be
called from DeleteSelectionCommand when layout is dirty. This patch adds
layout update before the call sites to fix it.

Bug:  781698 
Change-Id: I17b7596e93f025d61836e39cca4bf736713d3422
Reviewed-on: https://chromium-review.googlesource.com/778050
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517778}
[modify] https://crrev.com/169389495c8c62485d4f831892baf3cfaf7eae0e/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
[modify] https://crrev.com/169389495c8c62485d4f831892baf3cfaf7eae0e/third_party/WebKit/Source/core/layout/LayoutText.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 21 2017

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

commit b4ad5f1eaf81bde99af7aa2a5f4c9a897ecdf816
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Nov 21 01:49:51 2017

Ensure clean layout for another caller of CaretMin/MaxOffset

LayoutText::CaretMin/MaxOffset() should be called with clean layout,
but is not currently ensured at all call sites. This patch adds layout
update for one more call site to improve the current status.

Bug:  781698 
Change-Id: I63275826f75f3f3621ec8ffb1b9ed750a4ccdb35
Reviewed-on: https://chromium-review.googlesource.com/780079
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518052}
[modify] https://crrev.com/b4ad5f1eaf81bde99af7aa2a5f4c9a897ecdf816/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 21 2017

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

commit a8abb656fd4031387424d3897986b21c55844232
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Nov 21 04:53:16 2017

DCHECK clean layout in LayoutText::CaretMin/MaxOffset()

As now we haven't found any caller of LayoutText::CaretMin/MaxOffset()
with dirty layout, this patch adds DCHECK of clean layout into
the two functions to enforce the clean layout requirement.

Bug:  781698 
Change-Id: Id55463a71f309a58906ee65e1f89faa57f8708c6
Reviewed-on: https://chromium-review.googlesource.com/777956
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518120}
[modify] https://crrev.com/a8abb656fd4031387424d3897986b21c55844232/third_party/WebKit/Source/core/layout/LayoutText.cpp

Status: Fixed (was: Available)

Sign in to add a comment