Bug 190375 - REGRESSION (r236512): [ Mac WK2 ] Layout Test editing/undo/undo-smart-delete-word.html is flaky
Summary: REGRESSION (r236512): [ Mac WK2 ] Layout Test editing/undo/undo-smart-delete-...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-08 15:56 PDT by Truitt Savell
Modified: 2018-10-16 08:02 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.76 KB, patch)
2018-10-15 12:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Truitt Savell 2018-10-08 15:56:55 PDT
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
Comment 1 Radar WebKit Bug Importer 2018-10-10 16:40:43 PDT
<rdar://problem/45177807>
Comment 2 Chris Dumez 2018-10-11 15:30:51 PDT
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.
Comment 3 Chris Dumez 2018-10-15 12:16:29 PDT
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
Comment 4 Chris Dumez 2018-10-15 12:20:58 PDT
When it fails, WebEditorClient::canRedo() returns false.
Comment 5 Chris Dumez 2018-10-15 12:21:18 PDT
(In reply to Chris Dumez from comment #4)
> When it fails, WebEditorClient::canRedo() returns false.

I meant WebEditorClient::canUndo().
Comment 6 Chris Dumez 2018-10-15 12:35:52 PDT
Created attachment 352361 [details]
Patch
Comment 7 WebKit Commit Bot 2018-10-15 17:33:04 PDT
Comment on attachment 352361 [details]
Patch

Clearing flags on attachment: 352361

Committed r237159: <https://trac.webkit.org/changeset/237159>
Comment 8 WebKit Commit Bot 2018-10-15 17:33:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Geoffrey Garen 2018-10-16 08:02:59 PDT
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.