[ Mac Debug ] TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessForRegularNavigationRetainBundlePage is a flaky crash

• Component: WebKit API
• Status: RESOLVED FIXED

[Shawn Roberts]

The following API test is a flaky crash on Mojave Debug

TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessForRegularNavigationRetainBundlePage

Probable cause:

Flaky failures have been observed in on Mojave Debug queues for its entire available history https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29?numbuilds=200

Test was added in https://trac.webkit.org/changeset/241306/webkit 

Local testing with current revision and r241306 yield the same crash logs. Took about 15 tries to get it to crash locally.

Reproduced with:

run-api-tests TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessForRegularNavigationRetainBundlePage --debug

Will attach full crash log to radar.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000001020a1d00 WTFCrash + 16
1   com.apple.WebKit              	0x0000000107235bcb WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebKit              	0x0000000107972cc7 WebKit::FrameLoadState::didFailProvisionalLoad() + 103
3   com.apple.WebKit              	0x0000000107aa9199 WebKit::WebFrameProxy::didFailProvisionalLoad() + 25
4   com.apple.WebKit              	0x0000000107998aa9 WebKit::ProvisionalPageProxy::didFailProvisionalLoadForFrame(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WTF::String const&, WebCore::ResourceError const&, WebKit::UserData const&) + 393
5   com.apple.WebKit              	0x00000001079aef12 void IPC::callMemberFunctionImpl<WebKit::ProvisionalPageProxy, void (WebKit::ProvisionalPageProxy::*)(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WTF::String const&, WebCore::ResourceError const&, WebKit::UserData const&), std::__1::tuple<unsigned long long, WebCore::SecurityOriginData, unsigned long long, WTF::String, WebCore::ResourceError, WebKit::UserData>, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul>(WebKit::ProvisionalPageProxy*, void (WebKit::ProvisionalPageProxy::*)(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WTF::String const&, WebCore::ResourceError const&, WebKit::UserData const&), std::__1::tuple<unsigned long long, WebCore::SecurityOriginData, unsigned long long, WTF::String, WebCore::ResourceError, WebKit::UserData>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul>) + 434

[Radar WebKit Bug Importer]

<rdar://problem/49567254>

[Alex Christensen]

ASSERTION FAILED: m_state == State::Provisional

[Alex Christensen]

m_state is State::Finished.  I'm also hitting the ASSERT_NOT_REACHED() under if (fromItem->url() != pageLoadState().url()).  I bet we need to do something like https://trac.webkit.org/changeset/243767/webkit but for didFailProvisionalLoadForFrame

[Alex Christensen]

That didn't fix it.

[Alex Christensen]

FrameLoadState::didFinishLoad is being called before FrameLoadState::didFailProvisionalLoad...

[Alex Christensen]

It looks like didStartProvisionalLoad is not being called before didFailProvisionalLoadForFrame on a frame that has already completed a load.  It's hard to debug though because even adding logs makes it not reproduce.

[Chris Dumez]

Created attachment 367341 [details]
Patch

[Chris Dumez]

Comment on attachment 367341 [details]
Patch

I have a follow-up plan to see if we can avoid having to ignore leftover IPC explicitly. This will likely require some significant refactoring though so I think we should land this first.

[Darin Adler]

Comment on attachment 367341 [details]
Patch

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

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:217
> +    // If the previous provisional load used an existing process, we may receive leftover IPC for a previous navigation, which we need to ignore.
> +    if (!m_mainFrame || m_mainFrame->frameID() != frameID)
> +        return;

See below. Could have an overload that takes just a frame ID and not a navigation ID.

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:226
> +    // If the previous provisional load used an existing process, we may receive leftover IPC for a previous navigation, which we need to ignore.
> +    if (navigationID != m_navigationID || !m_mainFrame || m_mainFrame->frameID() != frameID)
>          return;

With this idiom repeated so many times, I suggest adding a named member function that takes navigationID and frameID as arguments. A bonus is that the function’s name can help document and so we won’t have such repetitive comments either.

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:347
> +    if (!isMainFrame || (m_mainFrame && m_mainFrame->frameID() != frameID) || navigationID != m_navigationID) {

I think we can even refactor this one to use the new helper.

[Darin Adler]

Comment on attachment 367341 [details]
Patch

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

>> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:226
>>          return;
> 
> With this idiom repeated so many times, I suggest adding a named member function that takes navigationID and frameID as arguments. A bonus is that the function’s name can help document and so we won’t have such repetitive comments either.

Seems fine to land without my suggested refinement, but I think it’s better for the long term.

[Chris Dumez]

Created attachment 367398 [details]
Patch

[Chris Dumez]

Created attachment 367399 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367399 [details]
Patch

Clearing flags on attachment: 367399

Committed r244243: <https://trac.webkit.org/changeset/244243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.