Bug 196774

Summary: Regression(r240562) Audio sometimes keeps playing in previous process after a process-swap
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: 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 Flags
Patch
none
Patch none

Chris Dumez
Reported 2019-04-10 10:19:02 PDT
Audio sometimes keeps playing in previous process after a process-swap since r240562.
Attachments
Patch (9.47 KB, patch)
2019-04-10 10:32 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (9.50 KB, patch)
2019-04-10 11:06 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2019-04-10 10:19:39 PDT
<rdar://problem/49460572>
Chris Dumez
Comment 2 2019-04-10 10:32:00 PDT
Created attachment 367136 [details] Patch
Chris Dumez
Comment 3 2019-04-10 11:06:17 PDT
Created attachment 367141 [details] Patch
Alex Christensen
Comment 4 2019-04-10 12:54:05 PDT
Comment on attachment 367141 [details] Patch This looks good. I lament the lack of tests.
Chris Dumez
Comment 5 2019-04-10 12:55:05 PDT
(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.
Chris Dumez
Comment 6 2019-04-10 12:56:48 PDT
Comment on attachment 367141 [details] Patch Clearing flags on attachment: 367141 Committed r244144: <https://trac.webkit.org/changeset/244144>
Chris Dumez
Comment 7 2019-04-10 12:56:50 PDT
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 8 2019-04-10 14:28:01 PDT
(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
Chris Dumez
Comment 9 2019-04-10 14:28:57 PDT
(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.
Chris Dumez
Comment 10 2019-04-10 14:30:02 PDT
(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.
Chris Dumez
Comment 11 2019-04-10 16:51:28 PDT
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.
Chris Dumez
Comment 12 2019-04-11 06:31:39 PDT
Follow up fix landed in https://trac.webkit.org/changeset/244186
Note You need to log in before you can comment on or make changes to this bug.