197438 2019-04-30 13:42:23 -0700 REGRESSION(r244494): editing/pasteboard/5761530-1.html is flaky due to a simple line layout bug 2019-05-01 15:38:04 -0700 1 1 1 Unclassified WebKit HTML Editing WebKit Nightly Build Unspecified Unspecified REOPENED InRadar P2 Normal --- 1 rniwa zalan commit-queue koivisto tsavell webkit-bug-importer wenson_hsieh zalan oldest_to_newest 1531686 0 rniwa 2019-04-30 13:42:23 -0700 Truitt Savell 2019-04-30 09:56:55 PDT It looks like the changes in https://trac.webkit.org/changeset/244494/webkit has caused the test editing/pasteboard/5761530-1.html to become flakey. I was able to reproduce this using command: run-webkit-tests editing/pasteboard/5761530-1.html --iterations 20 --ios-simulator The test passes on 244493 and with fail on 244494 History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=editing%2Fpasteboard%2F5761530-1.html Diff: --- /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-results/editing/pasteboard/5761530-1-expected.txt +++ /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-results/editing/pasteboard/5761530-1-actual.txt @@ -1,3 +1,3 @@ This tests to see that tabs are put into tab spans when they are copied individually. The pasted tab should be inside of a tab span, not a style span. To run the test manually, paste and then inspect the editable region, and ensure that there is a tab span at the beginning of the editable div. -<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space:pre;"> </span>xxx +<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space:pre;"> </span>xxx 1531688 1 webkit-bug-importer 2019-04-30 13:43:20 -0700 <rdar://problem/50348091> 1531846 2 wenson_hsieh 2019-04-30 20:07:59 -0700 It was specifically this change: --- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp +++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp @@ -1125,7 +1125,7 @@ void WebPage::updateEditorStateAfterLayoutIfEditabilityChanged() Frame& frame = m_page->focusController().focusedOrMainFrame(); EditorStateIsContentEditable editorStateIsContentEditable = frame.selection().selection().isContentEditable() ? EditorStateIsContentEditable::Yes : EditorStateIsContentEditable::No; if (m_lastEditorStateWasContentEditable != editorStateIsContentEditable) - sendPartialEditorStateAndSchedulePostLayoutUpdate(); + scheduleFullEditorStateUpdate(); } String WebPage::renderTreeExternalRepresentation() const ...that causes this test to be flaky. Reverting to always send the partial editor state update immediately when changing editability fixes the flakiness for me. I'll need to see what exactly in the UI process is depending on this state being up to date. 1531859 3 wenson_hsieh 2019-04-30 22:42:59 -0700 ...this is even weirder than I thought! Simply asking whether the current selection is at the beginning of a sentence via FrameSelection::selectionAtSentenceStart (and not even using the result) in this function: Frame& frame = m_page->focusController().focusedOrMainFrame(); EditorStateIsContentEditable editorStateIsContentEditable = frame.selection().selection().isContentEditable() ? EditorStateIsContentEditable::Yes : EditorStateIsContentEditable::No; - if (m_lastEditorStateWasContentEditable != editorStateIsContentEditable) + if (m_lastEditorStateWasContentEditable != editorStateIsContentEditable) { + frame.selection().selectionAtSentenceStart(); scheduleFullEditorStateUpdate(); + } ...seems to make this test pass reliably 🤔. 1531871 4 rniwa 2019-05-01 00:49:08 -0700 Oh, I wonder this is an issue with simple line layout. Try disabling it via settings? 1531915 5 wenson_hsieh 2019-05-01 07:32:39 -0700 (In reply to Ryosuke Niwa from comment #4) > Oh, I wonder this is an issue with simple line layout. Try disabling it via > settings? Oh interesting! Adding `internals.settings.setSimpleLineLayoutEnabled(false);` to the beginning of the test does seem to fix the flakiness... 1531947 6 rniwa 2019-05-01 09:25:05 -0700 Okay. So I think this might be a bug in TextIterator / StylizedMarkupAccumulator with simple line layout code in which a tab character or 4 spaces is collapsed in its serialization even though white-space: pre is set. 1532052 7 rniwa 2019-05-01 13:53:44 -0700 It's probably a bug in TextIterator::handleTextNode. When copying the content of a plain text, we end up using TextIterator to grab the content in StyledMarkupAccumulator::appendText 1532054 8 wenson_hsieh 2019-05-01 13:59:51 -0700 (In reply to Ryosuke Niwa from comment #7) > It's probably a bug in TextIterator::handleTextNode. When copying the > content of a plain text, we end up using TextIterator to grab the content in > StyledMarkupAccumulator::appendText Yep, I'm poking around in and around this code at the moment :) I'll post some more findings in a bit. There's some behaviors regarding the lifetime of a renderer's SLL that are currently confusing me. 1532064 9 368708 rniwa 2019-05-01 14:16:18 -0700 Created attachment 368708 Reduction Here's a reduction of the issue Zalan and I worked. 1532069 10 wenson_hsieh 2019-05-01 14:22:20 -0700 Alan, Ryosuke and I chatted about this over IRC. For now, let's disable SLL in this test to make it pass; Alan will continue investigating a fix in SLL logic to get innerText working for tab-spans in SLL. 1532077 11 368714 wenson_hsieh 2019-05-01 14:37:18 -0700 Created attachment 368714 Patch 1532109 12 368714 commit-queue 2019-05-01 15:34:09 -0700 Comment on attachment 368714 Patch Clearing flags on attachment: 368714 Committed r244854: <https://trac.webkit.org/changeset/244854> 1532110 13 commit-queue 2019-05-01 15:34:10 -0700 All reviewed patches have been landed. Closing bug. 1532111 14 wenson_hsieh 2019-05-01 15:38:04 -0700 Reopening to track the SLL issue described above. 368708 2019-05-01 14:16:18 -0700 2019-05-01 14:16:18 -0700 Reduction 5761530-1.html text/html 219 rniwa PCFET0NUWVBFIGh0bWw+CjxodG1sPgo8Ym9keT4KYQliCjxzY3JpcHQ+CmNvbnN0IHJlc3VsdCA9 IGRvY3VtZW50LmJvZHkuaW5uZXJUZXh0Owpkb2N1bWVudC5ib2R5LnRleHRDb250ZW50ID0gJyc7 CmRvY3VtZW50LndyaXRlKGBUaGVyZSBzaG91bGQgYmUgYSB0YWIgY2hhcmFjdGVyIGJlbG93Ojxw cmU+JHtyZXN1bHR9PC9wcmU+YCk7Cjwvc2NyaXB0Pgo8L2JvZHk+CjwvaHRtbD4K 368714 2019-05-01 14:37:18 -0700 2019-05-01 15:34:09 -0700 Patch bug-197438-20190501143718.patch text/plain 1912 wenson_hsieh U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ0ODQwCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggODUyNWE0MTNjNjQzNTA0ZjQwZTEy NTg5Y2NjYTIzYzIxZWNhZGM1OC4uMjE1MDVhNzE5M2ExOTMwNjM0YzhmYjNiMWVkOTM3ZjUwNTE2 OWJiMSAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE5LTA1LTAxICBXZW5zb24gSHNpZWggIDx3 ZW5zb25faHNpZWhAYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04ocjI0NDQ5NCk6IGVk aXRpbmcvcGFzdGVib2FyZC81NzYxNTMwLTEuaHRtbCBpcyBmbGFreSBkdWUgdG8gYSBzaW1wbGUg bGluZSBsYXlvdXQgYnVnCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0xOTc0MzgKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzUwMzQ4MDkxPgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdvcmsgYXJvdW5kIHRo ZSBzaW1wbGUgbGluZSBsYXlvdXQgYnVnIGluIHRoZSBmbGFreSBsYXlvdXQgdGVzdCBieSBkaXNh Ymxpbmcgc2ltcGxlIGxpbmUgbGF5b3V0LgorCisgICAgICAgICogZWRpdGluZy9wYXN0ZWJvYXJk LzU3NjE1MzAtMS5odG1sOgorCiAyMDE5LTA0LTI4ICBXZW5zb24gSHNpZWggIDx3ZW5zb25faHNp ZWhAYXBwbGUuY29tPgogCiAgICAgICAgIFtpT1NdIEFkZCBhIHZlcnNpb24gb2Ygdmlld3BvcnQg c2hyaW5rLXRvLWZpdCBoZXVyaXN0aWNzIHRoYXQgcHJlc2VydmVzIHBhZ2UgbGF5b3V0CmRpZmYg LS1naXQgYS9MYXlvdXRUZXN0cy9lZGl0aW5nL3Bhc3RlYm9hcmQvNTc2MTUzMC0xLmh0bWwgYi9M YXlvdXRUZXN0cy9lZGl0aW5nL3Bhc3RlYm9hcmQvNTc2MTUzMC0xLmh0bWwKaW5kZXggYTA4ODRk YTUxYjFlMDM4NzM0NTVkZTU5ZTI1NWZkMGZlMzA3YmM4OS4uOTAzZmE1ZDY2Njc5ODZhZWQ5NjQz NDg4MTBkMTA3Y2VhZjk3NmY3ZSAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvZWRpdGluZy9wYXN0 ZWJvYXJkLzU3NjE1MzAtMS5odG1sCisrKyBiL0xheW91dFRlc3RzL2VkaXRpbmcvcGFzdGVib2Fy ZC81NzYxNTMwLTEuaHRtbApAQCAtMSw4ICsxLDEwIEBACiA8ZGl2IGlkPSJkZXNjcmlwdGlvbiI+ VGhpcyB0ZXN0cyB0byBzZWUgdGhhdCB0YWJzIGFyZSBwdXQgaW50byB0YWIgc3BhbnMgd2hlbiB0 aGV5IGFyZSBjb3BpZWQgaW5kaXZpZHVhbGx5LiAgVGhlIHBhc3RlZCB0YWIgc2hvdWxkIGJlIGlu c2lkZSBvZiBhIHRhYiBzcGFuLCBub3QgYSBzdHlsZSBzcGFuLiAgVG8gcnVuIHRoZSB0ZXN0IG1h bnVhbGx5LCBwYXN0ZSBhbmQgdGhlbiBpbnNwZWN0IHRoZSBlZGl0YWJsZSByZWdpb24sIGFuZCBl bnN1cmUgdGhhdCB0aGVyZSBpcyBhIHRhYiBzcGFuIGF0IHRoZSBiZWdpbm5pbmcgb2YgdGhlIGVk aXRhYmxlIGRpdi48L2Rpdj4KIDxkaXYgaWQ9ImVkaXQiIGNvbnRlbnRlZGl0YWJsZT0idHJ1ZSI+ PHNwYW4gY2xhc3M9IkFwcGxlLXRhYi1zcGFuIiBzdHlsZT0id2hpdGUtc3BhY2U6cHJlOyI+CTwv c3Bhbj54eHg8L2Rpdj4KIDxzY3JpcHQ+Ci1pZiAod2luZG93LnRlc3RSdW5uZXIpCi0Jd2luZG93 LnRlc3RSdW5uZXIuZHVtcEFzVGV4dCgpOworaWYgKHdpbmRvdy50ZXN0UnVubmVyICYmIHdpbmRv dy5pbnRlcm5hbHMpIHsKKyAgICB3aW5kb3cudGVzdFJ1bm5lci5kdW1wQXNUZXh0KCk7CisgICAg aW50ZXJuYWxzLnNldHRpbmdzLnNldFNpbXBsZUxpbmVMYXlvdXRFbmFibGVkKGZhbHNlKTsKK30K IGVkaXQgPSBkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgiZWRpdCIpOwogCiB3aW5kb3cuZ2V0U2Vs ZWN0aW9uKCkuc2V0UG9zaXRpb24oZWRpdCwgMCk7Cg==