Bug 199503 – [iOS] Crash in WebKit::WebPage::positionInformation via Range::startPosition

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

0   WebKit                        	0x00000001bd8c5304 WebKit::WebPage::positionInformation(WebKit::InteractionInformationRequest const&) + 368 (Optional.h:371)
1   WebKit                        	0x00000001bd8c5300 WebKit::WebPage::positionInformation(WebKit::InteractionInformationRequest const&) + 364 (WebPageIOS.mm:2517)
2   WebKit                        	0x00000001bd8c510c WebKit::WebPage::getPositionInformation(WebKit::InteractionInformationRequest const&, WTF::CompletionHandler<void (WebKit::InteractionInformationAtPosition&&)>&&) + 76 (WebPageIOS.mm:2492)
3   WebKit                        	0x00000001bd9fca78 void IPC::callMemberFunctionImpl<WebKit::WebPage, void (WebKit::WebPage::*)(WebKit::InteractionInformationRequest const&, WTF::CompletionHandler<void (WebKit::InteractionInformationAtPosition&&)>&&), void (WebKit::InteractionInformationAtPosition const&), std::__1::tuple<WebKit::InteractionInformationRequest>, 0ul>(WebKit::WebPage*, void (WebKit::WebPage::*)(WebKit::InteractionInformationRequest const&, WTF::CompletionHandler<void (WebKit::InteractionInformationAtPosition&&)>&&), WTF::CompletionHandler<void (WebKit::InteractionInformationAtPosition const&)>&&, std::__1::tuple<WebKit::InteractionInformationRequest>&&, std::__1::integer_sequence<unsigned long, 0ul>) + 100 (HandleMessage.h:55)
4   WebKit                        	0x00000001bd9e8654 void IPC::handleMessageSynchronous<Messages::WebPage::GetPositionInformation, WebKit::WebPage, void (WebKit::WebPage::*)(WebKit::InteractionInformationRequest const&, WTF::CompletionHandler<void (WebKit::InteractionInformationAtPosition&&)>&&)>(IPC::Connection&, IPC::Decoder&, std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebKit::InteractionInformationRequest const&, WTF::CompletionHandler<void (WebKit::InteractionInformationAtPosition&&)>&&)) + 164 (HandleMessage.h:61)
5   WebKit                        	0x00000001bd5b9924 IPC::MessageReceiverMap::dispatchSyncMessage(IPC::Connection&, IPC::Decoder&, std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >&) + 112 (MessageReceiverMap.cpp:0)
6   WebKit                        	0x00000001bd904224 WebKit::WebProcess::didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >&) + 40 (WebProcess.cpp:732)
7   WebKit                        	0x00000001bd5a73f0 IPC::Connection::dispatchSyncMessage(IPC::Decoder&) + 196 (Connection.cpp:905)
8   WebKit                        	0x00000001bd5a4128 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 136 (Connection.cpp:1008)
9   WebKit                        	0x00000001bd5a7944 IPC::Connection::dispatchOneIncomingMessage() + 232 (Connection.cpp:1079)
10  JavaScriptCore                	0x00000001c8227534 WTF::RunLoop::performWork() + 276 (Function.h:79)
11  JavaScriptCore                	0x00000001c82277f4 WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38)

<rdar://problem/51597945>

[Ryosuke Niwa]

We’re crashing in startPosition of the composition range:

2517	            RefPtr<Range> compositionRange = frame.editor().compositionRange();
2518	            if (position < compositionRange->startPosition())
2519	                position = compositionRange->startPosition();
2520	            else if (position > compositionRange->endPosition())
2521	                position = compositionRange->endPosition();

Range’s start Position:
const Position startPosition() const { return m_start.toPosition(); }

inline const Position RangeBoundaryPoint::toPosition() const
{
    ensureOffsetIsValid();
    return createLegacyEditingPosition(m_containerNode.get(), m_offsetInContainer.value());
}

inline void RangeBoundaryPoint::ensureOffsetIsValid() const
{
    if (m_offsetInContainer)
        return;

    ASSERT(m_childBeforeBoundary);
    m_offsetInContainer = m_childBeforeBoundary->computeNodeIndex() + 1;
}

     0x00000001bd8c52f4:      ldr x0, [x26, #0xc8]
     0x00000001bd8c52f8:      sub x8, x29, #0xb8       ; =0xb8 
     0x00000001bd8c52fc:       bl 0x53037c             ; symbol stub for: WebCore::Editor::compositionRange() const
     0x00000001bd8c5300:     ldur x26, [x29, #-0xb8]
 ->  0x00000001bd8c5304:     ldrb w8, [x26, #0x18]
     0x00000001bd8c5308:      cbz w8, 0x34f314         ; <+384> [inlined] WTF::DumbPtrTraits<WebCore::Node>::unwrap(WebCore::Node* const&) at RefPtr.h:79
     0x00000001bd8c530c:      ldr w2, [x26, #0x1c]
     0x00000001bd8c5310:        b 0x34f334             ; <+416> [inlined] WTF::DumbPtrTraits<WebCore::Node>::unwrap(WebCore::Node* const&) at RefPtr.h:71
     0x00000001bd8c5314:      ldr x0, [x26, #0x20]

0x18 is the location of m_offsetInContainer in Range (24 = 3 pointers down from the address in x26). cbz w8 is std::nullopt check for Optional<unsigned> m_offsetInContainer in RangeBoundaryPoint. This would mean that what’s null is the range object itself.

compositionRange() is only called when frame.editor().hasComposition() is true in WebPage::positionInformation. However, Editor::compositionRange() could return nullptr when start >= end:
RefPtr<Range> Editor::compositionRange() const
{
    if (!m_compositionNode)
        return nullptr;
    unsigned length = m_compositionNode->length();
    unsigned start = std::min(m_compositionStart, length);
    unsigned end = std::min(std::max(start, m_compositionEnd), length);
    if (start >= end)
        return nullptr;
    return Range::create(m_compositionNode->document(), m_compositionNode.get(), start, m_compositionNode.get(), end);
}

[Ryosuke Niwa]

start >= end condition was introduced when Editor::compositionRange() was initially introduced 12 years ago: https://trac.webkit.org/changeset/25547/webkit

[Ryosuke Niwa]

I don't quite understand why compositionRange() needs to return nullptr when start == end. Given we're clamping end to be start, it seems natural that we may want to return a collapsed Range.

If this behavior is expected of compositionRange(), then alternatively, we should probably update

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #3)
> I don't quite understand why compositionRange() needs to return nullptr when
> start == end. Given we're clamping end to be start, it seems natural that we
> may want to return a collapsed Range.
> 
> If this behavior is expected of compositionRange(), then alternatively, we
> should probably update

focusedElementPositionInformation to exit early (like when frame.editor().hasComposition() returns false) or create a collapsed range at the start when compositionRange() when compositionRange() is nullptr.

[Ryosuke Niwa]

Created attachment 373474 [details]
Fixes the bug

[Wenson Hsieh]

Comment on attachment 373474 [details]
Fixes the bug

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

LGTM, with a build fix for macOS.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:551
> +- (void)_ensurePositionInformationAt:(CGPoint)location completion:(void (^)(void))updateBlock WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

This seems identical to -[WKWebView _requestActivatedElementAtPosition:completionBlock:], except it’s sync instead of async.

Do we need to make a sync position info request for this test, or can we have UIScriptController piggy-back off of the existing _requestActivatedElementAtPosition:completionBlock: SPI in WKWebViewPrivate.h?

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 373474 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373474&action=review
> 
> LGTM, with a build fix for macOS.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:551
> > +- (void)_ensurePositionInformationAt:(CGPoint)location completion:(void (^)(void))updateBlock WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This seems identical to -[WKWebView
> _requestActivatedElementAtPosition:completionBlock:], except it’s sync
> instead of async.
> 
> Do we need to make a sync position info request for this test, or can we
> have UIScriptController piggy-back off of the existing
> _requestActivatedElementAtPosition:completionBlock: SPI in
> WKWebViewPrivate.h?

Sure, we can do that. It does obtain the activated element information, and I thought it might be better to have an SPI that only updates the position information but it works for this test case so we can do that for now.

[Ryosuke Niwa]

Created attachment 373531 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 373531 [details]
Patch

Clearing flags on attachment: 373531

Committed r247180: <https://trac.webkit.org/changeset/247180>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.