JustifyNone command causes crash with unusual HTML |
|||||||||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=4965105683660800 Fuzzer: bj_broddelwerk Job Type: linux_debug_chrome Platform Id: linux Crash Type: CHECK failure Crash Address: Crash State: !range.IsCollapsed() in SelectionTemplate.cpp blink::SelectionTemplate<blink::EditingAlgorithm<blink::NodeTraversal> >::Builde blink::ApplyStyleCommand::UpdateStartEnd Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_debug_chrome&range=529333:529334 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4965105683660800 Issue filed automatically. See https://github.com/google/clusterfuzz-tools for more information.
,
Mar 23 2018
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/10d561d9f269ad31d0c7ffa7635cf8e7d57f3078 (Ensure we keep selection direction when applying style changes). If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
,
Mar 23 2018
I'll take a look.
,
Mar 23 2018
I could reproduce the bug with the attached test case and now working on a fix.
,
Mar 26 2018
It seems that the test case is able to reach the ApplyStyleCommand::ApplyBlockStyle function with a collapsed selection, when it should normally. My patch, which caused the issue, just tried to preserves the selection direction after the style change. To do that, I just checked the old selection's WasBaseFirst() flag, which is false when the selection is collapsed, hence, using the collapsed range as argument for SetAsBackwardsSelection. @yosin, what's the reason for the DCHECK in the SetAsBackwardSelection ? Although the patch to fix the issue is very simple, I'm having difficulties to get a reduced test case that I could use as regression test for my patch. Understanding the rationale behind the above mentioned DCHECK would help.
,
Mar 26 2018
Preliminary approach in https://crrev.com/c/977927
,
Mar 27 2018
>#c15 We design SelectionTemplate::Builder to handle valid parameters only to prevent callers do wrong thing. For SetAsBackwardSelection(), passed range should be start < end, because start == end or collapsed range are considered as forward selection, start <= end.
,
Mar 27 2018
For minimal test case, how about starting with HTML just before "JustifyNone"
command? e.g.
console.log(document.documentElement.outerHTML);
document.execCommand('JustifyNone');
Because of 'JustifyNone' does:
- removes style
- move paragraph outside style block
unless starting selection is empty, resulting selection should not be empty.
For example:
^<div style="align:center">foo</div>|
=> JustifyNone
^foo|
It is suspicious that |ApplyStyleCommand::ApplyBlockStyle()| calls
|UpdateStartAndEnd()| with |start_index| and |end_index| offsets computed by
|TextIterator::RangeLength()|.
I guess we have |start_index == end_index| in this bug.
selection.modify('extend', 'backward', 'documentboundary') may create unusual selection, e.g.
|<head></head><div style="center">... svg ...</div>^
backward range selection but no characters in range.
,
Mar 29 2018
Attached an even more reduced test case. Also I attached another file with the same reduced test, but with additional logging. It seems that the problem is triggered by the last call: range.surroundContents(oParentElement) In the last reduced test, such oParentElement happens to be HTMLHeadElement, but it was an HTMLBodyElement in the original test. Is's worth mentioning that the repeated calls to the handlers triggered by the previous JS code created a weird HTML with body and head elements.
,
Mar 29 2018
ClusterFuzz has detected this issue as fixed in range 546738:546739. Detailed report: https://clusterfuzz.com/testcase?key=4965105683660800 Fuzzer: bj_broddelwerk Job Type: linux_debug_chrome Platform Id: linux Crash Type: CHECK failure Crash Address: Crash State: !range.IsCollapsed() in SelectionTemplate.cpp blink::SelectionTemplate<blink::EditingAlgorithm<blink::NodeTraversal> >::Builde blink::ApplyStyleCommand::UpdateStartEnd Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_debug_chrome&range=529333:529334 Fixed: https://clusterfuzz.com/revisions?job=linux_debug_chrome&range=546738:546739 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4965105683660800 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Mar 29 2018
ClusterFuzz testcase 4965105683660800 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Mar 29 2018
Umm, I think we still have the bug. The change I'm proposing in https://crrev.com/c/977927 is still needed. What's true is that the test case that lead to the DCHECK failure is a very tricky one, involving several event handlers and time dependent. I'm not sure I'll be able to figure out how to reproduce it consistently. The key seems to be that while processing event_handler_A8_DOMNodeRemoved triggered by the last range.surroundContents call, a previous and ongoing DOMNodeRemoved event change the selection so that it becomes backwards. This sort of events would lead to an old selection backwards, while the new one is collapsed, which shouldn't happen. Without the above mentioned change, we will trigger the CHECK failure in SelectionTemplate::SetAsBackwardSelection
,
Mar 30 2018
There is nothing in the range https://clusterfuzz.com/revisions?job=linux_debug_chrome&range=546738:546739 that fixes the issue. Actually, I was able to reproduce the crash with the attached reduced-test-case.html file in current trunk. So, reopening.
,
Mar 30 2018
,
Apr 9 2018
,
Apr 9 2018
It seems that this issue is caused by 542529. There were many other bugs that had that 542529 bug as root cause; those were resolved by WONTFIX, waiting for 542529 to be fixed. I've been investigating the editing/selection code and it seems that just using the GetDocument()->body() in IsVisuallyEquivalentCandidateAlgorithm is enough to fix this issue, so I'm not sure if we have to close this as WONTFIX as well.
,
Apr 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b8edbd670d4d65fb49f1ecad4bc4a98631dcc5d commit 6b8edbd670d4d65fb49f1ecad4bc4a98631dcc5d Author: Javier Fernandez <jfernandez@igalia.com> Date: Wed Apr 11 22:33:14 2018 IsVisuallyEquivalentCandidateAlgorithm should use GetDocument().body() Several functions in the editing logic rely in the IsHTMLBodyElement() function to determine whether a node is the body element or not. Since HTML5 spec allows multiple Body elements, we shouldn't use that function and instead compare with the result of GetDocument().body() call. The bug 542529 has been filed as a task to apply these changes. The IsVisuallyEquivalentCandidateAlgorithm function is one of those still calling to isHTMLBodyElement() to determine whether a node is the body element. The consequence is that 2 different bodies could be used as selection boundaries and produce a range selection, even if it's an empty selection (which should be collapsed). This is the root cause of the the bug 825120 , which should be fixed with this CL. Bug: 542529, 825120 Change-Id: I7f6b88e53788fb9221fc16166c7f7ba26d942bc4 Reviewed-on: https://chromium-review.googlesource.com/977927 Commit-Queue: Javier Fernandez <jfernandez@igalia.com> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/heads/master@{#549954} [modify] https://crrev.com/6b8edbd670d4d65fb49f1ecad4bc4a98631dcc5d/third_party/blink/renderer/core/editing/visible_selection_test.cc [modify] https://crrev.com/6b8edbd670d4d65fb49f1ecad4bc4a98631dcc5d/third_party/blink/renderer/core/editing/visible_units.cc [modify] https://crrev.com/6b8edbd670d4d65fb49f1ecad4bc4a98631dcc5d/third_party/blink/renderer/core/editing/visible_units_test.cc
,
Apr 12 2018
This issue should be FIXED now. |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by ClusterFuzz
, Mar 23 2018Labels: Test-Predator-Auto-Components