Bug 190375 – REGRESSION (r236512): [ Mac WK2 ] Layout Test editing/undo/undo-smart-delete-word.html is flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Truitt Savell]

The following layout test is flaky on Mac WK2

editing/undo/undo-smart-delete-word.html

Probable cause:

This is fallout from https://trac.webkit.org/changeset/236512/webkit. 

reproduced with command: run-webkit-tests --root testbuild-236512 editing/undo/undo-smart-delete-word.html --iterations 500 -f
No failure occurs on r236511

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=editing%2Fundo%2Fundo-smart-delete-word.html

Diff:
--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/editing/undo/undo-smart-delete-word-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/editing/undo/undo-smart-delete-word-actual.txt
@@ -1,4 +1,8 @@
+CONSOLE MESSAGE: line 28: [object Text]
+CONSOLE MESSAGE: line 29: 4
+CONSOLE MESSAGE: line 30: [object Text]
+CONSOLE MESSAGE: line 31: 4
 Tests: 
 Select a word via double-click. Delete. Then undo the delete. The space that got smart deleted should now be selected.
-foo bar baz
-PASSED
+foo baz
+FAILED

[Radar WebKit Bug Importer]

<rdar://problem/45177807>

[Chris Dumez]

It seems to be related to the following IPC:
m_page->process().send(Messages::WebPage::UnapplyEditCommand(m_commandID), m_page->pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);

Which sometimes gets processed out of order.

[Chris Dumez]

If I dump editing callback, I see the following diff when it fails:
@@ -8,10 +8,11 @@
 EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 3 of #text > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
-EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+CONSOLE MESSAGE: line 30: [object Text]
+CONSOLE MESSAGE: line 31: 4
+CONSOLE MESSAGE: line 32: [object Text]
+CONSOLE MESSAGE: line 33: 4
 Tests:
 Select a word via double-click. Delete. Then undo the delete. The space that got smart deleted should now be selected.
-foo bar baz
-PASSED
+foo baz
+FAILED

[Chris Dumez]

When it fails, WebEditorClient::canRedo() returns false.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> When it fails, WebEditorClient::canRedo() returns false.

I meant WebEditorClient::canUndo().

[Chris Dumez]

Created attachment 352361 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 352361 [details]
Patch

Clearing flags on attachment: 352361

Committed r237159: <https://trac.webkit.org/changeset/237159>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

I find “DispatchMessageEvenWhenWaitingForSyncReply” to be a very hard to digest phrase. One reason is that it’s a send option, but it actually governs the behavior of the recipient and not the sender. I’m not sure how to fix that. 

Maybe a better name would be “CanDispatchDuringSyncWait”. 

And maybe a better design would be for the sync messages to explicitly list the async message types they depend on. So, for example, the undo message would specify [ RegisterEditCommandForUndo ] as the list of message types to process before it during a sync wait interruption. In the normal case this would do nothing. But in the sync wait interruption case for undo (and only undo), this would scan the queue and process all RegisterEditCommandForUndo messages eagerly.