We see the following flaky crash on the bots: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x00000001d319fb58 Exception Note: EXC_CORPSE_NOTIFY Termination Signal: Segmentation fault: 11 Termination Reason: Namespace SIGNAL, Code 0xb Terminating Process: exc handler [67946] Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebKit 0x00000001069ea7ad WebKit::NetworkResourceLoader::isAlwaysOnLoggingAllowed() const + 9 1 com.apple.WebKit 0x0000000106a09885 void WTF::__visitor_table<WTF::Visitor<auto WebKit::NetworkResourceLoader::start()::$_71::operator()<WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> >(WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError>&&) const::'lambda'(WebCore::ResourceError&), WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> WebKit::NetworkResourceLoader::start()::$_71::operator()<WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> >(WebCore::ResourceError&) const::'lambda'(WebKit::NetworkLoadChecker::RedirectionTriplet&), WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> WebKit::NetworkResourceLoader::start()::$_71::operator()<WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> >(WebCore::ResourceError&) const::'lambda'(WebCore::ResourceRequest&)>, WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError>::__trampoline_func<WebCore::ResourceError>(WTF::__visitor_table<WTF::Visitor<auto WebKit::NetworkResourceLoader::start()::$_71::operator()<WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> >(WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError>&&) const::'lambda'(WebCore::ResourceError&), WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> WebKit::NetworkResourceLoader::start()::$_71::operator()<WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> >(WebCore::ResourceError&) const::'lambda'(WebKit::NetworkLoadChecker::RedirectionTriplet&), WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> WebKit::NetworkResourceLoader::start()::$_71::operator()<WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError> >(WebCore::ResourceError&) const::'lambda'(WebCore::ResourceRequest&)>, WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError>&, WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError>&) + 61 2 com.apple.WebKit 0x0000000106a095a4 WTF::Detail::CallableWrapper<WebKit::NetworkResourceLoader::start()::$_71, void, WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError>&&>::call(WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError>&&) + 52 3 com.apple.WebKit 0x00000001069f8000 WTF::Detail::CallableWrapper<WebKit::NetworkLoadChecker::checkRequest(WebCore::ResourceRequest&&, WebCore::ContentSecurityPolicyClient*, WTF::CompletionHandler<void (WTF::Variant<WebCore::ResourceRequest, WebKit::NetworkLoadChecker::RedirectionTriplet, WebCore::ResourceError>&&)>&&)::$_2, void, WebCore::ResourceRequest&&>::call(WebCore::ResourceRequest&&) + 770 4 com.apple.WebKit 0x00000001069f7ba7 WTF::Detail::CallableWrapper<WebKit::NetworkLoadChecker::applyHTTPSUpgradeIfNeeded(WebCore::ResourceRequest&&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&) const::$_1, void, bool>::call(bool) + 223 5 com.apple.WebKit 0x00000001069c9bdf WTF::Detail::CallableWrapper<WebKit::NetworkHTTPSUpgradeChecker::query(WTF::String&&, PAL::SessionID, WTF::CompletionHandler<void (bool)>&&)::$_22::operator()()::'lambda0'(), void>::call() + 29 6 com.apple.JavaScriptCore 0x0000000108ee78fd WTF::RunLoop::performWork() + 237 7 com.apple.JavaScriptCore 0x0000000108ee7bda WTF::RunLoop::performWork(void*) + 26 8 com.apple.CoreFoundation 0x00007fff453143bb __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 9 com.apple.CoreFoundation 0x00007fff45314361 __CFRunLoopDoSource0 + 108 10 com.apple.CoreFoundation 0x00007fff452f826b __CFRunLoopDoSources0 + 195 11 com.apple.CoreFoundation 0x00007fff452f7833 __CFRunLoopRun + 1196 12 com.apple.CoreFoundation 0x00007fff452f7135 CFRunLoopRunSpecific + 459 13 com.apple.Foundation 0x00007fff475797df -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 280 14 com.apple.Foundation 0x00007fff475796b4 -[NSRunLoop(NSRunLoop) run] + 76 15 libxpc.dylib 0x00007fff7159d077 _xpc_objc_main + 552 16 libxpc.dylib 0x00007fff7159cb79 xpc_main + 433 17 com.apple.WebKit 0x0000000106a8d423 WebKit::XPCServiceMain(int, char const**) + 547 18 libdyld.dylib 0x00007fff713643d5 start + 1
Created attachment 376064 [details] Patch
Comment on attachment 376064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376064&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:177 > + m_networkLoadChecker->check(ResourceRequest { originalRequest() }, this, [this, weakThis = makeWeakPtr(*this)] (auto&& result) { How is it possible that 'this' is not valid here, given that NetworkLoadChecker is owned by NetworkResourceLoader. If NetworkResourceLoader gets destroyed, NetworkLoadChecker as well and the callback should be executed with a cancellation error which should be properly handled. If that is not the case, the fix should probably be at NetworkLoadChecker level. Or is it the error logging in the lambda that triggers the crash? > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:240 > + m_cache->retrieve(request, { m_parameters.webPageID, m_parameters.webFrameID }, [this, weakThis = makeWeakPtr(*this), request = ResourceRequest { request }](auto entry, auto info) mutable { I am not clear why this will fix a crash here given we go from refing to use a weakThis. Other places of the code do makeRef(*this) (like one m_cache->store call or a checkRedirection call). Should we try to be consistent?
(In reply to youenn fablet from comment #2) > Comment on attachment 376064 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376064&action=review > > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:177 > > + m_networkLoadChecker->check(ResourceRequest { originalRequest() }, this, [this, weakThis = makeWeakPtr(*this)] (auto&& result) { > > How is it possible that 'this' is not valid here, given that > NetworkLoadChecker is owned by NetworkResourceLoader. > If NetworkResourceLoader gets destroyed, NetworkLoadChecker as well and the > callback should be executed with a cancellation error which should be > properly handled. > If that is not the case, the fix should probably be at NetworkLoadChecker > level. > Or is it the error logging in the lambda that triggers the crash? It is the error logging in the lambda that triggers the crash. Also note that you are capturing |this| in that lambda, which may be a stale pointer upon returning. > > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:240 > > + m_cache->retrieve(request, { m_parameters.webPageID, m_parameters.webFrameID }, [this, weakThis = makeWeakPtr(*this), request = ResourceRequest { request }](auto entry, auto info) mutable { > > I am not clear why this will fix a crash here given we go from refing to use > a weakThis. > Other places of the code do makeRef(*this) (like one m_cache->store call or > a checkRedirection call). > Should we try to be consistent? This part of the change does not fix a crash. I think that capturing a RefPtr and returning early if hasOneRef() is much uglier than capturing a WeakPtr and returning early if !weakThis. I think the !weakThis pattern is much more common than hasOneRef(). Because I started subclassing CanMakeWeakPtr in this patch, we can now use WeakPtr here.
(In reply to Chris Dumez from comment #3) > (In reply to youenn fablet from comment #2) > > Comment on attachment 376064 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=376064&action=review > > > > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:177 > > > + m_networkLoadChecker->check(ResourceRequest { originalRequest() }, this, [this, weakThis = makeWeakPtr(*this)] (auto&& result) { > > > > How is it possible that 'this' is not valid here, given that > > NetworkLoadChecker is owned by NetworkResourceLoader. > > If NetworkResourceLoader gets destroyed, NetworkLoadChecker as well and the > > callback should be executed with a cancellation error which should be > > properly handled. > > If that is not the case, the fix should probably be at NetworkLoadChecker > > level. > > Or is it the error logging in the lambda that triggers the crash? > > It is the error logging in the lambda that triggers the crash. Also note > that you are capturing |this| in that lambda, which may be a stale pointer > upon returning. You can tell it is the logging because we crash here: WebKit::NetworkResourceLoader::isAlwaysOnLoggingAllowed() const + 9
> This part of the change does not fix a crash. I think that capturing a > RefPtr and returning early if hasOneRef() is much uglier than capturing a > WeakPtr and returning early if !weakThis. > I think the !weakThis pattern is much more common than hasOneRef(). Because > I started subclassing CanMakeWeakPtr in this patch, we can now use WeakPtr > here. So the plan is to try moving away from makeRef and use makeWeakPtr instead, and hopefully move from RefCounted to unique_ptr. If so, that seems good to me.
Comment on attachment 376064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376064&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:219 > + auto protectedThis = makeRef(*this); We might not need that ref here anymore. The code below is new and handles prefetch, I doubt it can trigger a loader cancellation.
Created attachment 376068 [details] Patch
(In reply to youenn fablet from comment #6) > Comment on attachment 376064 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376064&action=review > > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:219 > > + auto protectedThis = makeRef(*this); > > We might not need that ref here anymore. > The code below is new and handles prefetch, I doubt it can trigger a loader > cancellation. I think we should keep this around for now to reduce the risk, and isolate code changes. We can get rid of it later in a separate patch as a cleanup.
Committed r248540: <https://trac.webkit.org/changeset/248540>
<rdar://problem/54220519>