Bug 196548 - [ Mac Debug ] TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessForRegularNavigationRetainBundlePage is a flaky crash
Summary: [ Mac Debug ] TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessForRegularNaviga...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh macOS 10.14
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-03 11:37 PDT by Shawn Roberts
Modified: 2019-04-13 21:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.83 KB, patch)
2019-04-12 12:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.18 KB, patch)
2019-04-13 18:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2019-04-13 18:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Roberts 2019-04-03 11:37:21 PDT
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
Comment 1 Radar WebKit Bug Importer 2019-04-03 11:55:13 PDT
<rdar://problem/49567254>
Comment 2 Alex Christensen 2019-04-03 14:50:40 PDT
ASSERTION FAILED: m_state == State::Provisional
Comment 3 Alex Christensen 2019-04-03 15:01:39 PDT
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
Comment 4 Alex Christensen 2019-04-03 15:37:03 PDT
That didn't fix it.
Comment 5 Alex Christensen 2019-04-03 15:48:06 PDT
FrameLoadState::didFinishLoad is being called before FrameLoadState::didFailProvisionalLoad...
Comment 6 Alex Christensen 2019-04-04 12:21:30 PDT
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.
Comment 7 Chris Dumez 2019-04-12 12:52:33 PDT
Created attachment 367341 [details]
Patch
Comment 8 Chris Dumez 2019-04-12 14:36:44 PDT
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.
Comment 9 Darin Adler 2019-04-13 08:26:50 PDT
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.
Comment 10 Darin Adler 2019-04-13 08:27:25 PDT
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.
Comment 11 Chris Dumez 2019-04-13 18:22:30 PDT
Created attachment 367398 [details]
Patch
Comment 12 Chris Dumez 2019-04-13 18:55:47 PDT
Created attachment 367399 [details]
Patch
Comment 13 WebKit Commit Bot 2019-04-13 21:22:39 PDT
Comment on attachment 367399 [details]
Patch

Clearing flags on attachment: 367399

Committed r244243: <https://trac.webkit.org/changeset/244243>
Comment 14 WebKit Commit Bot 2019-04-13 21:22:41 PDT
All reviewed patches have been landed.  Closing bug.