Bug 169002

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

Created attachment 303033 [details] EWS trial run

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 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?

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

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

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

Created attachment 304174 [details] Patch

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

Created attachment 304333 [details] Patch for landing

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 on attachment 304333 [details] Patch for landing Clearing flags on attachment: 304333 Committed r213902: <http://trac.webkit.org/changeset/213902>