Bug 212678 – REGRESSION (r262212): [ iOS Debug wk2 ] ASSERTION FAILED: !isSynchronous() || !m_synchronousLoadData->delayedReply in WebKit::NetworkResourceLoader

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

This test started flaky crashing after r262212

[Kate Cheney]

<rdar://problem/63797758>

[Chris Dumez]

From stderr, we can see:
Failed to load about:blank, terminating process and trying again.
ASSERTION FAILED: !isSynchronous() || !m_synchronousLoadData->delayedReply
/Volumes/Data/worker/build/OpenSource/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp(137) : virtual WebKit::NetworkResourceLoader::~NetworkResourceLoader()
1   0x1262664c9 WTFCrash
2   0x119f32e3b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x11a58f88d WebKit::NetworkResourceLoader::~NetworkResourceLoader()
4   0x11a58fcd5 WebKit::NetworkResourceLoader::~NetworkResourceLoader()
5   0x11a58fd5c WebKit::NetworkResourceLoader::~NetworkResourceLoader()
6   0x11a1c250f std::__1::default_delete<WebKit::NetworkResourceLoader>::operator()(WebKit::NetworkResourceLoader*) const
7   0x11a1c24d2 WTF::RefCounted<WebKit::NetworkResourceLoader, std::__1::default_delete<WebKit::NetworkResourceLoader> >::deref() const
8   0x11a53bef7 void WTF::derefIfNotNull<WebKit::NetworkResourceLoader>(WebKit::NetworkResourceLoader*)
9   0x11a53beb9 WTF::RefPtr<WebKit::NetworkResourceLoader, WTF::DumbPtrTraits<WebKit::NetworkResourceLoader> >::~RefPtr()
10  0x11a514cb5 WTF::RefPtr<WebKit::NetworkResourceLoader, WTF::DumbPtrTraits<WebKit::NetworkResourceLoader> >::~RefPtr()
11  0x11a58b158 WebKit::NetworkResourceLoadMap::remove(unsigned long long)
12  0x11a5149a3 WebKit::NetworkConnectionToWebProcess::didCleanupResourceLoader(WebKit::NetworkResourceLoader&)
13  0x11a595d28 WebKit::NetworkResourceLoader::cleanup(WebKit::NetworkResourceLoader::LoadResult)
14  0x11a596e9f WebKit::NetworkResourceLoader::abort()
15  0x11a515ce6 WebKit::NetworkConnectionToWebProcess::didClose(IPC::Connection&)
16  0x119fc26f7 IPC::Connection::connectionDidClose()::$_6::operator()()
17  0x119fc25ee WTF::Detail::CallableWrapper<IPC::Connection::connectionDidClose()::$_6, void>::call()
18  0x12628ec32 WTF::Function<void ()>::operator()() const
19  0x1262fece8 WTF::RunLoop::performWork()
20  0x1262ff6b1 WTF::RunLoop::performWork(void*)
21  0x10f4b4c71 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
22  0x10f4b4b9c __CFRunLoopDoSource0
23  0x10f4b4374 __CFRunLoopDoSources0
24  0x10f4aef6e __CFRunLoopRun
25  0x10f4ae884 CFRunLoopRunSpecific
26  0x10ed77831 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
27  0x10ed77a45 -[NSRunLoop(NSRunLoop) run]
28  0x1110fa5ae _xpc_objc_main
29  0x1110fc976 xpc_main
30  0x11a6ae27b WebKit::XPCServiceMain(int, char const**)
31  0x11bb3beab WKXPCServiceMain
LEAK: 3 WebPageProxy

So the WebProcess appears to be unresponsive and at least failing to load about:blank before (or after) the test. As a result, WebKitTestRunner terminates the WebContent process. The termination of the WebContent process triggers some logic in the network process which ends up hitting the assertion. Given the assertion and the test, this is likely related to terminating a WebContent process while a synchronous XHR is going on.

[Kate Cheney]

Since this was caused by r262212, I'm guessing it has something to do with clearing the bundle identifier in the network process between tests.

[Kate Cheney]

Created attachment 401040 [details]
Patch

[Chris Dumez]

Comment on attachment 401040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401040&action=review

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2769
>      WebCore::clearApplicationBundleIdentifierTestingOverride();

If this clears an "override", then why do we need to set the identifier again then? From implementation, it looks like override and actual identifier are stored separately. Therefore, once the override is gone, I am unclear why we don't automatically go back to using the default one..

> Source/WebKit/NetworkProcess/NetworkProcess.h:359
> +    void resetBundleIdentifierTesting(String&&, CompletionHandler<void()>&&);

Seems like this should be "ForTesting", not "Testing".

[Kate Cheney]

Comment on attachment 401040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401040&action=review

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2769
>>      WebCore::clearApplicationBundleIdentifierTestingOverride();
> 
> If this clears an "override", then why do we need to set the identifier again then? From implementation, it looks like override and actual identifier are stored separately. Therefore, once the override is gone, I am unclear why we don't automatically go back to using the default one..

The bundle ID in the network process is set to the UI process bundle identifier in NetworkProcess::platformInitializeNetworkProcessCocoa(). Clearing it sets it back to com.apple.WebKit.Networking instead of com.apple.WebKitTestRunner.

[Chris Dumez]

(In reply to katherine_cheney from comment #6)
> Comment on attachment 401040 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401040&action=review
> 
> >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2769
> >>      WebCore::clearApplicationBundleIdentifierTestingOverride();
> > 
> > If this clears an "override", then why do we need to set the identifier again then? From implementation, it looks like override and actual identifier are stored separately. Therefore, once the override is gone, I am unclear why we don't automatically go back to using the default one..
> 
> The bundle ID in the network process is set to the UI process bundle
> identifier in NetworkProcess::platformInitializeNetworkProcessCocoa().
> Clearing it sets it back to com.apple.WebKit.Networking instead of
> com.apple.WebKitTestRunner.

This all seems to imply a bug somewhere else. When the network process launches, it should set its bundle identifier based on what the UIProcess tell it. Then if tests need to change this identifier, we should store the new value from the tests in this override variable (not overwrite the one the UIProcess provided). Then in between tests, it should be sufficient to clear the override. 

Override -> used for testing
NonOverride -> never used for testing

[Kate Cheney]

(In reply to Chris Dumez from comment #7)
> (In reply to katherine_cheney from comment #6)
> > Comment on attachment 401040 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=401040&action=review
> > 
> > >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2769
> > >>      WebCore::clearApplicationBundleIdentifierTestingOverride();
> > > 
> > > If this clears an "override", then why do we need to set the identifier again then? From implementation, it looks like override and actual identifier are stored separately. Therefore, once the override is gone, I am unclear why we don't automatically go back to using the default one..
> > 
> > The bundle ID in the network process is set to the UI process bundle
> > identifier in NetworkProcess::platformInitializeNetworkProcessCocoa().
> > Clearing it sets it back to com.apple.WebKit.Networking instead of
> > com.apple.WebKitTestRunner.
> 
> This all seems to imply a bug somewhere else. When the network process
> launches, it should set its bundle identifier based on what the UIProcess
> tell it. Then if tests need to change this identifier, we should store the
> new value from the tests in this override variable (not overwrite the one
> the UIProcess provided). Then in between tests, it should be sufficient to
> clear the override. 
> 
> Override -> used for testing
> NonOverride -> never used for testing

I see your point. The problem is, right now both tests and non-tests use the same setter, so there is no way to distinguish when a test is setting the identifier. I can add a new function to set the identifier for tests only, then store the non-test value so when we clear the override we can set it back to that value.

[Chris Dumez]

(In reply to katherine_cheney from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to katherine_cheney from comment #6)
> > > Comment on attachment 401040 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=401040&action=review
> > > 
> > > >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2769
> > > >>      WebCore::clearApplicationBundleIdentifierTestingOverride();
> > > > 
> > > > If this clears an "override", then why do we need to set the identifier again then? From implementation, it looks like override and actual identifier are stored separately. Therefore, once the override is gone, I am unclear why we don't automatically go back to using the default one..
> > > 
> > > The bundle ID in the network process is set to the UI process bundle
> > > identifier in NetworkProcess::platformInitializeNetworkProcessCocoa().
> > > Clearing it sets it back to com.apple.WebKit.Networking instead of
> > > com.apple.WebKitTestRunner.
> > 
> > This all seems to imply a bug somewhere else. When the network process
> > launches, it should set its bundle identifier based on what the UIProcess
> > tell it. Then if tests need to change this identifier, we should store the
> > new value from the tests in this override variable (not overwrite the one
> > the UIProcess provided). Then in between tests, it should be sufficient to
> > clear the override. 
> > 
> > Override -> used for testing
> > NonOverride -> never used for testing
> 
> I see your point. The problem is, right now both tests and non-tests use the
> same setter, so there is no way to distinguish when a test is setting the
> identifier. I can add a new function to set the identifier for tests only,
> then store the non-test value so when we clear the override we can set it
> back to that value.

Why do we have a concept of override then? If both tests and normal usage end up setting the same variable? Feels like this code needs to be refactored one way or another. Either no override and we use a single variable, or a different method for testing that sets the override but not the real value.

[Kate Cheney]

(In reply to Chris Dumez from comment #9)
> (In reply to katherine_cheney from comment #8)
> > (In reply to Chris Dumez from comment #7)
> > > (In reply to katherine_cheney from comment #6)
> > > > Comment on attachment 401040 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=401040&action=review
> > > > 
> > > > >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2769
> > > > >>      WebCore::clearApplicationBundleIdentifierTestingOverride();
> > > > > 
> > > > > If this clears an "override", then why do we need to set the identifier again then? From implementation, it looks like override and actual identifier are stored separately. Therefore, once the override is gone, I am unclear why we don't automatically go back to using the default one..
> > > > 
> > > > The bundle ID in the network process is set to the UI process bundle
> > > > identifier in NetworkProcess::platformInitializeNetworkProcessCocoa().
> > > > Clearing it sets it back to com.apple.WebKit.Networking instead of
> > > > com.apple.WebKitTestRunner.
> > > 
> > > This all seems to imply a bug somewhere else. When the network process
> > > launches, it should set its bundle identifier based on what the UIProcess
> > > tell it. Then if tests need to change this identifier, we should store the
> > > new value from the tests in this override variable (not overwrite the one
> > > the UIProcess provided). Then in between tests, it should be sufficient to
> > > clear the override. 
> > > 
> > > Override -> used for testing
> > > NonOverride -> never used for testing
> > 
> > I see your point. The problem is, right now both tests and non-tests use the
> > same setter, so there is no way to distinguish when a test is setting the
> > identifier. I can add a new function to set the identifier for tests only,
> > then store the non-test value so when we clear the override we can set it
> > back to that value.
> 
> Why do we have a concept of override then? If both tests and normal usage
> end up setting the same variable? Feels like this code needs to be
> refactored one way or another. Either no override and we use a single
> variable, or a different method for testing that sets the override but not
> the real value.

I don't know what the thought process was when writing the code. I agree a separate function for setting the identifier for tests would work.

[Kate Cheney]

Created attachment 401072 [details]
Patch

[EWS]

Committed r262582: <https://trac.webkit.org/changeset/262582>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401072 [details].

[Kate Cheney]

The correct radar is: rdar://problem/63884304