editorState() should not cause a synchronous layout on iOS. Radar: <rdar://problem/20041506>
Created attachment 248335 [details] WIP Patch
Created attachment 248340 [details] WIP patch
Created attachment 248342 [details] Patch
Comment on attachment 248342 [details] Patch The patch looks good to me. I would add ASSERTS in WKContentViewInteraction.mm in all the places where we try to use components of editor state that require layout when we only have the partial editor state.
Created attachment 248373 [details] Patch
(In reply to comment #4) > Comment on attachment 248342 [details] > Patch > > The patch looks good to me. I would add ASSERTS in > WKContentViewInteraction.mm in all the places where we try to use components > of editor state that require layout when we only have the partial editor > state. Good idea. I made the change but I still need to verify on device that the assertion is not hit before we can land.
Attachment 248373 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/EditorState.h:105: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 248394 [details] Patch
Attachment 248394 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/EditorState.h:105: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
I am unfortunately hitting the new assertion, so this will need a bit more work. When WebPageProxy::didCommitLayerTree() is called, we end up calling [WKContentView(WKInteraction) _updateChangedSelection:]. In there, we construct a WKSelectionDrawingInfo object from the EditorState which accesses the PostLayoutData (caretRectAtEnd and selectionRects). Apparently, this is called before the WebProcess has had time to send the full EditorState (with PostLayoutData) to the UIProcess.
(In reply to comment #10) > I am unfortunately hitting the new assertion, so this will need a bit more > work. > > When WebPageProxy::didCommitLayerTree() is called, we end up calling > [WKContentView(WKInteraction) _updateChangedSelection:]. In there, we > construct a WKSelectionDrawingInfo object from the EditorState which > accesses the PostLayoutData (caretRectAtEnd and selectionRects). Apparently, > this is called before the WebProcess has had time to send the full > EditorState (with PostLayoutData) to the UIProcess. This is not the only case where we try to access the EditorState's PostLayoutData before layout is done: 2 WebKit 0x000000010323417c -[WKContentView(WKInteraction) selectedTextRange] + 460 3 UIKit 0x0000000187050b0c -[UITextSelection selectionChanged] + 36 4 UIKit 0x0000000187115d10 -[UITextSelectionView activate] + 204 5 WebKit 0x000000010322eb34 -[WKContentView(WKInteraction) becomeFirstResponder] + 84
Created attachment 250404 [details] Patch
Attachment 250404 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/EditorState.h:105: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 250404 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250404&action=review I highlighted the few changes compared to the previous iteration to facilitate review. > Source/WebKit2/Shared/EditorState.h:100 > + ASSERT_WITH_MESSAGE(!isMissingPostLayoutData, "Attempt to access post layout data before receiving it"); Added assertion as suggested to make sure we don't try to access the postLayoutData of a partial editor state. > Source/WebKit2/Shared/EditorState.h:106 > + ASSERT_WITH_MESSAGE(!isMissingPostLayoutData, "Attempt to access post layout data before receiving it"); Added assertion as suggested to make sure we don't try to access the postLayoutData of a partial editor state. > Source/WebKit2/UIProcess/ios/WKContentView.mm:496 > + if (!_page->editorState().isMissingPostLayoutData) I added this check otherwise we would sometimes try to access the postLayoutData even though we have a partial EditorState, thus hitting the new assertions above. This can happen when a new layout a scheduled and the selection is changed after the previous layout is done but *before* didCommitLayerTree() is called. We flush the layers and send the didCommitLayerTree IPC in a background thread so nothing prevents a new layout to be scheduled and thus a partial EditorState to be sent before the didCommitLayerTree message is received. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4379 > + auto editorState = this->editorState(needsLayout ? IncludePostLayoutDataHint::No : IncludePostLayoutDataHint::Yes); We now only send a partial editor state if there is a layout pending as this is the only case where calling editorState() would trigger a synchronous layout. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4380 > + ASSERT_WITH_MESSAGE(needsLayout == (view && view->needsLayout()), "Calling editorState() should not cause a synchronous layout."); I added this assertion to make sure that calling editorState() does not cause a synchronous layout. This could happen if someone added a new field to the EditorState which requires a layout to be computed and forget to add it to the PostLayoutData part.
Comment on attachment 250404 [details] Patch Looks good to me.
Comment on attachment 250404 [details] Patch Clearing flags on attachment: 250404 Committed r182603: <http://trac.webkit.org/changeset/182603>
All reviewed patches have been landed. Closing bug.
This broke a test on multiple bots: <https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=editing%2Fselection%2Fselection-invalid-offset.html>
(In reply to comment #18) > This broke a test on multiple bots: > <https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=editing%2Fselection%2Fselection-invalid-offset. > html> Fix is pending review in https://bugs.webkit.org/show_bug.cgi?id=143591. I missed a null-check on the EditorClient.
(In reply to comment #16) > Comment on attachment 250404 [details] > Patch > > Clearing flags on attachment: 250404 > > Committed r182603: <http://trac.webkit.org/changeset/182603> This caused more than 500 crashes on the GTK Debug build. Reported here https://bugs.webkit.org/show_bug.cgi?id=143715