160334 2016-07-28 23:51:32 -0700 Crash with an Invalid Web Process IPC Message ID: WebPageProxy.AttributedStringForCharacterRangeCallback 2016-07-29 11:46:30 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa rniwa andersca ap sam oldest_to_newest 1215505 0 rniwa 2016-07-28 23:51:32 -0700 <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 1215506 1 rniwa 2016-07-28 23:52:00 -0700 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 1215507 2 284849 rniwa 2016-07-28 23:57:56 -0700 Created attachment 284849 Adds a speculative fix 1215628 3 rniwa 2016-07-29 11:46:30 -0700 Committed r203909: <http://trac.webkit.org/changeset/203909> 284849 2016-07-28 23:57:56 -0700 2016-07-29 06:31:56 -0700 Adds a speculative fix bug-160334-20160728235642.patch text/plain 2544 rniwa SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwMzg1OCkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBACisyMDE2LTA3LTI4ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIENyYXNoIHdpdGggYW4gSW52YWxp ZCBXZWIgUHJvY2VzcyBJUEMgTWVzc2FnZSBJRDogV2ViUGFnZVByb3h5LkF0dHJpYnV0ZWRTdHJp bmdGb3JDaGFyYWN0ZXJSYW5nZUNhbGxiYWNrCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xNjAzMzQKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzI3MDc4 MDg5PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRo ZSBjcmFzaCBpcyBtb3N0IGxpa2VseSBjYXVzZWQgYnkgYW4gTUVTU0FHRV9DSEVDSyBmYWlsdXJl IGluIFdlYlBhZ2VQcm94eTo6YXR0cmlidXRlZFN0cmluZ0ZvckNoYXJhY3RlclJhbmdlQ2FsbGJh Y2sKKyAgICAgICAgd2hpY2ggbWFya3MgdGhlIGN1cnJlbnRseSBkaXNwYXRjaGluZyBtZXNzYWdl IHdhcyBpbnZhbGlkIGluc2lkZSB0aGUgbWFjcm8uCisKKyAgICAgICAgTWFrZSBzdXJlIHdlIG5l dmVyIGZhaWwgdGhpcyBjaGVjayBieSBzZW5kaW5nIGFuIGVtcHR5IEVkaXRpbmdSYW5nZSBpbiBh dHRyaWJ1dGVkU3Vic3RyaW5nRm9yQ2hhcmFjdGVyUmFuZ2VBc3luYyB3aGVuCisgICAgICAgIHRo ZSBlZGl0aW5nIHJhbmdlIHdlJ3JlIGFib3V0IHRvIHNlbmQgdG8gdGhlIFVJUHJvY2VzcyBpcyBp bnZhbGlkIGluIFdlYlByb2Nlc3MuCisKKyAgICAgICAgVW5mb3J0dW5hdGVseSwgbm8gbmV3IHRl c3RzIHNpbmNlIHdlIGRvbid0IGhhdmUgYW55IHJlcHJvZHVjdGlvbiBhbmQgSSBjb3VsZG4ndCBz cG90IGFueSBjb2RlIHBhdGggaW4gd2hpY2ggd2UgZW5kIHVwCisgICAgICAgIHdpdGggYW4gaW52 YWxpZCBFZGl0aW5nUmFnZSBoZXJlIHdpdGggbXVsdGlwbGUgaW5zcGVjdGlvbiBvZiB0aGUgcmVs ZXZhbnQgY29kZS4KKworICAgICAgICAqIFdlYlByb2Nlc3MvV2ViUGFnZS9tYWMvV2ViUGFnZU1h Yy5tbToKKyAgICAgICAgKFdlYktpdDo6V2ViUGFnZTo6YXR0cmlidXRlZFN1YnN0cmluZ0ZvckNo YXJhY3RlclJhbmdlQXN5bmMpOgorCiAyMDE2LTA3LTI4ICBDYXJsb3MgR2FyY2lhIENhbXBvcyAg PGNnYXJjaWFAaWdhbGlhLmNvbT4KIAogICAgICAgICBTcGxpdCBjYWxjdWxhdGVDYWNoZVNpemVz IGluIHR3byBtZXRob2RzCkluZGV4OiBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2Uv bWFjL1dlYlBhZ2VNYWMubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdDIvV2ViUHJvY2Vz cy9XZWJQYWdlL21hYy9XZWJQYWdlTWFjLm1tCShyZXZpc2lvbiAyMDM4NTgpCisrKyBTb3VyY2Uv V2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvbWFjL1dlYlBhZ2VNYWMubW0JKHdvcmtpbmcgY29w eSkKQEAgLTM1Miw3ICszNTIsMTUgQEAgdm9pZCBXZWJQYWdlOjphdHRyaWJ1dGVkU3Vic3RyaW5n Rm9yQ2hhcgogICAgICAgICByZXN1bHQuc3RyaW5nID0gW2F0dHJpYnV0ZWRTdHJpbmcgYXR0cmli dXRlZFN1YnN0cmluZ0Zyb21SYW5nZTpOU01ha2VSYW5nZSgwLCBlZGl0aW5nUmFuZ2UubGVuZ3Ro KV07CiAgICAgfQogCi0gICAgc2VuZChNZXNzYWdlczo6V2ViUGFnZVByb3h5OjpBdHRyaWJ1dGVk U3RyaW5nRm9yQ2hhcmFjdGVyUmFuZ2VDYWxsYmFjayhyZXN1bHQsIEVkaXRpbmdSYW5nZShlZGl0 aW5nUmFuZ2UubG9jYXRpb24sIFtyZXN1bHQuc3RyaW5nIGxlbmd0aF0pLCBjYWxsYmFja0lEKSk7 CisgICAgRWRpdGluZ1JhbmdlIHJhbmdlVG9TZW5kKGVkaXRpbmdSYW5nZS5sb2NhdGlvbiwgW3Jl c3VsdC5zdHJpbmcgbGVuZ3RoXSk7CisgICAgQVNTRVJUKHJhbmdlVG9TZW5kLmlzVmFsaWQoKSk7 CisgICAgaWYgKCFyYW5nZVRvU2VuZC5pc1ZhbGlkKCkpIHsKKyAgICAgICAgLy8gU2VuZCBhbiBl bXB0eSBFZGl0aW5nUmFuZ2UgYXMgYSBsYXN0IHJlc29ydCBmb3IgPHJkYXI6Ly9wcm9ibGVtLzI3 MDc4MDg5Pi4KKyAgICAgICAgc2VuZChNZXNzYWdlczo6V2ViUGFnZVByb3h5OjpBdHRyaWJ1dGVk U3RyaW5nRm9yQ2hhcmFjdGVyUmFuZ2VDYWxsYmFjayhyZXN1bHQsIEVkaXRpbmdSYW5nZSgpLCBj YWxsYmFja0lEKSk7CisgICAgICAgIHJldHVybjsKKyAgICB9CisKKyAgICBzZW5kKE1lc3NhZ2Vz OjpXZWJQYWdlUHJveHk6OkF0dHJpYnV0ZWRTdHJpbmdGb3JDaGFyYWN0ZXJSYW5nZUNhbGxiYWNr KHJlc3VsdCwgcmFuZ2VUb1NlbmQsIGNhbGxiYWNrSUQpKTsKIH0KIAogdm9pZCBXZWJQYWdlOjpm b250QXRTZWxlY3Rpb24odWludDY0X3QgY2FsbGJhY2tJRCkK