Bug 169002 - [WK2] Data interaction tests occasionally hit assertions in debug builds
Summary: [WK2] Data interaction tests occasionally hit assertions in debug builds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-28 16:07 PST by Wenson Hsieh
Modified: 2017-03-14 12:00 PDT (History)
2 users (show)

See Also:


Attachments
EWS trial run (4.74 KB, patch)
2017-02-28 21:20 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.02 MB, application/zip)
2017-02-28 22:42 PST, Build Bot
no flags Details
Patch (31.63 KB, patch)
2017-03-11 13:59 PST, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (32.36 KB, patch)
2017-03-13 19:00 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2017-02-28 16:07:43 PST
This should help mitigate isMissingPostLayoutData assertions on debug builds.
Comment 1 Wenson Hsieh 2017-02-28 20:36:16 PST
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
Comment 2 Wenson Hsieh 2017-02-28 21:20:59 PST
Created attachment 303033 [details]
EWS trial run
Comment 3 Tim Horton 2017-02-28 21:39:28 PST
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.
Comment 4 Wenson Hsieh 2017-02-28 21:52:03 PST
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?
Comment 5 Tim Horton 2017-02-28 22:00:01 PST
(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.
Comment 6 Build Bot 2017-02-28 22:42:00 PST
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
Comment 7 Build Bot 2017-02-28 22:42:02 PST
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
Comment 8 Wenson Hsieh 2017-03-11 13:06:27 PST
(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.
Comment 9 Wenson Hsieh 2017-03-11 13:59:09 PST
Created attachment 304174 [details]
Patch
Comment 10 Tim Horton 2017-03-13 13:37:36 PDT
r=me but bugzilla is broken so I can't set the bit on the patch
Comment 11 Wenson Hsieh 2017-03-13 19:00:31 PDT
Created attachment 304333 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2017-03-13 19:44:04 PDT
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
Comment 13 WebKit Commit Bot 2017-03-14 10:01:44 PDT
Comment on attachment 304333 [details]
Patch for landing

Clearing flags on attachment: 304333

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