Bug 169002 – [WK2] Data interaction tests occasionally hit assertions in debug builds

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

This should help mitigate isMissingPostLayoutData assertions on debug builds.

[Wenson Hsieh]

Retitled to reflect actual purpose.

Example trace:

ASSERTION FAILED: Attempt to access post layout data before receiving it
!isMissingPostLayoutData
/Users/whsieh/work/OpenSource/Source/WebKit2/Shared/EditorState.h(137) : const WebKit::EditorState::PostLayoutData &WebKit::EditorState::postLayoutData() const
1   0x10530effd WTFCrash
2   0x1080fecbf WebKit::EditorState::postLayoutData() const
3   0x1084a610b -[WKContentView(WKInteraction) selectedTextRange]
4   0x115acd5d2 <UIKit>
5   0x115ab2a0d <UIKit>
6   0x115ab2812 <UIKit>
7   0x116418ac2 <UIKit>
8   0x11641916d <UIKit>
9   0x117c08575 __NSThreadPerformPerform
10  0x11878c451 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
11  0x118770f9f __CFRunLoopDoSources0
12  0x1187704df __CFRunLoopRun
13  0x11876fe79 CFRunLoopRunSpecific
14  0x117bbe754 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
15  0x1040a7293 TestWebKitAPI::Util::run(bool*)
16  0x103fa9faa -[DataInteractionSimulator runFrom:to:]
17  0x103f7e3b8 TestWebKitAPI::DataInteractionTests_ExternalSourceJPEGOnly_Test::TestBody()
18  0x1040e888a testing::Test::Run()
19  0x1040e96bd testing::internal::TestInfoImpl::Run()
20  0x1040ea6bd testing::TestCase::Run()
21  0x1040f0dfb testing::internal::UnitTestImpl::RunAllTests()
22  0x1040f0a79 testing::UnitTest::Run()
23  0x10402c65c TestWebKitAPI::TestsController::run(int, char**)
24  0x1040b8920 main
25  0x118ef265d start

[Wenson Hsieh]

Created attachment 303033 [details]
EWS trial run

[Tim Horton]

Comment on attachment 303033 [details]
EWS trial run

View in context: https://bugs.webkit.org/attachment.cgi?id=303033&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3319
> +        send(Messages::WebPageProxy::EditorStateChanged(editorState()), pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);

Why not inside the transaction? As you've left it, there could be an arbitrarily long delay between the layer tree commit and editor state update (at least, they are not guaranteed to happen in the same UI-side runloop cycle), when really they seem like things that should happen simultaneously.

[Wenson Hsieh]

Comment on attachment 303033 [details]
EWS trial run

View in context: https://bugs.webkit.org/attachment.cgi?id=303033&action=review

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3319
>> +        send(Messages::WebPageProxy::EditorStateChanged(editorState()), pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
> 
> Why not inside the transaction? As you've left it, there could be an arbitrarily long delay between the layer tree commit and editor state update (at least, they are not guaranteed to happen in the same UI-side runloop cycle), when really they seem like things that should happen simultaneously.

True -- this approach only means that the UI process will get an EditorState update and then (some time later) get the layer tree commit. There isn't a contract AFAIK between the layer tree update cycle and the editor state update cycle, so I don't think this change is breaking any invariants there, but it's also true that that's not how things should be. I'll investigate folding the editor state into the transaction and see how that goes -- I'm thinking of an optional<EditorState> on the layer tree transaction that's nullopt in the case where the editor state (in the web process) has not changed, and in all the places where we currently send EditorStateChanged, we would instead dirty a "editorStateNeedsUpdate" flag and schedule a layer tree commit in the web process, and in willCommitLayerTree, we check the flag and compute/send over the EditorState if needed, and then switch the flag off. Does this sound reasonable?

[Tim Horton]

(In reply to comment #4)
> Comment on attachment 303033 [details]
> EWS trial run
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303033&action=review
> 
> >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3319
> >> +        send(Messages::WebPageProxy::EditorStateChanged(editorState()), pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
> > 
> > Why not inside the transaction? As you've left it, there could be an arbitrarily long delay between the layer tree commit and editor state update (at least, they are not guaranteed to happen in the same UI-side runloop cycle), when really they seem like things that should happen simultaneously.
> 
> True -- this approach only means that the UI process will get an EditorState
> update and then (some time later) get the layer tree commit. There isn't a
> contract AFAIK between the layer tree update cycle and the editor state
> update cycle, so I don't think this change is breaking any invariants there,

No, it's not, it's just going 99% of the way to the Right Thing and then stopping short :)

> but it's also true that that's not how things should be. I'll investigate
> folding the editor state into the transaction and see how that goes -- I'm
> thinking of an optional<EditorState> on the layer tree transaction that's
> nullopt in the case where the editor state (in the web process) has not
> changed, and in all the places where we currently send EditorStateChanged,
> we would instead dirty a "editorStateNeedsUpdate" flag and schedule a layer
> tree commit in the web process, and in willCommitLayerTree, we check the
> flag and compute/send over the EditorState if needed, and then switch the
> flag off. Does this sound reasonable?

Yes, that sounds exactly right.

[Build Bot]

Comment on attachment 303033 [details]
EWS trial run

Attachment 303033 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3213210

New failing tests:
editing/inserting/insert-div-024.html
editing/inserting/insert-div-026.html
editing/style/5084241.html
editing/style/unbold-in-bold.html

[Build Bot]

Created attachment 303041 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Wenson Hsieh]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 303033 [details]
> > EWS trial run
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=303033&action=review
> > 
> > >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3319
> > >> +        send(Messages::WebPageProxy::EditorStateChanged(editorState()), pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
> > > 
> > > Why not inside the transaction? As you've left it, there could be an arbitrarily long delay between the layer tree commit and editor state update (at least, they are not guaranteed to happen in the same UI-side runloop cycle), when really they seem like things that should happen simultaneously.
> > 
> > True -- this approach only means that the UI process will get an EditorState
> > update and then (some time later) get the layer tree commit. There isn't a
> > contract AFAIK between the layer tree update cycle and the editor state
> > update cycle, so I don't think this change is breaking any invariants there,
> 
> No, it's not, it's just going 99% of the way to the Right Thing and then
> stopping short :)
> 
> > but it's also true that that's not how things should be. I'll investigate
> > folding the editor state into the transaction and see how that goes -- I'm
> > thinking of an optional<EditorState> on the layer tree transaction that's
> > nullopt in the case where the editor state (in the web process) has not
> > changed, and in all the places where we currently send EditorStateChanged,
> > we would instead dirty a "editorStateNeedsUpdate" flag and schedule a layer
> > tree commit in the web process, and in willCommitLayerTree, we check the
> > flag and compute/send over the EditorState if needed, and then switch the
> > flag off. Does this sound reasonable?
> 
> Yes, that sounds exactly right.

I talked to Ryosuke about this plan, and he disagreed with folding EditorState updates into the layer tree transaction. What we should be doing instead is simply dispatching the EditorState updates at the end of the runloop via the editorUIUpdateTimer on a zero delay. There are some special cases that need extra care though, such as ensuring the UI process gets all the updates it requires during IME, and also that the EditorState in the UI process is up to date during focused node assistance (we may have to be more aggressive in the latter case and send a post-layout-data-less EditorState update immediately before letting the UI process know about a focused node).

For this bug in particular though, I don't think it makes sense to block reenabling these tests on refactoring how EditorStates are updated. These crashes are occurring because of transient selection changes in DragController::concludeEditDrag that really should not be getting propagated to the UI process. We should just ignore selection changes while performing a drag operation, and that should fix the issue here with flaky tests.

[Wenson Hsieh]

Created attachment 304174 [details]
Patch

[Tim Horton]

r=me but bugzilla is broken so I can't set the bit on the patch

[Wenson Hsieh]

Created attachment 304333 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 304333 [details]
Patch for landing

Rejecting attachment 304333 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 304333, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
8da0 M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
fatal: unable to connect to git.webkit.org:
git.webkit.org[0: 54.190.50.177]: errno=Operation timed out

Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/3313075

[WebKit Commit Bot]

Comment on attachment 304333 [details]
Patch for landing

Clearing flags on attachment: 304333

Committed r213902: <http://trac.webkit.org/changeset/213902>