Bug 160334 – Crash with an Invalid Web Process IPC Message ID: WebPageProxy.AttributedStringForCharacterRangeCallback

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Ryosuke Niwa]

<rdar://problem/27078089>

      50 Safari: invalidMessageFunction(OpaqueWKString const*) <==
        50 WebKit: WebKit::WebProcessPool::didReceiveInvalidMessage(IPC::StringReference const&, IPC::StringReference const&)
          50 WebKit: WebKit::WebProcessProxy::didReceiveInvalidMessage(IPC::Connection&, IPC::StringReference, IPC::StringReference)
            50 WebKit: IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)
              50 WebKit: IPC::Connection::dispatchOneMessage()
                50 JavaScriptCore: WTF::RunLoop::performWork()
                  50 JavaScriptCore: WTF::RunLoop::performWork(void*)
                    50 CoreFoundation: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
                      50 CoreFoundation: __CFRunLoopDoSources0
                        50 CoreFoundation: __CFRunLoopRun
                          50 CoreFoundation: CFRunLoopRunSpecific
                            50 HIToolbox: RunCurrentEventLoopInMode
                              50 HIToolbox: ReceiveNextEventCommon
                                50 HIToolbox: _BlockUntilNextEventMatchingListInModeWithFilter
                                  50 AppKit: _DPSNextEvent
                                    50 AppKit: -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
                                      50 Safari: -[BrowserApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
                                        50 AppKit: -[NSApplication run]
                                          50 AppKit: NSApplicationMain
                                            50 libdyld.dylib: start

[Ryosuke Niwa]

7/28/16, 7:25 PM Ryosuke Niwa:
Alexey and I looked into this but we couldn’t figure out where the message is marked as invalid.

We initially thought MESSAGE_CHECK in WebPageProxy::attributedStringForCharacterRangeCallback was failing:
void WebPageProxy::attributedStringForCharacterRangeCallback(const AttributedString& string, const EditingRange& actualRange, uint64_t callbackID)
{
    MESSAGE_CHECK(actualRange.isValid());

EditingRange::isValid checks location + length >= location.  This can fail if location + length overflows or length is negative.  length can’t be negative since it’s uint64_t, and location is guaranteeed to be less than INT_MAX inside rangeFromEditingRange, which returns nullptr if location > INT_MAX.  It’s also quite inconceivable that location + length doesn’t fit in 2^31 since what we’re counting here is the visible number of characters.

Another place where the message can be marked as invalid would be decoding.  But I couldn’t spot any obvious flaws in ArgumentCodersMac.mm that decodes NSAttributedString by either code inspection or reconvering text in Japanese input method.

I did find one case in which we hit the following assertions in WebPage::attributedSubstringForCharacterRangeAsync:

ASSERT([attributedString length] == editingRange.length + 1);
ASSERT([[attributedString string] characterAtIndex:editingRange.length] == '\n' || [[attributedString string] characterAtIndex:editingRange.length] == ' ');
result.string = [attributedString attributedSubstringFromRange:NSMakeRange(0, editingRange.length)];

when the attributed string contains an image but this doesn’t result in a crash because of the truncation that happens in the third line before sending the message.

7/28/16, 7:32 PM Ryosuke Niwa:
Overall, there isn’t an obvious bug fix we can do here.

Now, for MESSAGE_CHECK, then we could speculatively adding ASSERT_RELEASE(editingRange.isValid()) in WebContent process side so that it kills WebContent process instead of Safari, which is probably a better UX, and we can dig into the range issue more once we determine this new assertion fails after GM.  Alternatively, we could remove the asssertion in UIProcess side and “fix up” the range but this has a downside that it may simply cover up a bug elsewhere.  This is probably the least risky path forward.

Either of those fixes don’t address the possibility that decoding is failing.  However, given the lack of a reproducible test case and an insight as to where the crash is happening, it’s hard to tell where problem lies in decoding.  One thing we can do is to add more release assertions in the decoding code of NSAttributedString so that we can pinpoint which decoding phase is failing but this will also result in a crash.  We would at least get a crash log with more useful information, however.

7/28/16, 10:26 PM Ryosuke Niwa:
This appears to be top #3 crasher in Gala: https://crashtracer.apple.com/app/show/Safari?train=SUGalaFiesta&users=external

I’ve looked through the logs and most of crash logs contain libSimplifiedChineseConverter.dylib which means this crash is encoutered by mainland Chinese users.

7/28/16, 10:28 PM Ryosuke Niwa:
It looks like a failure in decode won’t cause a crash as handleMessage simply bails out:

template<typename T, typename C, typename MF>
void handleMessage(MessageDecoder& decoder, C* object, MF function)
{
    typename T::DecodeType arguments;
    if (!decoder.decode(arguments)) {
        ASSERT(decoder.isInvalid());
        return;
    }

    callMemberFunction(WTFMove(arguments), object, function);
}

* thread #1: tid = 0x460929, 0x0000000107134a33 WebKit`void IPC::handleMessage<Messages::WebPageProxy::AttributedStringForCharacterRangeCallback, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebKit::AttributedString const&, WebKit::EditingRange const&, unsigned long long)>(decoder=0x0000000118974000, object=0x00007fff2e000a18, function=0x0000000107117f70)(WebKit::AttributedString const&, WebKit::EditingRange const&, unsigned long long)) + 211 at HandleMessage.h:87, queue = 'com.apple.main-thread', stop reason = step over
  * frame #0: 0x0000000107134a33 WebKit`void IPC::handleMessage<Messages::WebPageProxy::AttributedStringForCharacterRangeCallback, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebKit::AttributedString const&, WebKit::EditingRange const&, unsigned long long)>(decoder=0x0000000118974000, object=0x00007fff2e000a18, function=0x0000000107117f70)(WebKit::AttributedString const&, WebKit::EditingRange const&, unsigned long long)) + 211 at HandleMessage.h:87
    frame #1: 0x000000010712cfa4 WebKit`WebKit::WebPageProxy::didReceiveMessage(this=0x00007fff2e000a18, connection=0x000000011898f1a0, decoder=0x0000000118974000) + 10820 at WebPageProxyMessageReceiver.cpp:608
    frame #2: 0x0000000106ac8d58 WebKit`IPC::MessageReceiverMap::dispatchMessage(this=0x00000001157e7838, connection=0x000000011898f1a0, decoder=0x0000000118974000) + 456 at MessageReceiverMap.cpp:123
    frame #3: 0x00000001069adca4 WebKit`WebKit::ChildProcessProxy::dispatchMessage(this=0x00000001157e7800, connection=0x000000011898f1a0, decoder=0x0000000118974000) + 52 at ChildProcessProxy.cpp:162
    frame #4: 0x000000010722bafa WebKit`WebKit::WebProcessProxy::didReceiveMessage(this=0x00000001157e7800, connection=0x000000011898f1a0, decoder=0x0000000118974000) + 58 at WebProcessProxy.cpp:497
    frame #5: 0x00000001069be003 WebKit`IPC::Connection::dispatchMessage(this=0x000000011898f1a0, decoder=0x0000000118974000) + 51 at Connection.cpp:917
    frame #6: 0x00000001069b3450 WebKit`IPC::Connection::dispatchMessage(this=0x000000011898f1a0, message=unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> > at 0x00007fff5c805d00) + 784 at Connection.cpp:950
    frame #7: 0x00000001069be5f3 WebKit`IPC::Connection::dispatchOneMessage(this=0x000000011898f1a0) + 1507 at Connection.cpp:985
    frame #8: 0x00000001069d355d WebKit`IPC::Connection::enqueueIncomingMessage(this=0x00000001157e5158)::$_10::operator()() + 29 at Connection.cpp:911
    frame #9: 0x00000001069d34b9 WebKit`WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(this=0x00000001157e5150)::$_10>::call() + 25 at Function.h:101

[Ryosuke Niwa]

Created attachment 284849 [details]
Adds a speculative fix

[Ryosuke Niwa]

Committed r203909: <http://trac.webkit.org/changeset/203909>