171616 2017-05-03 13:59:14 -0700 REGRESSION (r216129): ASSERTION FAILED: m_process->state() == WebProcessProxy::State::Terminated 2017-05-03 21:37:41 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 171565 1 ryanhaddad cdumez andersca beidson cdumez ggaren jlewis3 sam oldest_to_newest 1304225 0 ryanhaddad 2017-05-03 13:59:14 -0700 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 1304226 1 ryanhaddad 2017-05-03 13:59:34 -0700 Started with https://trac.webkit.org/changeset/216129/webkit 1304231 2 cdumez 2017-05-03 14:04:40 -0700 (In reply to Ryan Haddad from comment #1) > Started with https://trac.webkit.org/changeset/216129/webkit Looking now. 1304240 3 cdumez 2017-05-03 14:19:31 -0700 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. 1304243 4 cdumez 2017-05-03 14:22:33 -0700 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. 1304245 5 cdumez 2017-05-03 14:24:15 -0700 (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(). 1304246 6 308953 cdumez 2017-05-03 14:27:05 -0700 Created attachment 308953 Patch 1304247 7 cdumez 2017-05-03 14:27:23 -0700 (In reply to Chris Dumez from comment #6) > Created attachment 308953 [details] > Patch Still building locally to confirm the fix. 1304254 8 308953 cdumez 2017-05-03 14:36:38 -0700 Comment on attachment 308953 Patch API tests are passing in Debug locally with this fix. 1304274 9 308953 cdumez 2017-05-03 14:56:38 -0700 Comment on attachment 308953 Patch Clearing flags on attachment: 308953 Committed r216144: <http://trac.webkit.org/changeset/216144> 1304275 10 cdumez 2017-05-03 14:56:40 -0700 All reviewed patches have been landed. Closing bug. 1304360 11 ap 2017-05-03 16:26:41 -0700 > 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. 1304510 12 cdumez 2017-05-03 21:37:41 -0700 (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. 308953 2017-05-03 14:27:05 -0700 2017-05-03 14:56:38 -0700 Patch bug-171616-20170503142704.patch text/plain 1741 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjE2MTMwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggZmY0MDkyNGM5ODliMTM5 YzNlZWY0YzRmZGM3OTY2YjZiNTRiMjY0Zi4uOWJmZTkwZjI0YTJlMTI5ZjY0NDI3ODdiMzFlZjMw ZmU5YzI1NDg4YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIwIEBACiAyMDE3LTA1LTAzICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CiAKKyAgICAgICAgUkVHUkVTU0lPTiAocjIxNjEy OSk6IEFTU0VSVElPTiBGQUlMRUQ6IG1fcHJvY2Vzcy0+c3RhdGUoKSA9PSBXZWJQcm9jZXNzUHJv eHk6OlN0YXRlOjpUZXJtaW5hdGVkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0xNzE2MTYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBTdG9wIGNhbGxpbmcgcmVzZXRTdGF0ZUFmdGVyUHJvY2Vzc0V4aXRlZCgp IGluIFdlYlBhZ2VQcm94eTo6dGVybWluYXRlUHJvY2VzcygpIGFzIHRoZSBjYWxsIHRvCisgICAg ICAgIFdlYlByb2Nlc3NQcm94eTo6cmVxdWVzdFRlcm1pbmF0aW9uKCkgYWxyZWFkeSBjYXVzZXMg V2ViUGFnZVByb3h5Ojpwcm9jZXNzRGlkQ3Jhc2goKSB0byBiZSBjYWxsZWQKKyAgICAgICAgYWZ0 ZXIgcjIxNjEyOS4gV2ViUGFnZVByb3h5Ojpwcm9jZXNzRGlkQ3Jhc2goKSBhbHJlYWR5IHRha2Vz IGNhcmUgb2YgY2FsbGluZworICAgICAgICByZXNldFN0YXRlQWZ0ZXJQcm9jZXNzRXhpdGVkKCku CisKKyAgICAgICAgKiBVSVByb2Nlc3MvV2ViUGFnZVByb3h5LmNwcDoKKyAgICAgICAgKFdlYktp dDo6V2ViUGFnZVByb3h5Ojp0ZXJtaW5hdGVQcm9jZXNzKToKKworMjAxNy0wNS0wMyAgQ2hyaXMg RHVtZXogIDxjZHVtZXpAYXBwbGUuY29tPgorCiAgICAgICAgIFtXSzJdIEV4dGVuZCBwcm9jZXNz RGlkQ3Jhc2ggZGVsZWdhdGUgdG8gbGV0IHRoZSBjbGllbnQga25vdyB0aGUgcmVhc29uIGZvciB0 aGUgY3Jhc2gKICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTE3MTU2NQogICAgICAgICA8cmRhcjovL3Byb2JsZW0vMzEyMDQ0MTc+CmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvV2ViUGFnZVByb3h5LmNwcCBiL1NvdXJjZS9XZWJLaXQy L1VJUHJvY2Vzcy9XZWJQYWdlUHJveHkuY3BwCmluZGV4IGI2NWI2MTk5NzA5ZmQxZTIxZGI5N2Ex ZDA0YzY5NTk0ZWFlMGJlMzMuLjg5MDA2YWJjOGEwYjQ3NjVmOTBlYTIwM2I0NDcyY2NkOGFlMzI3 MjggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9XZWJQYWdlUHJveHkuY3Bw CisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9XZWJQYWdlUHJveHkuY3BwCkBAIC0yNDEy LDcgKzI0MTIsNiBAQCB2b2lkIFdlYlBhZ2VQcm94eTo6dGVybWluYXRlUHJvY2VzcygpCiAgICAg ICAgIHJldHVybjsKIAogICAgIG1fcHJvY2Vzcy0+cmVxdWVzdFRlcm1pbmF0aW9uKCk7Ci0gICAg cmVzZXRTdGF0ZUFmdGVyUHJvY2Vzc0V4aXRlZCgpOwogfQogCiBTZXNzaW9uU3RhdGUgV2ViUGFn ZVByb3h5OjpzZXNzaW9uU3RhdGUoY29uc3Qgc3RkOjpmdW5jdGlvbjxib29sIChXZWJCYWNrRm9y d2FyZExpc3RJdGVtJik+JiBmaWx0ZXIpIGNvbnN0Cg==