Summary: | Regression(r240562) Audio sometimes keeps playing in previous process after a process-swap | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, beidson, ggaren, koivisto, ryanhaddad, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=197799 | ||||||||
Bug Depends on: | 196784 | ||||||||
Bug Blocks: | 193716 | ||||||||
Attachments: |
|
Description
Chris Dumez
2019-04-10 10:19:02 PDT
Created attachment 367136 [details]
Patch
Created attachment 367141 [details]
Patch
Comment on attachment 367141 [details]
Patch
This looks good. I lament the lack of tests.
(In reply to Alex Christensen from comment #4) > Comment on attachment 367141 [details] > Patch > > This looks good. I lament the lack of tests. Yes, I wasn't able to find a way to API test this. Comment on attachment 367141 [details] Patch Clearing flags on attachment: 367141 Committed r244144: <https://trac.webkit.org/changeset/244144> All reviewed patches have been landed. Closing bug. (In reply to Chris Dumez from comment #5) > (In reply to Alex Christensen from comment #4) > > Comment on attachment 367141 [details] > > Patch > > > > This looks good. I lament the lack of tests. > > Yes, I wasn't able to find a way to API test this. It did manage to break one though, as shown by EWS: https://ews-build.webkit.org/#/builders/3/builds/385 (In reply to Ryan Haddad from comment #8) > (In reply to Chris Dumez from comment #5) > > (In reply to Alex Christensen from comment #4) > > > Comment on attachment 367141 [details] > > > Patch > > > > > > This looks good. I lament the lack of tests. > > > > Yes, I wasn't able to find a way to API test this. > It did manage to break one though, as shown by EWS: > https://ews-build.webkit.org/#/builders/3/builds/385 Will look now. (In reply to Chris Dumez from comment #9) > (In reply to Ryan Haddad from comment #8) > > (In reply to Chris Dumez from comment #5) > > > (In reply to Alex Christensen from comment #4) > > > > Comment on attachment 367141 [details] > > > > Patch > > > > > > > > This looks good. I lament the lack of tests. > > > > > > Yes, I wasn't able to find a way to API test this. > > It did manage to break one though, as shown by EWS: > > https://ews-build.webkit.org/#/builders/3/builds/385 > > Will look now. Looks like that API test is flaky. Comment on attachment 367141 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367141&action=review > Source/WebKit/UIProcess/SuspendedPageProxy.cpp:172 > + m_process->processPool().removeSuspendedPage(*this); // Will destroy |this|. I introduced an unintentional behavior change here by destroying the SuspendedPageProxy instead of closing it. Fixing via https://bugs.webkit.org/show_bug.cgi?id=196784 > Source/WebKit/UIProcess/WebPageProxy.cpp:-7091 > - m_process->processPool().closeFailedSuspendedPagesForPage(*this); Note that this was closing the SuspendedPage, ... > Source/WebKit/UIProcess/WebProcessPool.cpp:-2409 > - suspendedPage->close(); ... here, not destroying it. Follow up fix landed in https://trac.webkit.org/changeset/244186 |