Bug 171616

Summary: REGRESSION (r216129): ASSERTION FAILED: m_process->state() == WebProcessProxy::State::Terminated
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, beidson, cdumez, ggaren, jlewis3, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 171565    
Attachments:
Description Flags
Patch none

Description Ryan Haddad 2017-05-03 13:59:14 PDT
Tests that failed:
  WebKit2.ReloadPageAfterCrash
  WebKit2.ResizeWindowAfterCrash

UNEXPECTEDLY EXITED WebKit2.ReloadPageAfterCrash
ASSERTION FAILED: m_process->state() == WebProcessProxy::State::Terminated
/Volumes/Data/slave/sierra-debug/build/Source/WebKit2/UIProcess/WebPageProxy.cpp(707) : void WebKit::WebPageProxy::reattachToWebProcess()
1   0x1080a2f1d WTFCrash
2   0x10dafa073 WebKit::WebPageProxy::reattachToWebProcess()
3   0x10dafe4ce WebKit::WebPageProxy::loadRequest(WebCore::ResourceRequest const&, WebCore::ShouldOpenExternalURLsPolicy, API::Object*)
4   0x10dee689b WKPageLoadURL
5   0x1065c97ea TestWebKitAPI::WebKit2_ReloadPageAfterCrash_Test::TestBody()
6   0x10671d7aa testing::Test::Run()
7   0x10671e5dd testing::internal::TestInfoImpl::Run()
8   0x10671f5dd testing::TestCase::Run()
9   0x106725d1b testing::internal::UnitTestImpl::RunAllTests()
10  0x106725999 testing::UnitTest::Run()
11  0x10661242c TestWebKitAPI::TestsController::run(int, char**)
12  0x1066ed8df main
13  0x7fffa4e93235 start
14  0x2

UNEXPECTEDLY EXITED WebKit2.ResizeWindowAfterCrash
ASSERTION FAILED: m_process->state() == WebProcessProxy::State::Terminated
/Volumes/Data/slave/sierra-debug/build/Source/WebKit2/UIProcess/WebPageProxy.cpp(707) : void WebKit::WebPageProxy::reattachToWebProcess()
1   0x10dc53f1d WTFCrash
2   0x111d22073 WebKit::WebPageProxy::reattachToWebProcess()
3   0x111d264ce WebKit::WebPageProxy::loadRequest(WebCore::ResourceRequest const&, WebCore::ShouldOpenExternalURLsPolicy, API::Object*)
4   0x11210e89b WKPageLoadURL
5   0x10c183c39 TestWebKitAPI::WebKit2_ResizeWindowAfterCrash_Test::TestBody()
6   0x10c2ce7aa testing::Test::Run()
7   0x10c2cf5dd testing::internal::TestInfoImpl::Run()
8   0x10c2d05dd testing::TestCase::Run()
9   0x10c2d6d1b testing::internal::UnitTestImpl::RunAllTests()
10  0x10c2d6999 testing::UnitTest::Run()
11  0x10c1c342c TestWebKitAPI::TestsController::run(int, char**)
12  0x10c29e8df main
13  0x7fffa4e93235 start
14  0x2

https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK1%20%28Tests%29/builds/946/steps/run-api-tests/logs/stdio
Comment 1 Ryan Haddad 2017-05-03 13:59:34 PDT
Started with https://trac.webkit.org/changeset/216129/webkit
Comment 2 Chris Dumez 2017-05-03 14:04:40 PDT
(In reply to Ryan Haddad from comment #1)
> Started with https://trac.webkit.org/changeset/216129/webkit

Looking now.
Comment 3 Chris Dumez 2017-05-03 14:19:31 PDT
Those 2 API tests do the same thing:
WKPageTerminate()
WKPageLoadURL()

and in their didCrash() handler, they call WKPageReload().

so when the didCrash handler gets called, we have already started a load.

The reason this started failing with my patch is that before r216129, the processDidCrash delegates were not called as a result to calls to WKPageTerminate().

I did it in r216129 so I could test my new processDidCrash delegate.
Comment 4 Chris Dumez 2017-05-03 14:22:33 PDT
The issue seems to be here:
void WebPageProxy::terminateProcess()
{
    // NOTE: This uses a check of m_isValid rather than calling isValid() since
    // we want this to run even for pages being closed or that already closed.
    if (!m_isValid)
        return;

    m_process->requestTermination();
    resetStateAfterProcessExited();
}

We call resetStateAfterProcessExited() after calling requestTermination(). However, after r216129, requestTermination() ends up calling the processDidCrash delegate before we get a chance to reset the state.
Comment 5 Chris Dumez 2017-05-03 14:24:15 PDT
(In reply to Chris Dumez from comment #4)
> The issue seems to be here:
> void WebPageProxy::terminateProcess()
> {
>     // NOTE: This uses a check of m_isValid rather than calling isValid()
> since
>     // we want this to run even for pages being closed or that already
> closed.
>     if (!m_isValid)
>         return;
> 
>     m_process->requestTermination();
>     resetStateAfterProcessExited();
> }
> 
> We call resetStateAfterProcessExited() after calling requestTermination().
> However, after r216129, requestTermination() ends up calling the
> processDidCrash delegate before we get a chance to reset the state.

It seems we do not need to call resetStateAfterProcessExited() at all in WebPageProxy::terminateProcess() since requestTermination() will call WebPageProxy::processDidCrash(), which already calls resetStateAfterProcessExited().
Comment 6 Chris Dumez 2017-05-03 14:27:05 PDT
Created attachment 308953 [details]
Patch
Comment 7 Chris Dumez 2017-05-03 14:27:23 PDT
(In reply to Chris Dumez from comment #6)
> Created attachment 308953 [details]
> Patch

Still building locally to confirm the fix.
Comment 8 Chris Dumez 2017-05-03 14:36:38 PDT
Comment on attachment 308953 [details]
Patch

API tests are passing in Debug locally with this fix.
Comment 9 Chris Dumez 2017-05-03 14:56:38 PDT
Comment on attachment 308953 [details]
Patch

Clearing flags on attachment: 308953

Committed r216144: <http://trac.webkit.org/changeset/216144>
Comment 10 Chris Dumez 2017-05-03 14:56:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Alexey Proskuryakov 2017-05-03 16:26:41 PDT
> before r216129, the processDidCrash delegates were not called as a result to calls to WKPageTerminate()

That seems like correct behavior; changing it just for testing doesn't sound right. One can always send a kill signal to properly simulate a crash, termination is not a crash.
Comment 12 Chris Dumez 2017-05-03 21:37:41 PDT
(In reply to Alexey Proskuryakov from comment #11)
> > before r216129, the processDidCrash delegates were not called as a result to calls to WKPageTerminate()
> 
> That seems like correct behavior; changing it just for testing doesn't sound
> right. One can always send a kill signal to properly simulate a crash,
> termination is not a crash.

Addressing feedback via https://bugs.webkit.org/show_bug.cgi?id=171624.