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

Description Chris Dumez 2019-04-10 10:19:02 PDT
Audio sometimes keeps playing in previous process after a process-swap since r240562.
Comment 1 Chris Dumez 2019-04-10 10:19:39 PDT
<rdar://problem/49460572>
Comment 2 Chris Dumez 2019-04-10 10:32:00 PDT
Created attachment 367136 [details]
Patch
Comment 3 Chris Dumez 2019-04-10 11:06:17 PDT
Created attachment 367141 [details]
Patch
Comment 4 Alex Christensen 2019-04-10 12:54:05 PDT
Comment on attachment 367141 [details]
Patch

This looks good.  I lament the lack of tests.
Comment 5 Chris Dumez 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.
Comment 6 Chris Dumez 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>
Comment 7 Chris Dumez 2019-04-10 12:56:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Ryan Haddad 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
Comment 9 Chris Dumez 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.
Comment 10 Chris Dumez 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.
Comment 11 Chris Dumez 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.
Comment 12 Chris Dumez 2019-04-11 06:31:39 PDT
Follow up fix landed in https://trac.webkit.org/changeset/244186