WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
171616
REGRESSION (
r216129
): ASSERTION FAILED: m_process->state() == WebProcessProxy::State::Terminated
https://bugs.webkit.org/show_bug.cgi?id=171616
Summary
REGRESSION (r216129): ASSERTION FAILED: m_process->state() == WebProcessProxy...
Ryan Haddad
Reported
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
Attachments
Patch
(1.70 KB, patch)
2017-05-03 14:27 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Ryan Haddad
Comment 1
2017-05-03 13:59:34 PDT
Started with
https://trac.webkit.org/changeset/216129/webkit
Chris Dumez
Comment 2
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.
Chris Dumez
Comment 3
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.
Chris Dumez
Comment 4
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.
Chris Dumez
Comment 5
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().
Chris Dumez
Comment 6
2017-05-03 14:27:05 PDT
Created
attachment 308953
[details]
Patch
Chris Dumez
Comment 7
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.
Chris Dumez
Comment 8
2017-05-03 14:36:38 PDT
Comment on
attachment 308953
[details]
Patch API tests are passing in Debug locally with this fix.
Chris Dumez
Comment 9
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
>
Chris Dumez
Comment 10
2017-05-03 14:56:40 PDT
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 11
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.
Chris Dumez
Comment 12
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
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug