Bug 199960 - [iOS] ASSERTION FAILURE: !isMissingPostLayoutData in WebKit::EditorState::postLayoutData()
Summary: [iOS] ASSERTION FAILURE: !isMissingPostLayoutData in WebKit::EditorState::pos...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Daniel Bates
URL: data:text/html,<textarea%20placeholde...
Keywords: InRadar, PlatformOnly
Depends on: 201608
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-19 14:28 PDT by Daniel Bates
Modified: 2019-09-09 11:08 PDT (History)
5 users (show)

See Also:


Attachments
Patch and layout test (8.24 KB, patch)
2019-07-19 14:54 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (8.39 KB, patch)
2019-07-19 15:29 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test - encoded in enum (27.44 KB, patch)
2019-07-22 14:29 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (27.80 KB, patch)
2019-07-22 14:35 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (27.79 KB, patch)
2019-07-22 14:49 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (28.39 KB, patch)
2019-07-26 10:54 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (28.41 KB, patch)
2019-08-22 14:58 PDT, Daniel Bates
dbates: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2019-07-19 14:28:42 PDT
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().
Comment 1 Radar WebKit Bug Importer 2019-07-19 14:28:57 PDT
<rdar://problem/53323966>
Comment 2 Daniel Bates 2019-07-19 14:30:07 PDT
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()()
Comment 3 Daniel Bates 2019-07-19 14:30:27 PDT
^^^ the ... is UIKit
Comment 4 Daniel Bates 2019-07-19 14:32:45 PDT
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.
Comment 5 Daniel Bates 2019-07-19 14:33:47 PDT
Also does not happen on Mac because WebCore performs text insertion/deletion and updates layout in same even loop iteration as keydown.
Comment 6 Daniel Bates 2019-07-19 14:35:50 PDT
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>
Comment 7 Daniel Bates 2019-07-19 14:39:18 PDT
^^^^ 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.
Comment 8 Daniel Bates 2019-07-19 14:44:38 PDT
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.
Comment 9 Daniel Bates 2019-07-19 14:54:51 PDT
Created attachment 374498 [details]
Patch and layout test
Comment 10 Simon Fraser (smfr) 2019-07-19 15:09:25 PDT
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 11 Daniel Bates 2019-07-19 15:11:30 PDT
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....
Comment 12 Daniel Bates 2019-07-19 15:29:56 PDT
Created attachment 374508 [details]
Patch and layout test
Comment 13 Daniel Bates 2019-07-19 16:27:36 PDT
Going to re-encode the fix in terms of an new editorState() enum.
Comment 14 Daniel Bates 2019-07-22 14:29:30 PDT
Created attachment 374635 [details]
Patch and layout test - encoded in enum
Comment 15 Daniel Bates 2019-07-22 14:32:56 PDT
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).
Comment 16 Daniel Bates 2019-07-22 14:35:11 PDT
Created attachment 374637 [details]
Patch and layout test
Comment 17 Daniel Bates 2019-07-22 14:49:05 PDT
Created attachment 374638 [details]
Patch and layout test
Comment 18 Daniel Bates 2019-07-22 19:18:57 PDT
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 19 Daniel Bates 2019-07-22 19:26:59 PDT
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?
Comment 20 Daniel Bates 2019-07-26 10:54:40 PDT
Created attachment 374973 [details]
Patch and layout test

Removed enumerator ShouldPerformLayout::No; only ::Default and ::Yes now.
Comment 21 Daniel Bates 2019-08-22 14:58:22 PDT
Created attachment 377056 [details]
Patch and layout test