com.apple.WebKit.Networking crashes at -[WebCoreAuthenticationClientAsChallengeSender performDefaultHandlingForAuthenticationChallenge:] due to an unrecognized selector sent to instance: Crashed Thread: 0 Dispatch queue: com.apple.main-thread Exception Type: EXC_CRASH (SIGABRT) Exception Codes: 0x0000000000000000, 0x0000000000000000 Exception Note: EXC_CORPSE_NOTIFY Application Specific Information: *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[WebCoreAuthenticationClientAsChallengeSender performDefaultHandlingForAuthenticationChallenge:]: unrecognized selector sent to instance 0x7fbe4ba2c100' abort() called terminating with uncaught exception of type NSException Application Specific Backtrace 1: 0 CoreFoundation 0x00007fff81124e32 __exceptionPreprocess + 178 1 libobjc.A.dylib 0x00007fff8a34f4fa objc_exception_throw + 48 2 CoreFoundation 0x00007fff8118e34d -[NSObject(NSObject) doesNotRecognizeSelector:] + 205 3 CoreFoundation 0x00007fff81095472 ___forwarding___ + 514 4 CoreFoundation 0x00007fff810951e8 _CF_forwarding_prep_0 + 120 5 WebKit 0x00007fff8bf08490 _ZN6WebKit21AuthenticationManager40performDefaultHandlingForSingleChallengeEy + 104 6 WebKit 0x00007fff8bf083d9 _ZN6WebKit21AuthenticationManager22performDefaultHandlingEy + 73 7 WebKit 0x00007fff8bf098ca _ZN6WebKit21AuthenticationManager17didReceiveMessageERN3IPC10ConnectionERNS1_14MessageDecoderE + 196 8 WebKit 0x00007fff8bf44b61 _ZN3IPC18MessageReceiverMap15dispatchMessageERNS_10ConnectionERNS_14MessageDecoderE + 113 9 WebKit 0x00007fff8bf69216 _ZN6WebKit14NetworkProcess17didReceiveMessageERN3IPC10ConnectionERNS1_14MessageDecoderE + 28 10 WebKit 0x00007fff8bf0c602 _ZN3IPC10Connection15dispatchMessageENSt3__110unique_ptrINS_14MessageDecoderENS1_14default_deleteIS3_EEEE + 102 11 WebKit 0x00007fff8bf0eb2e _ZN3IPC10Connection18dispatchOneMessageEv + 114 12 JavaScriptCore 0x00007fff92100d85 _ZN3WTF7RunLoop11performWorkEv + 437 13 JavaScriptCore 0x00007fff92101462 _ZN3WTF7RunLoop11performWorkEPv + 34 14 CoreFoundation 0x00007fff810ba8b1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 15 CoreFoundation 0x00007fff8109a0ac __CFRunLoopDoSources0 + 556 16 CoreFoundation 0x00007fff810995cf __CFRunLoopRun + 927 17 CoreFoundation 0x00007fff81098fc8 CFRunLoopRunSpecific + 296 18 Foundation 0x00007fff86bcdf59 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 270 19 Foundation 0x00007fff86bcde38 -[NSRunLoop(NSRunLoop) run] + 74 20 libxpc.dylib 0x00007fff8412a4c8 _xpc_objc_main + 751 21 libxpc.dylib 0x00007fff84128f1e xpc_main + 494 22 com.apple.WebKit.Networking 0x10df6db4a main + 16 (Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:89) 23 libdyld.dylib 0x00007fff94b305ad start + 1 Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 libsystem_kernel.dylib 0x00007fff884c60ae __pthread_kill + 10 1 libsystem_pthread.dylib 0x00007fff90a9a500 pthread_kill + 90 2 libsystem_c.dylib 0x00007fff8723137b abort + 129 3 libc++abi.dylib 0x00007fff900baf81 abort_message + 257 4 libc++abi.dylib 0x00007fff900e0a47 default_terminate_handler() + 267 5 libobjc.A.dylib 0x00007fff8a35115e _objc_terminate() + 103 6 libc++abi.dylib 0x00007fff900de19e std::__terminate(void (*)()) + 8 7 libc++abi.dylib 0x00007fff900dde27 __cxa_rethrow + 99 8 libobjc.A.dylib 0x00007fff8a34f998 objc_exception_rethrow + 40 9 com.apple.CoreFoundation 0x00007fff8109905b CFRunLoopRunSpecific + 443 10 com.apple.Foundation 0x00007fff86bcdf59 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 270 11 com.apple.Foundation 0x00007fff86bcde38 -[NSRunLoop(NSRunLoop) run] + 74 12 libxpc.dylib 0x00007fff8412a4c8 _xpc_objc_main + 751 13 libxpc.dylib 0x00007fff84128f1e xpc_main + 494 14 com.apple.WebKit.Networking 0x10df6db4a main + 16 (Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:89) 15 libdyld.dylib 0x00007fff94b305ad start + 1
<rdar://problem/23325160>
Created attachment 277136 [details] Patch v1
Attachment 277136 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > Created attachment 277136 [details] > Patch v1 Looking for feedback on the approach for the patch, as well as suggestions for how to write a test. (Does this path require SSL certificate authentication? Do changes to Tools projects look like they're reasonable?)
Comment on attachment 277136 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=277136&action=review > Does this path require SSL certificate authentication? Unsure. Maybe sending -performDefaultHandlingForAuthenticationChallenge: for a password challenge would result in returning the original 401 response. > Source/WebCore/ChangeLog:24 > + - Add -respondsToSelector: checks since > + -performDefaultHandlingForAuthenticationChallenge: and > + -rejectProtectionSpaceAndContinueWithChallenge: are optional > + by API contract. I'm not sure how this can work. If we don't send the message, then the sender will never know about the decision, and the connection will leak, won't it? > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:571 > - [challenge.sender() performDefaultHandlingForAuthenticationChallenge:challenge.nsURLAuthenticationChallenge()]; > + if ([challenge.sender() respondsToSelector:@selector(performDefaultHandlingForAuthenticationChallenge:)]) > + [challenge.sender() performDefaultHandlingForAuthenticationChallenge:challenge.nsURLAuthenticationChallenge()]; Is the sender here always CFNetwork? It would seem surprising if it's free to decide which methods of the protocol to implement. > Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:206 > + if (gTestRunner->performsDefaultHandlingForAuthenticationChallenge()) { This is starting to look like we should let tests implement authentication callbacks in JS, instead of controlling client behavior with a set of confusing interdependent booleans.
Created attachment 277147 [details] Patch v2
Attachment 277147 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 277136 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=277136&action=review >> Source/WebCore/ChangeLog:24 >> + by API contract. > > I'm not sure how this can work. If we don't send the message, then the sender will never know about the decision, and the connection will leak, won't it? The API contract clearly states these methods are optional: <https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Protocols/NSURLAuthenticationChallengeSender_Protocol/#//apple_ref/doc/uid/20001706-SW2> >> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:571 >> + [challenge.sender() performDefaultHandlingForAuthenticationChallenge:challenge.nsURLAuthenticationChallenge()]; > > Is the sender here always CFNetwork? It would seem surprising if it's free to decide which methods of the protocol to implement. I'm not sure who the sender is, but why is that relevant. The API contract is very clear that these methods are optional, so why wouldn't we check here?
> I'm not sure who the sender is, but why is that relevant. The API contract is very clear that these methods are optional, so why wouldn't we check here? That's not the API contract, that's something that unrelated code using this protocol may choose to do. What matters to us is what CFNetwork does, not what unrelated theoretical class in user code is allowed to do.
To put it another way, if CFNetwork ever provides a sender that doesn't implement these methods, we should probably fall back to another method, not just drop the challenge on the floor. My uninformed guess is that CFNetwork always implements these, and we are actually dealing with a use after free.
(In reply to comment #10) > To put it another way, if CFNetwork ever provides a sender that doesn't > implement these methods, we should probably fall back to another method, not > just drop the challenge on the floor. > > My uninformed guess is that CFNetwork always implements these, and we are > actually dealing with a use after free. Moving to RESOLVED/INVALID.
I think we should at least land the respondsToSelector checks.
Is there any evidence that these checks are appropriate? I wouldn't want to confuse the code with unnecessary checks when we expect that there is some other kind of bug in it.
(In reply to comment #13) > Is there any evidence that these checks are appropriate? I wouldn't want to > confuse the code with unnecessary checks when we expect that there is some > other kind of bug in it. Yes. The methods are marked as optional in the NSProtocol, so they should be checked to see if they exist before calling them on an object. Why would it ever be correct to assume that optional methods are always implemented?
The methods are optional in protocol, which means that some other code using the protocol is not required to implement them. That is not evidence towards what CFNetwork is required/documented/expected to do.
Reopening to attach new patch.
Created attachment 284636 [details] Patch
Comment on attachment 284636 [details] Patch Clearing flags on attachment: 284636 Committed r203743: <http://trac.webkit.org/changeset/203743>
All reviewed patches have been landed. Closing bug.
Landed without respondsToSelector checks. Having missing methods in WebCoreAuthenticationClientAsChallengeSender is demonstrably wrong, so we added them with a test.
Minor test fix in https://trac.webkit.org/changeset/203755
I assume that the INVALID state was accidental, moving to FIXED.