Bug 142536 – [WK2][iOS] editorState() should not cause a synchronous layout

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Chris Dumez]

editorState() should not cause a synchronous layout on iOS.

Radar: <rdar://problem/20041506>

[Chris Dumez]

Created attachment 248335 [details]
WIP Patch

[Chris Dumez]

Created attachment 248340 [details]
WIP patch

[Chris Dumez]

Created attachment 248342 [details]
Patch

[Enrica Casucci]

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.

[Chris Dumez]

Created attachment 248373 [details]
Patch

[Chris Dumez]

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

[WebKit Commit Bot]

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.

[Chris Dumez]

Created attachment 248394 [details]
Patch

[WebKit Commit Bot]

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.

[Chris Dumez]

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.

[Chris Dumez]

(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

[Chris Dumez]

Created attachment 250404 [details]
Patch

[WebKit Commit Bot]

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.

[Chris Dumez]

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.

[Enrica Casucci]

Comment on attachment 250404 [details]
Patch

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 250404 [details]
Patch

Clearing flags on attachment: 250404

Committed r182603: <http://trac.webkit.org/changeset/182603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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>

[Chris Dumez]

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

[Carlos Alberto Lopez Perez]

(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