Bug 197438 – REGRESSION(r244494): editing/pasteboard/5761530-1.html is flaky due to a simple line layout bug

• Status: REOPENED
• Component: HTML Editing

[Ryosuke Niwa]

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

[Radar WebKit Bug Importer]

<rdar://problem/50348091>

[Wenson Hsieh]

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.

[Wenson Hsieh]

...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 🤔.

[Ryosuke Niwa]

Oh, I wonder this is an issue with simple line layout. Try disabling it via settings?

[Wenson Hsieh]

(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...

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Wenson Hsieh]

(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.

[Ryosuke Niwa]

Created attachment 368708 [details]
Reduction

Here's a reduction of the issue Zalan and I worked.

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 368714 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 368714 [details]
Patch

Clearing flags on attachment: 368714

Committed r244854: <https://trac.webkit.org/changeset/244854>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Wenson Hsieh]

Reopening to track the SLL issue described above.