Bug 199960 – [iOS] ASSERTION FAILURE: !isMissingPostLayoutData in WebKit::EditorState::postLayoutData()

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Steps to reproduce:

(Use the software keyboard)

1. Visit <data:text/html,<textarea%20placeholder="Type%20w%20then%20press%20backspace"%20onkeydown="document.getElementById('output').value%20=%20document.getElementById('output').value%20+%20'\n'"></textarea><textarea%20id="output"%20readonly></textarea>>.

2. Focus the first textarea and press w then press the backspace.

Then the WebContent process will crash because the ASSERT_WITH_MESSAGE(!isMissingPostLayoutData, "Attempt to access post layout data before receiving it") fails in WebKit::EditorState::postLayoutData().

[Radar WebKit Bug Importer]

<rdar://problem/53323966>

[Daniel Bates]

Backtrace has the form:

ASSERTION FAILED: Attempt to access post layout data before receiving it
!isMissingPostLayoutData
/Volumes/.../OpenSource/Source/WebKit/Shared/EditorState.h(150) : const WebKit::EditorState::PostLayoutData &WebKit::EditorState::postLayoutData() const
1   0x111c5e1c9 WTFCrash
2   0x11684e6db WebKit::EditorState::postLayoutData() const
3   0x11773b7f9 -[WKContentView(WKInteraction) _characterBeforeCaretSelection]
...
17  0x11774033d WTF::BlockPtr<void (WebEvent*, bool)>::operator()(WebEvent*, bool) const
18  0x117740195 -[WKContentView(WKInteraction) _didHandleKeyEvent:eventWasHandled:]
19  0x116d28319 WebKit::PageClientImpl::doneWithKeyEvent(WebKit::NativeWebKeyboardEvent const&, bool)
20  0x116b689f9 WebKit::WebPageProxy::didReceiveEvent(unsigned int, bool)
21  0x11750f848 void IPC::callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned int, bool), std::__1::tuple<unsigned int, bool>, 0ul, 1ul>(WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned int, bool), std::__1::tuple<unsigned int, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>)
22  0x11750f780 void IPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned int, bool), std::__1::tuple<unsigned int, bool>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<unsigned int, bool>&&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned int, bool))
23  0x1174f4e2e void IPC::handleMessage<Messages::WebPageProxy::DidReceiveEvent, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned int, bool)>(IPC::Decoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned int, bool))
24  0x1174ecfdc WebKit::WebPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
25  0x1162a26f9 IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&)
26  0x1169e7f64 WebKit::AuxiliaryProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&)
27  0x116c4a5fa WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
28  0x116234e19 IPC::Connection::dispatchMessage(IPC::Decoder&)
29  0x11622d969 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
30  0x116233cab IPC::Connection::dispatchIncomingMessages()
31  0x116254425 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_11::operator()()

[Daniel Bates]

^^^ the ... is UIKit

[Daniel Bates]

This assertion failure only happens in Modern WebKit (read not Legacy WebKit). The reason is that Modern WebKit tries to avoid performing a layout as part of gathering editor state as a performance optimization. Legacy has no such optimization and performs a layout after handling every keydown and keyup.

[Daniel Bates]

Also does not happen on Mac because WebCore performs text insertion/deletion and updates layout in same even loop iteration as keydown.

[Daniel Bates]

Doesn't occur with a hardware keyboard attached on iOS because we force layout in WebPage::platformEditorState():<https://trac.webkit.org/browser/trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm?rev=247573#L224>

[Daniel Bates]

^^^^ though this is the editor state computed when we call to UI process to interpret the key event. But this info is out-of-date since layout can change as a result of JavaScript and editor state the UI process does not have the most up-to-date details by the time the Web process tells it that the key event has been processed.

[Daniel Bates]

We might be able to get away with ^^^^. So, not having the most up-to-date info. I coded up a fix (forthcoming) so that UI process has the most up-to-date info.

[Daniel Bates]

Created attachment 374498 [details]
Patch and layout test

[Simon Fraser (smfr)]

Comment on attachment 374498 [details]
Patch and layout test

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

> Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm:44
> +    page.corePage()->focusController().focusedOrMainFrame().document()->updateLayout();

Be aware that this can run arbitrary script and destroy frames, nodes etc.

[Daniel Bates]

Comment on attachment 374498 [details]
Patch and layout test

Let me think about this some more. Don't want to have this explode with security bugs. Maybe go with the stale data....

[Daniel Bates]

Created attachment 374508 [details]
Patch and layout test

[Daniel Bates]

Going to re-encode the fix in terms of an new editorState() enum.

[Daniel Bates]

Created attachment 374635 [details]
Patch and layout test - encoded in enum

[Daniel Bates]

The patch also removes m_isEditorStateMissingPostLayoutData in WebPage.h. This ivar has been unused since r221064 (bug #175370).

Also we haven't been using IncludePostLayoutDataHint::No since the last reference to it was removed in r244494 (bug #197145).

[Daniel Bates]

Created attachment 374637 [details]
Patch and layout test

[Daniel Bates]

Created attachment 374638 [details]
Patch and layout test

[Daniel Bates]

Forgot the expected results. Could probably use auto more at least on lines I touched. Could size the enum class to char, but since not a store type there's little value. I guess more efficient register passing and stack pack if register spill, but we're not even close to having enough args to matter.

[Daniel Bates]

Comment on attachment 374638 [details]
Patch and layout test

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1015
> +    Ref<Frame> frame = m_page->focusController().focusedOrMainFrame();

FYI scope of ref here is generous. Could move to above updateLayout(). I thought to be generous 😀

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1017
> +    if (shouldPerformLayout == ShouldPerformLayout::Default)

The ::No is only used internal to this function. There are no outside callers. Could just remove this enumerator and internally store a Boolean. Enum class could then be sized to bool (doesn't matter though), but gives less options for people to shoot themselves in the 🦶 (that's what matters). Opinions?

[Daniel Bates]

Created attachment 374973 [details]
Patch and layout test

Removed enumerator ShouldPerformLayout::No; only ::Default and ::Yes now.

[Daniel Bates]

Created attachment 377056 [details]
Patch and layout test

[Daniel Bates]

Created attachment 393899 [details]
Patch

[Daniel Bates]

Created attachment 393900 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 393900 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:316
> +void WebPage::platformEditorStateCommon(const Frame& frame, EditorState& result) const

This should be getEditorStateCommon or computeEditorStateCommon.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1067
> +    auto& document = *frame->document();

RefPtr<Document>?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:237
> +void WebPage::platformEditorState(Frame& frame, EditorState& result) const

get/computeEditorState.

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:136
> +void WebPage::platformEditorState(Frame& frame, EditorState& result) const

get/compute

[Daniel Bates]

Comment on attachment 393900 [details]
Patch

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

Thanks for the review.

>> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:316
>> +void WebPage::platformEditorStateCommon(const Frame& frame, EditorState& result) const
> 
> This should be getEditorStateCommon or computeEditorStateCommon.

Will use get.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1067
>> +    auto& document = *frame->document();
> 
> RefPtr<Document>?

Will fix.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:237
>> +void WebPage::platformEditorState(Frame& frame, EditorState& result) const
> 
> get/computeEditorState.

Will use get.

>> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:136
>> +void WebPage::platformEditorState(Frame& frame, EditorState& result) const
> 
> get/compute

Will use get.

[Daniel Bates]

Created attachment 394432 [details]
To Land

[Daniel Bates]

Created attachment 394464 [details]
To Land

[Daniel Bates]

Created attachment 394472 [details]
To Land

Fix up Playstation code. I still do not know what is the problem with the GTK API tests. Can someone help me?

[Daniel Bates]

GTK API failure is buried in the output:

[[
ERROR:../../Tools/TestWebKitAPI/Tests/WebKitGtk/TestWebViewEditor.cpp:341:void testWebViewEditorEditorStateTypingAttributes(EditorTest*, gconstpointer): assertion failed (typingAttributes == WEBKIT_EDITOR_TYPING_ATTRIBUTE_NONE): (0 == 2)
]]

[Daniel Bates]

(In reply to Daniel Bates from comment #29)
> GTK API failure is buried in the output:
> 
> [[
> ERROR:../../Tools/TestWebKitAPI/Tests/WebKitGtk/TestWebViewEditor.cpp:341:
> void testWebViewEditorEditorStateTypingAttributes(EditorTest*,
> gconstpointer): assertion failed (typingAttributes ==
> WEBKIT_EDITOR_TYPING_ATTRIBUTE_NONE): (0 == 2)
> ]]

To fix this I am leaning to patch webkitEditorStateCreate() to initialize editorState->priv->typingAttributes to WEBKIT_EDITOR_TYPING_ATTRIBUTE_NONE.

[Daniel Bates]

Created attachment 394474 [details]
Attempt to fix GTK

I am too tired to think this through right now, just posting a patch based on my previous comment to see what EWS thinks (since it takes some time for EWS to process a patch). Patch may not be correct.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Adrian Perez]

Comment on attachment 394474 [details]
Attempt to fix GTK

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

I believe that the change to glib/WebKitEditorState.cpp are correct,
but would not mind having Carlos García's rubber stamp on that ~_~

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:317
> +

No need to add this empty line here :)

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #33)
> Comment on attachment 394474 [details]
> Attempt to fix GTK
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394474&action=review
> 
> I believe that the change to glib/WebKitEditorState.cpp are correct,
> but would not mind having Carlos García's rubber stamp on that ~_~

LGTM

> > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:317
> > +
> 
> No need to add this empty line here :)

[Daniel Bates]

Thanks Adrian and Carlos!

[Daniel Bates]

Committed r258989: <https://trac.webkit.org/changeset/258989>

[Daniel Bates]

(In reply to Adrian Perez from comment #33)
> Comment on attachment 394474 [details]
> Attempt to fix GTK
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394474&action=review
> 
> I believe that the change to glib/WebKitEditorState.cpp are correct,
> but would not mind having Carlos García's rubber stamp on that ~_~
> 
> > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:317
> > +
> 
> No need to add this empty line here :)

Oops, I forgot to remove it. When I have a moment, I'll look to do so.

[Daniel Bates]

(In reply to Daniel Bates from comment #37)
> (In reply to Adrian Perez from comment #33)
> > Comment on attachment 394474 [details]
> > Attempt to fix GTK
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394474&action=review
> > 
> > I believe that the change to glib/WebKitEditorState.cpp are correct,
> > but would not mind having Carlos García's rubber stamp on that ~_~
> > 
> > > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:317
> > > +
> > 
> > No need to add this empty line here :)
> 
> Oops, I forgot to remove it. When I have a moment, I'll look to do so.

Removed in <https://trac.webkit.org/changeset/258995>

[Darin Adler]

Comment on attachment 394474 [details]
Attempt to fix GTK

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:-261
> -    if ((shouldIncludePostLayoutData == IncludePostLayoutDataHint::No || needsLayout) && !requiresPostLayoutData) {
> -        result.isMissingPostLayoutData = true;
> +    if (needsLayout || result.isMissingPostLayoutData)
>          return;
> -    }

This change broke the Mac Catalyst build.

After this refactoring, the requiresPostLayoutData boolean local variable above is unused. In the Mac Catalyst configuration the code is simple enough that the compiler emits a warning about the unused boolean; in other configurations it doesn’t warn. I believe the fix is to remove the 4 lines of code above that define and set the value of the local variable.

[Daniel Bates]

Comment on attachment 394474 [details]
Attempt to fix GTK

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:-261
>> -    }
> 
> This change broke the Mac Catalyst build.
> 
> After this refactoring, the requiresPostLayoutData boolean local variable above is unused. In the Mac Catalyst configuration the code is simple enough that the compiler emits a warning about the unused boolean; in other configurations it doesn’t warn. I believe the fix is to remove the 4 lines of code above that define and set the value of the local variable.

Oops! Bad merge...  Here's the fix, I got to run, but I'll try to post later:

[[
diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
index 47c620d0478..fe664b6e41b 100644
--- a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
+++ b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
@@ -260,14 +260,8 @@ void WebPage::getPlatformEditorState(Frame& frame, EditorState& result) const
     }
 
     // We only set the remaining EditorState entries if layout is done as a performance optimization
-    // to avoid the need to force a synchronous layout here to compute these entries. If we
-    // have a composition or are using a hardware keyboard then we send the full editor state
-    // immediately so that the UIProcess can update UI, including the position of the caret.
+    // to avoid the need to force a synchronous layout here to compute these entries.
     bool needsLayout = view->needsLayout();
-    bool requiresPostLayoutData = frame.editor().hasComposition();
-#if !PLATFORM(MACCATALYST)
-    requiresPostLayoutData |= m_keyboardIsAttached;
-#endif
     if (needsLayout || result.isMissingPostLayoutData)
         return;
]]

Why is this the fix? Because I moved the old logic into WebPage::platformNeedsLayoutForEditorState(), which is called before this function.

[Daniel Bates]

Comment on attachment 394474 [details]
Attempt to fix GTK

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

>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:-261
>>> -    }
>> 
>> This change broke the Mac Catalyst build.
>> 
>> After this refactoring, the requiresPostLayoutData boolean local variable above is unused. In the Mac Catalyst configuration the code is simple enough that the compiler emits a warning about the unused boolean; in other configurations it doesn’t warn. I believe the fix is to remove the 4 lines of code above that define and set the value of the local variable.
> 
> Oops! Bad merge...  Here's the fix, I got to run, but I'll try to post later:
> 
> [[
> diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
> index 47c620d0478..fe664b6e41b 100644
> --- a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
> +++ b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
> @@ -260,14 +260,8 @@ void WebPage::getPlatformEditorState(Frame& frame, EditorState& result) const
>      }
>  
>      // We only set the remaining EditorState entries if layout is done as a performance optimization
> -    // to avoid the need to force a synchronous layout here to compute these entries. If we
> -    // have a composition or are using a hardware keyboard then we send the full editor state
> -    // immediately so that the UIProcess can update UI, including the position of the caret.
> +    // to avoid the need to force a synchronous layout here to compute these entries.
>      bool needsLayout = view->needsLayout();
> -    bool requiresPostLayoutData = frame.editor().hasComposition();
> -#if !PLATFORM(MACCATALYST)
> -    requiresPostLayoutData |= m_keyboardIsAttached;
> -#endif
>      if (needsLayout || result.isMissingPostLayoutData)
>          return;
> ]]
> 
> Why is this the fix? Because I moved the old logic into WebPage::platformNeedsLayoutForEditorState(), which is called before this function.

Got a spare minute and committed the fix in <https://trac.webkit.org/changeset/259183>.