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().
<rdar://problem/53323966>
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()()
^^^ the ... is UIKit
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.
Also does not happen on Mac because WebCore performs text insertion/deletion and updates layout in same even loop iteration as keydown.
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>
^^^^ 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.
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.
Created attachment 374498 [details] Patch and layout test
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.
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....
Created attachment 374508 [details] Patch and layout test
Going to re-encode the fix in terms of an new editorState() enum.
Created attachment 374635 [details] Patch and layout test - encoded in enum
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).
Created attachment 374637 [details] Patch and layout test
Created attachment 374638 [details] Patch and layout test
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.
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?
Created attachment 374973 [details] Patch and layout test Removed enumerator ShouldPerformLayout::No; only ::Default and ::Yes now.
Created attachment 377056 [details] Patch and layout test
Created attachment 393899 [details] Patch
Created attachment 393900 [details] Patch
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
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.
Created attachment 394432 [details] To Land
Created attachment 394464 [details] To Land
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?
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) ]]
(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.
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.
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
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 :)
(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 :)
Thanks Adrian and Carlos!
Committed r258989: <https://trac.webkit.org/changeset/258989>
(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.
(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>
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.
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.
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>.