Summary: | REGRESSION (r216129): ASSERTION FAILED: m_process->state() == WebProcessProxy::State::Terminated | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||
Component: | New Bugs | Assignee: | 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
Ryan Haddad
2017-05-03 13:59:14 PDT
Started with https://trac.webkit.org/changeset/216129/webkit (In reply to Ryan Haddad from comment #1) > Started with https://trac.webkit.org/changeset/216129/webkit Looking now. 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. 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. (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(). Created attachment 308953 [details]
Patch
(In reply to Chris Dumez from comment #6) > Created attachment 308953 [details] > Patch Still building locally to confirm the fix. Comment on attachment 308953 [details]
Patch
API tests are passing in Debug locally with this fix.
Comment on attachment 308953 [details] Patch Clearing flags on attachment: 308953 Committed r216144: <http://trac.webkit.org/changeset/216144> All reviewed patches have been landed. Closing bug. > 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.
(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. |