Bug 200628 – Crash under NetworkResourceLoader::start()

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

Created attachment 376064 [details]
Patch

[youenn fablet]

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?

[Chris Dumez]

(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.

[Chris Dumez]

(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

[youenn fablet]

> 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.

[youenn fablet]

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.

[Chris Dumez]

Created attachment 376068 [details]
Patch

[Ryosuke Niwa]

(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.

[Chris Dumez]

Committed r248540: <https://trac.webkit.org/changeset/248540>

[Radar WebKit Bug Importer]

<rdar://problem/54220519>